-
Notifications
You must be signed in to change notification settings - Fork 20
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
feat: extending with rustup components and external subcommands #31
base: master
Are you sure you want to change the base?
Conversation
8cc9fa2
to
9dbb321
Compare
c06c366
to
b564759
Compare
How difficult would it be to make these changes without adding new dependencies? Most users of this project download and compile it in CI so it's important for it to be a small binary that compiles fast. |
Im not the owner not maintainer if this crate, but here are my 2 cents. All the crates used resemble a milestone in the evolution of a language. Parsing with
|
I feel like it would be good to depend on serde and an argument parser at the least, |
Besides, at least on GHA you can cache these installs |
I get the case of "compatibility" and "ease of use", but at the same time. Things like |
I would bet most projects in the Rust ecosystem use So before I review this, it would be helpful to know how much longer it takes to download + compile this branch on GitHub actions, compared to |
Well Will do these benchmarks on Friday, @frewsxcv do you have a threshold? |
So before committing to scripting I wanted to quickly check if the compile times are out of this world. I used my gitpod instance for this, as I do not have a laptop or pc. Build times: Yes, I quickly checked with Couldn't find much more optimizations to reduce build time. But I feel like the other depdencies are worth the 10 secs added considering all the stuff this crate doesn't have to fear about. As said, this was just a quick check, these aren't github actions build times, will do them next |
src/bin/common/deprecated_glue.rs
Outdated
.collect(); | ||
|
||
println!( | ||
"{}: the command `cargo {}` may be deprecated, please use `cargo all-features build`", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"{}: the command `cargo {}` may be deprecated, please use `cargo all-features build`", | |
"{}: the command `cargo {}` is deprecated, please use `cargo all-features build`", |
README.md
Outdated
- name: Install cargo-all-features | ||
uses: actions-rs/cargo@v1 | ||
with: | ||
command: install cargo-all-features |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should suggest passing the argument that specifies a version to install. That could make it easier to introduce breaking changes in the future 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a fan of the idea, but I do not really think many users will manually change the version inside the commands input as this would not be covered by something like dependabot.
One suggestion would be create a gh action for cargo-all-features itself, this could mean a 1:1 mapping between gh-action version and crate version which then could be updated using dependabot.
im so so sorry, thought could do some stuff between final exams but they hit real hard. Will do things this comming weekend |
This is great! Do you plan to also add |
@TimDiekmann added it @frewsxcv implemented your suggestions Also, did some installs for all major platforms over at https://github.com/somehowchris/cargo-all-features-install-testing/actions/runs/2601011045 Results are similar for all platforms for gh actions, adding about 30-50 seconds |
The last commit added IMO for ci usage, having binaries is a nice thing as to not having to rebuild it every time (cost and efficiency). Also the binaries are install procedure agnostic, so you could also curl and run them, cargo binstall is just a lovely cargo tooling |
README.md
Outdated
- [`cargo miri test`](https://github.com/rust-lang/miri) for testing using miri -> _rustup component `miri` is needed_ | ||
- Cargo plugins | ||
- [`cargo udeps`](https://github.com/est31/cargo-udeps) to analyze for unused dependencies -> _cargo plugin `cargo-udeps` is needed_ | ||
- [`cargo tarpaulin`](https://github.com/xd009642/tarpaulin) to analyze for unused dependencies -> _cargo plugin `cargo-tarpaulin` is needed_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tarpaulin
is a code coverage reporting tool, and seems like not a unused dependencies analyzer:
Tarpaulin is a code coverage reporting tool for the Cargo build system, named for a waterproof cloth used to cover cargo on a ship. Currently, tarpaulin provides working line coverage and while fairly reliable may still contain minor inaccuracies in the results. A lot of work has been done to get it working on a wide range of projects, but often unique combinations of packages and build features can cause issues so please report anything you find that's wrong. Also, check out our roadmap for planned features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sry, yes totally missed that copy paste error, thanks
Besides, I can't pass |
how about nvm, my bad. clap seems to get and compare what the possible values provided are and tries to compare them. wasn't like that when I originally rewrote that. just removed the possible values part |
What's the state of this? I'm really looking forward to using cargo-all-features with miri in CI. |
Chunking and I think what should be done here is splitting this into a lot of smaller changes, like all of the refactorings and some of the QoL improvements, and landing those first before adding tons to the CLI. |
Also really interested in using @somehowchris Do you need help? |
didn't receive any words from @frewsxcv, saw some things being integrated but personally moved away from this as I didn't hear anything. is there a need to create a new PR with some of the this from this PR or should it be considered as failed and therefore closed? I personally still believe that many of the issues would be fixed with the changes in this PR |
It would be nice to split this into smaller things to make it more easily reviewed if possible. |
Definitely not finished, but just wanna show my intent and get some feedback on the way.
Followup (16.4.2022):
It's kind of a big PR 😅 but it's not all new code.
General approaches:
validator
dyn Error
clap
for anything related to the cli interation aka frontend (mentioned once in Support chunking #28 by @Manishearth)Specific changes:
cargo all-features
, and let other commands run with a warning for deprecation Binary unification #22--dry-run
and--verbose
flags Add flags to print dry run of commands to support concurrent runs #7CargoCommandOrigin
and its logic with a lookup table to allow other commands to be added Cargo udeps integration #26 miri support #15 Support for other commands #5yansi
fortermcolor
to reduce boilerplate--no-color
to allow disabling colors--target-command
to allow users to specify if the features should be forwarded tocross
orcargo
Breaking changes
First of all the binary would be unified as
cargo all-features ....
but the old subcommands would just run with the old cli logic and a small warning.The bigger breaking change would be the layout of the flags supplied to the subcommand vs the actual command being called by this crate. (almost like this mention #4 (comment))
As you can see there is a
--
separating the two parts, this wasn't the case before but clap introduces best practises. If there would be the desicion to somehow circumvent such a syntax change this would mean it is much harder not only for this crates parting strategy but also cargo itself to tell them apart and in case ofcross
options like--target
would be ignored.The main business logic has not been touched, but I'm still testing in the wild.
Feedback to anything is very much welcome 🥂