-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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(toml): Add autolib
#14591
feat(toml): Add autolib
#14591
Conversation
Starting FCP in parallel to the PR review @rfcbot fcp merge |
Team member @epage has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
PR rust-lang#5335 added `autobins`, etc for rust-lang#5330. Nowhere in there is discussion of `autolib`. Cargo script disables support for additional build-targets by disabling discovery. Except we don't have a way to disable discovery of `autolib`, leading to rust-lang#14476. By adding `autolib`, we can continue in that direction. This also allows us to bypass inferring of libs on published packages, like all other build-targets which were handled in rust-lang#13849. As this seems fairly low controversy, this insta-stabilizes the field. In prior versions of Cargo, users will get an "unused manifest key" warning. For packags where this is set by `cargo publish`, the warning will be suppressed and things will work as normal. For `cargo vendor`, the same except there will be some churn in the vendored source as this field will now be set. For local development, it should be rare to set `autolib` so the lack of error by discovering a file when this is set shouldn't be a problem. Fixes rust-lang#14476
🔔 This is now entering its final comment period, as per the review above. 🔔 |
@@ -173,6 +173,7 @@ pub struct TomlPackage { | |||
pub publish: Option<InheritableVecStringOrBool>, | |||
pub workspace: Option<String>, | |||
pub im_a_teapot: Option<bool>, | |||
pub autolib: Option<bool>, |
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.
Everything looks good, except this could potentially break other third-party build tools like cargo-manifest
in the future. I'd like to give them a heads-up in https://rust-lang.zulipchat.com/#narrow/stream/334885-t-cargo.2Fbuild-integration and merge on Friday. Any other opinion?
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.
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.
Thanks for taking care of that!
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.
cc @kornelski and @LukeMathWalker since you're maintainers of relevant crates.
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.
cc @Turbo87 who is covering for me while I'm on pat leave
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.
yep yep, already aware of it :)
25310c7
to
5e35e27
Compare
It's Friday! Let us not wait for the entire FCP. Feel free to comment below if you have any further concerns. @bors r+ |
☀️ Test successful - checks-actions |
Update cargo 19 commits in eaee77dc1584be45949b75e4c4c9a841605e3a4b..80d82ca22abbee5fb7b51fa1abeb1ae34e99e88a 2024-09-19 21:10:23 +0000 to 2024-09-27 17:56:01 +0000 - Update cc to 1.1.22 (rust-lang/cargo#14607) - feat: lockfile path implies --locked on cargo install (rust-lang/cargo#14556) - feat(toml): Add `autolib` (rust-lang/cargo#14591) - fix: correct error count for `cargo check --message-format json` (rust-lang/cargo#14598) - test: relax panic output assertion (rust-lang/cargo#14602) - feat(timings): support dark color scheme in HTML output (rust-lang/cargo#14588) - feat: add CARGO_MANIFEST_PATH env variable (rust-lang/cargo#14404) - fix(config): Don't double-warn about `$CARGO_HOME/config` (rust-lang/cargo#14579) - fix(cargo-rustc): give trailing flags higher precedence on nightly (rust-lang/cargo#14587) - feat: make lockfile v4 the default (rust-lang/cargo#14595) - perf: Improve quality of completion performance traces (rust-lang/cargo#14592) - test: Remove completion tests (rust-lang/cargo#14590) - feat: Add support for completing `cargo update <TAB>` (rust-lang/cargo#14552) - test: Migrate remaining with_stdout/with_stderr calls (rust-lang/cargo#14577) - fix(resolve): Improve multi-MSRV workspaces (rust-lang/cargo#14569) - chore: Bump MSRV to 1.81 (rust-lang/cargo#14585) - Add a `--dry-run` flag to the `install` command (rust-lang/cargo#14280) - fix(resolve): Don't list transitive, incompatible dependencies as available (rust-lang/cargo#14568) - feat(complete): Upgrade clap_complete (rust-lang/cargo#14573)
This adds support for parsing the new `autolib` member (rust-lang/cargo#14591). This is a quick and easy fix, suitable for backport. A larger more involved fix will be to stop splatting out the cargo dictionaries, and instead pass the fields we know about 1 by 1, and warning the user if we come across unknown fields. Fixes: mesonbuild#13826
This adds support for parsing the new `autolib` member (rust-lang/cargo#14591). This is a quick and easy fix, suitable for backport. A larger more involved fix will be to stop splatting out the cargo dictionaries, and instead pass the fields we know about 1 by 1, and warning the user if we come across unknown fields. Fixes: mesonbuild#13826
This adds support for parsing the new `autolib` member (rust-lang/cargo#14591). This is a quick and easy fix, suitable for backport. A larger more involved fix will be to stop splatting out the cargo dictionaries, and instead pass the fields we know about 1 by 1, and warning the user if we come across unknown fields. Fixes: #13826
What does this PR try to resolve?
PR #5335 added
autobins
, etc for #5330. Nowhere in there isdiscussion of
autolib
.Cargo script disables support for additional build-targets by disabling
discovery.
Except we don't have a way to disable discovery of
autolib
, leading to #14476.By adding
autolib
, we can continue in that direction.This also allows us to bypass inferring of libs on published packages,
like all other build-targets which were handled in #13849.
Fixes #14476
How should we test and review this PR?
Additional information
As this seems fairly low controversy, this insta-stabilizes the field.
In prior versions of Cargo, users will get an "unused manifest key"
warning.
For packages where this is set by
cargo publish
, the warning will be suppressed and things will work as normal.For
cargo vendor
, the same except there will be some churn in thevendored source as this field will now be set.
For local development, it should be rare to set
autolib
so the lack oferror by discovering a file when this is set shouldn't be a problem.