Adding feature-flags to `cargo publish` and `cargo package` #6453

spacekookie · 2018-12-17T13:47:15Z

This change adds the --features and --all-features flags to the
above mentioned commands. This is to allow publishing of crates that
have certain feature requirements for compilation, without having to
rely on --no-verify which isn't good practise.

This PR adds two new fields features and all_features to both the
PackageOpts and PublishOpts and also adds the argument options
to the CLI commands.

Merging this feature will allow people to publish crates on crates.io
that require some feature flag to compile (or all?) without needing
to rely on not checking the package before uploading, potentially
resulting in less packaging errors and broken packages.

rust-highfive · 2018-12-17T13:47:19Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

This PR adds `features` and `all_features` as both flags to the CLI as well as the `release.toml` configuration. This change allows you to tell `cargo-release` to verify the package with certain features enabled, in order to avoid needing to use `no-verify` for crates that depend on certain features being enabled (or simply any feature) before they can be uploaded. This change is dependent on rust-lang/cargo#6453, which adds this feature to the base `cargo publish` command. The motivation behind this PR is the same as the above mentioned cargo PR: reducing the amount of packaging errors that can happen due to people not being able to verify their code because it has some feature requirements for compilation, ultimately resulting in a more healthy package ecosystem.

Eh2406 · 2018-12-17T15:07:09Z

This makes sense to me, but I am not comfortable adding a insta stable argument on my own.
@alexcrichton do you have an opinion on adding this?
@nrc how does this work with the feature freeze?

ehuss · 2018-12-17T15:45:03Z

I'm not sure I understand the use case here. If you publish a package like this, and I depend on it, it will also fail to compile? Can you explain more about why you need it?

As an aside, this is missing --no-default-features and tests.

spacekookie · 2018-12-17T16:02:22Z

@ehuss it would fail to compile with an error like "please provide either feature a, b or c".

That's wanted behaviour because maybe it it's not obvious which feature should be used by default and an explicit choice is the most desirable.

The crate should still be publishable though, thus this change.

I will look into writing some tests for this later though

alexcrichton · 2018-12-17T16:49:14Z

I think this is fine to add, even as insta stable, personally. This is following the clear precedent of other subcommands, and while I don't necessarily personally think it's good for packages to require having a feature activated, that's not really up to us to pass a judgment on!

I'd be ok r+'ing this with some tests!

bors · 2018-12-20T16:14:43Z

☔ The latest upstream changes (presumably #6466) made this pull request unmergeable. Please resolve the merge conflicts.

This change adds the `--features` and `--all-features` flags to the above mentioned commands. This is to allow publishing of crates that have certain feature requirements for compilation, without having to rely on `--no-verify` which isn't good practise. This PR adds two new fields `features` and `all_features` to both the `PackageOpts` and `PublishOpts` and also adds the argument options to the CLI commands. Merging this feature will allow people to publish crates on crates.io that require some feature flag to compile (or all?) without needing to rely on not checking the package before uploading, potentially resulting in less packaging errors and broken packages.

spacekookie · 2018-12-29T11:11:57Z

ping @alexcrichton, there's tests now and they pass 🎉

ehuss

Also, this still needs to add support for --no-default-features.

ehuss · 2018-12-29T13:26:21Z

src/cargo/ops/cargo_package.rs

@@ -26,6 +26,9 @@ pub struct PackageOpts<'cfg> {
    pub verify: bool,
    pub jobs: Option<u32>,
    pub target: Option<String>,
+    pub registry: Option<String>,


Why is registry added if it is never used?

Good catch!

I copied the registry field over from somewhere else and then implemented the init without realising it was actually never used 😅

ehuss · 2018-12-29T13:27:06Z

tests/testsuite/publish.rs

@@ -23,8 +23,7 @@ fn simple() {
            license = "MIT"
            description = "foo"
        "#,
-        )
-        .file("src/main.rs", "fn main() {}")
+        ).file("src/main.rs", "fn main() {}")


All these formatting changes need to be removed.

ehuss · 2018-12-29T13:28:10Z

tests/testsuite/package.rs

+        .file(
+            "Cargo.toml",
+            r#"
+            cargo-features = ["alternative-registries"]


Why do these tests have this cargo-feature?

ehuss · 2018-12-29T13:31:30Z

tests/testsuite/publish.rs

+             fn main() {}",
+        ).build();
+
+    p.cargo("publish --registry alternative -Zunstable-options --features required")


I'm not sure why these tests are set up this way with alternative registries. You can publish to a local (fake) registry. See simple for an example.

Hey, so I finally got the chance to look at this again.

I think the reason why most of the other tests are using the alternative registry is so that they can omit options from the Cargo.toml, which is technically not valid (anymore?).

