Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - Removed some non-needed dependencies #2246

Closed
wants to merge 1 commit into from

Conversation

Razican
Copy link
Member

@Razican Razican commented Aug 21, 2022

This Pull Request removes two dependencies that were not really needed, and fixes #2244 by no longer having the package in the dependency tree.

It changes the following:

  • The structopt dependency in boa_tester has been replaced by clap v3, the same way as we did in boa_cli. This means that we have one less dependency (at least), and that clap v2 is only used as a dev-dependency by criterion (which will probably be removed in 0.4, as per Possibly switch to a more lightweight CLI arg parsing library bheisler/criterion.rs#596).
  • The no-longer-updated num-format dependency has been removed from boa_tester. We were only using it to add comma thousands separator on results, so I added a simple function to do it (not very performant, but it will only be used a few times when showing results).

Looking at this, I noticed a couple of things:

  • The csv dependency, used by criterion has not been updated in more than a year, and it's using a very old itoa dependency. They updated the dependency in the repository in March, but unfortunately, the release is taking some more time than expected, and a tracking issue can be found here: Please release current master as 1.1.7 BurntSushi/rust-csv#271
  • cargo update fails, because the latest update to tinystr in the ICU4x breaks ICU4x 0.6. I have reported this here: Latest update to tinystr breaks compilation unicode-org/icu4x#2428 and their recommendation is for us to use a beta version of the library, but I don't think we should go for that, since this is a semver breakage.

@Razican Razican added the dependencies Pull requests that update a dependency file label Aug 21, 2022
@Razican Razican added this to the v0.16.0 milestone Aug 21, 2022
@Razican Razican linked an issue Aug 21, 2022 that may be closed by this pull request
@github-actions
Copy link

Test262 conformance changes

VM implementation

Test result main count PR count difference
Total 91,733 91,733 0
Passed 64,908 64,908 0
Ignored 14,750 14,750 0
Failed 12,075 12,075 0
Panics 0 0 0
Conformance 70.76% 70.76% 0.00%

@codecov
Copy link

codecov bot commented Aug 21, 2022

Codecov Report

Merging #2246 (f37d24f) into main (4f4a500) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2246   +/-   ##
=======================================
  Coverage   41.30%   41.31%           
=======================================
  Files         234      234           
  Lines       22014    22016    +2     
=======================================
+ Hits         9092     9095    +3     
+ Misses      12922    12921    -1     
Impacted Files Coverage Δ
boa_cli/src/main.rs 5.55% <ø> (ø)
boa_tester/src/main.rs 0.00% <ø> (ø)
boa_engine/src/builtins/function/mod.rs 23.71% <0.00%> (+0.79%) ⬆️
boa_engine/src/syntax/parser/mod.rs 26.78% <0.00%> (+0.89%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@raskad raskad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice finds!

Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks perfect to me! :)

@HalidOdat
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Aug 22, 2022
<!---
Thank you for contributing to Boa! Please fill out the template below, and remove or add any
information as you feel neccesary.
--->

This Pull Request removes two dependencies that were not really needed, and fixes #2244 by no longer having the package in the dependency tree.

It changes the following:

- The `structopt` dependency in `boa_tester` has been replaced by `clap` v3, the same way as we did in `boa_cli`. This means that we have one less dependency (at least), and that `clap` v2 is only used as a dev-dependency by `criterion` (which will probably be removed in 0.4, as per bheisler/criterion.rs#596).
- The no-longer-updated `num-format` dependency has been removed from `boa_tester`. We were only using it to add comma thousands separator on results, so I added a simple function to do it (not very performant, but it will only be used a few times when showing results).

Looking at this, I noticed a couple of things:

 - The `csv` dependency, used by `criterion` has not been updated in more than a year, and it's using a very old `itoa` dependency. They updated the dependency in the repository in March, but unfortunately, the release is taking some more time than expected, and a tracking issue can be found here: BurntSushi/rust-csv#271
 - `cargo update` fails, because the latest update to `tinystr` in the ICU4x breaks ICU4x 0.6. I have reported this here: unicode-org/icu4x#2428 and their recommendation is for us to use a beta version of the library, but I don't think we should go for that, since this is a semver breakage.
@bors
Copy link

bors bot commented Aug 22, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Removed some non-needed dependencies [Merged by Bors] - Removed some non-needed dependencies Aug 22, 2022
@bors bors bot closed this Aug 22, 2022
@bors bors bot deleted the dep_clean branch August 22, 2022 04:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RUSTSEC-2021-0139: ansi_term is Unmaintained
3 participants