cargo
cargo copied to clipboard
rust-lang
cargo: prevent dashes in lib.name
The TOML parser of Cargo currently refuses lib.name entries that contain dashes. Unfortunately, it uses the package-name as default if no explicit lib.name entry is specified. This package-name, however, can contain dashes.
Cargo documentation states that the package name is converted first, yet this was never implemented by the code-base.
Fix this inconsistency and convert the package name to a suitable crate-name first.
This fixes #12780. It is an alternative to #12640.
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @weihanglo (or someone else) soon.
Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:
-
@rustbot author: the review is finished, PR author should check the comments and take action accordingly -
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue
@rfcbot fcp merge
When auto-filling lib.name, cargo has a discrepancy
- documentation: says lib.name will use
_ - implementation: any
-->_conversion happens later in the process, missing some areas likecargo metadataand environment variables set for artifact dependencies
We talked in the cargo team meeting about leaning towards making the code match the documentation. This FCP is a strawpoll to confirm that.
In stable rust, this only affects the lib name in cargo metadata. I've been looking for any other way this may manifest, including our environment variables and haven't seen any so far.
Team member @epage has proposed to merge this. The next step is review by the rest of the tagged team members:
- [x] @Eh2406
- [ ] @Muscraft
- [x] @arlosi
- [x] @ehuss
- [x] @epage
- [ ] @joshtriplett
- [x] @weihanglo
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.
Hm, I didn't think about the consequence of this impacting the artifact-dependency environment variables, which appears to be a breaking change. For example, I did a quick code search and found https://github.com/lf-/clipper/blob/dd1caf13191e0fa58afbc3680fb83782c20e63d2/fixtures/dlopen-openssl-fixture/src/main.rs#L9 which would break with this change.
I'm not sure what to do about that. Perhaps one option is to set both environment variables for backwards compatibility?
The RFC says the env-var is named after the "crate", so this is actually a fix. But that is more of a theoretical argument, it is still a breaking change.
. Perhaps one option is to set both environment variables for backwards compatibility?
If that works, it seems reasonable.
(update: we now remember whether target-names were inferred, which in turn allows us to keep setting the previously used artifact env-vars)
The bindeps and multidep features of Cargo are unstable, but the new revision of this PR tries to avoid breakage in the env-vars of these features. Unfortunately, dash->underscore conversion is lossy, so we cannot reliably tell whether compatibility-mode is required. Therefore, I extended the Cargo-internal manifest state to remember whether a target-name was inferred, or whether it is explicit. Now we know whether a target-name used to be different in the past, and we can use this information to keep backwards-compatibility.
Note that I think the stripped env-var of bindeps is quite confusing, since changing the dep-name locally will affect which env-var becomes the stripped one. I think I did my best to retain this behavior, so this should be fine.
@Eh2406, @Muscraft, @joshtriplett do you think you can check the FCP above? Do you have any questions or concerns? Anything that can help you understand this change?
The final comment period, with a disposition to merge, as per the review above, is now complete.
As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.
This will be merged soon.
:pushpin: Commit 3ca04e261e1abde062b1c58a31a0ae622547d972 has been approved by ehuss
It is now in the queue for this repository.
:hourglass: Testing commit 3ca04e261e1abde062b1c58a31a0ae622547d972 with merge 69726309bf12443a618643cc86125ef60dcbecfb...
:sunny: Test successful - checks-actions Approved by: ehuss Pushing 69726309bf12443a618643cc86125ef60dcbecfb to master...