Without an alternative registry I'll need to provide an author which is fine by itself, just means that it would be the only test in the file to provide one.

I opted for "do it the same way all the other tests work" over trying to be clever.

Do you think that's okay? Otherwise I'll happily change the test and just add more fields to the manifest as are needed.

The nearby tests that are using alternative registries are doing so to test the publish field which requires that feature.

I'm not sure I understand what you mean by the author field, since there is one in the test. Here's the style I'm thinking of:

diff --git a/tests/testsuite/publish.rs b/tests/testsuite/publish.rs index dca4e92c..7b3dce40 100644 --- a/tests/testsuite/publish.rs +++ b/tests/testsuite/publish.rs @@ -812,15 +812,12 @@ fn publish_with_select_features() { .file( "Cargo.toml", r#" - cargo-features = ["alternative-registries"] - [project] name = "foo" version = "0.0.1" authors = [] license = "MIT" description = "foo" - publish = [] [features] required = [] @@ -829,20 +826,15 @@ fn publish_with_select_features() { ) .file( "src/main.rs", - "#[cfg(not(required))] + "#[cfg(not(feature=\"required\"))] compile_error!(\"This crate requires `required` feature!\"); fn main() {}", ).build(); - p.cargo("publish --registry alternative -Zunstable-options --features required") - .masquerade_as_nightly_cargo() - .with_status(101) - .with_stderr( - "\ -[ERROR] some crates cannot be published. -`foo` is marked as unpublishable -", - ).run(); + p.cargo("publish --features required --index") + .arg(publish::registry().to_string()) + .with_stderr_contains("[UPLOADING] foo v0.0.1 ([CWD])") + .run(); } #[test]

Hmm...so the problem with authors = [] was that it was complaining about it being empty. But I just realised I missed your point about the --index flag which seems to fix this issue.

spacekookie · 2019-01-09T17:26:53Z

Should be good now, @Eh2406 please let me know if there's anything else I've missed

Eh2406 · 2019-01-09T17:41:45Z

I will give others a chance to speak up, but looks good to me.

alexcrichton · 2019-01-10T17:10:57Z

@bors: r+

bors · 2019-01-10T17:10:58Z

📌 Commit b29e379 has been approved by alexcrichton

bors · 2019-01-10T18:37:28Z

⌛ Testing commit b29e379 with merge c9fa7db...

Adding feature-flags to `cargo publish` and `cargo package` This change adds the `--features` and `--all-features` flags to the above mentioned commands. This is to allow publishing of crates that have certain feature requirements for compilation, without having to rely on `--no-verify` which isn't good practise. This PR adds two new fields `features` and `all_features` to both the `PackageOpts` and `PublishOpts` and also adds the argument options to the CLI commands. Merging this feature will allow people to publish crates on crates.io that require some feature flag to compile (or all?) without needing to rely on not checking the package before uploading, potentially resulting in less packaging errors and broken packages.

bors · 2019-01-10T19:39:45Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: alexcrichton
Pushing c9fa7db to master...

therealprof · 2019-01-11T09:06:06Z

@alexcrichton There's a very prominent usecase in embedded where we want to provide hardware specific abstractions for multiple devices and the user needs to select the target hardware. Neither does it make sense to allow not selecting hardware (in some cases the crate wouldn't even compile then because of lower level dependencies) nor would it make sense to provide a default.

Up to now the common workarounds were:

--no-verify 😞
Specify a default target
Use a meta-feature and cfg-gate everything which would not compile without a target specified, e.g. as done here with device-selected https://github.com/stm32-rs/stm32f0xx-hal/blob/master/Cargo.toml

I'm very psyched about this landing! 🎉

spacekookie · 2019-01-11T16:13:24Z

@therealprof I think diesel-rs/diesel has so far only been publishable with --no-verify.

I'm glad it's having impact in use cases I hadn't even considered originally 🙂

@alexcrichton

Update cargo 13 commits in 34320d212dca8cd27d06ce93c16c6151f46fcf2e..2b4a5f1f0bb6e13759e88ea9512527b0beba154f 2019-01-03 19:12:38 +0000 to 2019-01-12 04:13:12 +0000 - Add test for publish with [patch] + cleanup. (rust-lang/cargo#6544) - Fix clippy warning (rust-lang/cargo#6546) - Revert "Workaround by using yesterday's nightly" (rust-lang/cargo#6540) - Adding feature-flags to `cargo publish` and `cargo package` (rust-lang/cargo#6453) - Fix the Travis CI badge (rust-lang/cargo#6530) - Add helpful text for Windows exceptions like Unix (rust-lang/cargo#6532) - Report fix bugs to Rust instead of Cargo (rust-lang/cargo#6531) - --{example,bin,bench,test} with no argument now lists all available targets (rust-lang/cargo#6505) - Rebuild on mid build file modification (rust-lang/cargo#6484) - Derive Clone for TomlDependency (rust-lang/cargo#6527) - publish: rework the crates.io detection logic. (rust-lang/cargo#6525) - avoid duplicates in ignore files (rust-lang/cargo#6521) - Rustflags in metadata (rust-lang/cargo#6503) r? @alexcrichton

Add documentation for new package/publish feature flags. Added in #6453.

This PR adds `features` and `all_features` as both flags to the CLI as well as the `release.toml` configuration. This change allows you to tell `cargo-release` to verify the package with certain features enabled, in order to avoid needing to use `no-verify` for crates that depend on certain features being enabled (or simply any feature) before they can be uploaded. This change is dependent on rust-lang/cargo#6453, which adds this feature to the base `cargo publish` command. The motivation behind this PR is the same as the above mentioned cargo PR: reducing the amount of packaging errors that can happen due to people not being able to verify their code because it has some feature requirements for compilation, ultimately resulting in a more healthy package ecosystem.

rust-highfive assigned Eh2406 Dec 17, 2018

spacekookie mentioned this pull request Dec 17, 2018

Adding feature-flags to release configuration crate-ci/cargo-release#75

Merged

spacekookie force-pushed the publish-featured branch from c067173 to 256fc39 Compare December 26, 2018 11:48

Adding feature tests to publish and package

142bafb

ehuss requested changes Dec 29, 2018

View reviewed changes

Undoing bad formatting changes and removing redundant struct fields

40cca80

spacekookie force-pushed the publish-featured branch from ec8f206 to 40cca80 Compare December 29, 2018 17:19

Fixing the publish tests

8c3bf82

Adding support for no-default-features to package and publish

b29e379

spacekookie force-pushed the publish-featured branch from 6bf0a97 to b29e379 Compare January 9, 2019 17:31

bors merged commit b29e379 into rust-lang:master Jan 10, 2019

therealprof mentioned this pull request Jan 11, 2019

Get rid of "device-selected"? stm32-rs/stm32f0xx-hal#44

Open

spacekookie deleted the publish-featured branch January 11, 2019 16:13

ehuss mentioned this pull request Jan 12, 2019

Update cargo rust-lang/rust#57559

Merged

ehuss mentioned this pull request Jan 15, 2019

Add documentation for new package/publish feature flags. #6553

Merged

bors added a commit that referenced this pull request Jan 15, 2019

Auto merge of #6553 - ehuss:doc-publish-features, r=alexcrichton

9b5d4b7

Add documentation for new package/publish feature flags. Added in #6453.

spacekookie mentioned this pull request Jan 21, 2019

Add mysql support rust-db/barrel#41

Merged

ehuss added this to the 1.33.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding feature-flags to `cargo publish` and `cargo package` #6453

Adding feature-flags to `cargo publish` and `cargo package` #6453

spacekookie commented Dec 17, 2018

rust-highfive commented Dec 17, 2018

Eh2406 commented Dec 17, 2018

ehuss commented Dec 17, 2018

spacekookie commented Dec 17, 2018

alexcrichton commented Dec 17, 2018

bors commented Dec 20, 2018

spacekookie commented Dec 29, 2018

ehuss left a comment

ehuss Dec 29, 2018

spacekookie Dec 29, 2018

ehuss Dec 29, 2018

ehuss Dec 29, 2018

ehuss Dec 29, 2018

spacekookie Jan 9, 2019

ehuss Jan 9, 2019

spacekookie Jan 9, 2019

spacekookie commented Jan 9, 2019

Eh2406 commented Jan 9, 2019

alexcrichton commented Jan 10, 2019

bors commented Jan 10, 2019

bors commented Jan 10, 2019

bors commented Jan 10, 2019

therealprof commented Jan 11, 2019

spacekookie commented Jan 11, 2019

Adding feature-flags to cargo publish and cargo package #6453

Adding feature-flags to cargo publish and cargo package #6453

Conversation

spacekookie commented Dec 17, 2018

rust-highfive commented Dec 17, 2018

Eh2406 commented Dec 17, 2018

ehuss commented Dec 17, 2018

spacekookie commented Dec 17, 2018

alexcrichton commented Dec 17, 2018

bors commented Dec 20, 2018

spacekookie commented Dec 29, 2018

ehuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacekookie commented Jan 9, 2019

Eh2406 commented Jan 9, 2019

alexcrichton commented Jan 10, 2019

bors commented Jan 10, 2019

bors commented Jan 10, 2019

bors commented Jan 10, 2019

therealprof commented Jan 11, 2019

spacekookie commented Jan 11, 2019

Adding feature-flags to `cargo publish` and `cargo package` #6453

Adding feature-flags to `cargo publish` and `cargo package` #6453