improvement: add option --no-default-features and --features
#92
Conversation
This is still not tested if it works. Signed-off-by: Soc Virnyl Estela <[email protected]>
There was a problem hiding this comment.
Thanks for working on this! Just some nits.
As far as a test case, the Rust tests in this project only test vendoring our own code, which doesn't have any features. However, if you look at the Github action flow, we test vendoring nushell, protobuf and netavark and at least nushell definitely has features.
Looks like in the current code wasi is optional so we could test with adding that as a feature?
…ng> and bool, respectively Signed-off-by: Soc Virnyl Estela <[email protected]>
this is to test new options: no-default-features and features Signed-off-by: Soc Virnyl Estela <[email protected]>
| @@ -312,6 +327,8 @@ impl VendorFilter { | |||
| let args_unset = args.platform.is_none() | |||
| && args.tier.is_none() | |||
| && args.all_features.is_none() | |||
| && !args.no_default_features | |||
There was a problem hiding this comment.
I guess in retrospect, we're inconsistent now with the Option<bool> for all_features...I don't remember why I did that now. The logic here is checking for "was the argument provided", but there's no way to do e.g. cargo vendor --all-features=false - you can only provide a single bit, so there's no distinguishing between "option unset" and "option set to false".
So we can do a followup cleanup to drop that Option too I think.
|
Oh hmm, unit tests aren't happy
Ah I bet we need a |
Args - `features` is just a Vec<String> so default is always empty - `no_default_features` is bool but set with default_value_t of false VendorFilter - `features` is just a Vec<String> set to default which is empty? and skip serializing if Vec is empty - `no_default_features` is set to default. default is always false for bool Signed-off-by: Soc Virnyl Estela <[email protected]>
uncomfyhalomacro
force-pushed
the
20240329-feature-flags
branch
from
529751f to
b5a90e2
Compare
|
I will attempt doing a cleanup with all features too. just a small quick change. |
…eatures Signed-off-by: Soc Virnyl Estela <[email protected]>
…ackages_for_platform too Signed-off-by: Soc Virnyl Estela <[email protected]>
…o-vendor-filterer again Signed-off-by: Soc Virnyl Estela <[email protected]>
uncomfyhalomacro
force-pushed
the
20240329-feature-flags
branch
from
10af33e to
55e8f06
Compare
…d. enabling them as features. we replace also `true` directory as `vendor`. I was confused of this at first... but it seems `true` was used as to set all-features to true instead of it being an output directory currently, to set `all-features` and `no-default-features`, you just have to add the flag without any parameters. removing the flags will just auto set it to false. Signed-off-by: Soc Virnyl Estela <[email protected]>
uncomfyhalomacro
force-pushed
the
20240329-feature-flags
branch
from
55e8f06 to
29f0b25
Compare
Signed-off-by: Soc Virnyl Estela <[email protected]>
.github/workflows/ci.yml
Outdated
| @@ -96,14 +96,14 @@ jobs: | |||
| # For netavark | |||
| - run: sudo apt install protobuf-compiler | |||
| - run: | | |||
| mkdir -p .cargo && cargo-vendor-filterer --platform x86_64-unknown-linux-gnu --all-features true > .cargo/config.toml | |||
| rm -rfv true | |||
There was a problem hiding this comment.
Heh wow yes, that was a confusing thing before...
|
(Note to all of us, I had to update the branch protection rules for the nushell version, I think we can simplify this in the future by dropping the version from the job name) |
|
@uncomfyhalomacro can you review #93 ? |
This is still not tested if it works.
Changes not documented
cargo build:Reference issues/prs: #91