cargo: prevent dashes in lib.name #12783
Conversation
|
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 (
|
rustbot
added
the
S-waiting-on-review
|
(update: fixed rustfmt complaints) |
|
@rfcbot fcp merge When auto-filling
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 |
|
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. |
rfcbot
added
proposed-final-comment-period
|
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. |
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 Note that I think the stripped env-var of |
Add a boolean state to `Target` that tells us whether the name of the target was inferred by Cargo, or whether it was directly specified in the Manifest. This value will be required in the future, to allow changing the inferred names of targets, but retaining enough information to keep backwards compatibility.
We are about to change the default value for target-names of libraries. They used to match the package-name. In the future, they will use the package-name with dashes converted to underscores. This will affect the artifact env-variables, since they expose target-names. Hence, set the old env-vars, too, to avoid breakage. Note that we do not retain the name of a target before it was converted, and the conversion is lossy, so we cannot reconstruct it. However, we can rely on the fact that the conversion only happens for default values (since user-supplied values never allowed dashes). Furthermore, we now remember whether a target-name was inferred, so we can exactly reconstruct whether a library-target could have contained dashes in older releases, or not.
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.
|
(update: rebase to master to fix CI failures) |
|
(Any update on this?) |
|
@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? |
|
Sorry for the delay in ✔️ |
rfcbot
added
the
final-comment-period
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
bors
added
the
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Update cargo 5 commits in 2fe739fcf16c5bf8c2064ab9d357f4a0e6c8539b..d438c80c45c24be676ef5867edc79d0a14910efe 2024-03-15 21:39:18 +0000 to 2024-03-19 16:11:22 +0000 - refactor(toml): Expose surce/spans for VirtualManifests (rust-lang/cargo#13603) - cargo/init: avoid target.name assignments if possible (rust-lang/cargo#13606) - chore: Fix minor grammar nit in command-line help (rust-lang/cargo#13602) - Bump to 0.80.0; update changelog (rust-lang/cargo#13604) - cargo: prevent dashes in lib.name (rust-lang/cargo#12783) r? ghost
Fix doc collision for lib/bin with a dash in the inferred name. This fixes an issue where `cargo doc` would report a collision if the project has a `-` in the name, and both a lib and bin. As a consequence of the change in #12783, the target name for the library no longer has a `-`. The code that checks for the lib/bin doc collision was only checking if the target names were equal, which is no longer the case after #12783. The solution here is to use the `crate_name` and not the target name, which has the appropriate `_` translation done. This is the more correct method to use, even before #12783, because rustdoc uses crate names, not target names (and thus even documenting bins have their filenames converted to underscores). Fixes #13628
Cargo disallows dashes in library target names (lib.name in Cargo.toml), but when not set it uses the package name. That meant that dashes snuck in and rust-lang/cargo#12783 fixes that. The targets Corrosion creates correspond to the library name, so after that change it's slint_cpp. To make this work with stable cargo, explicitly switch lib.name to slint_cpp.
Cargo disallows dashes in library target names (lib.name in Cargo.toml), but when not set it uses the package name. That meant that dashes snuck in and rust-lang/cargo#12783 fixes that. The targets Corrosion creates correspond to the library name, so after that change it's slint_cpp. To make this work with stable cargo, explicitly switch lib.name to slint_cpp.
Cargo disallows dashes in library target names (lib.name in Cargo.toml), but when not set it uses the package name. That meant that dashes snuck in and rust-lang/cargo#12783 fixes that. The targets Corrosion creates correspond to the library name, so after that change it's slint_cpp. To make this work with stable cargo, explicitly switch lib.name to slint_cpp.
Upgrades the Rust toolchain to `nightly-2024-03-21`. The relevant changes in Rust are: * rust-lang/rust#122480 * rust-lang/rust#122748 * rust-lang/cargo#12783 I wasn't confident that our regression testing could detect differences in the file paths being generated with the new logic, so I added code similar to the following just to check they were equivalent: ```rust let base_filename = tcx.output_filenames(()).output_path(OutputType::Object); + let binding = tcx.output_filenames(()).path(OutputType::Object); + let base_filename2 = binding.as_path(); + assert_eq!(base_filename, base_filename2); ``` Note that this was done for each instance where we used `output_path`, and completed regression testing with the previous toolchain version. I also checked manually the instance where we generate a `.dot` graph for debug purposes following the instructions [here](https://model-checking.github.io/kani/cheat-sheets.html?highlight=dot#debug) (a `libmain.dot` file was generated for the `main.rs` in one of my projects). --------- Co-authored-by: Celina G. Val <celinval@amazon.com>
Cargo disallows dashes in library target names (lib.name in Cargo.toml), but when not set it uses the package name. That meant that dashes snuck in and rust-lang/cargo#12783 fixes that. The targets Corrosion creates correspond to the library name, so after that change it's slint_cpp. To make this work with stable cargo, explicitly switch lib.name to slint_cpp.
Cargo disallows dashes in library target names (lib.name in Cargo.toml), but when not set it uses the package name. That meant that dashes snuck in and rust-lang/cargo#12783 fixes that. The targets Corrosion creates correspond to the library name, so after that change it's slint_cpp. To make this work with stable cargo, explicitly switch lib.name to slint_cpp.
Cargo disallows dashes in library target names (lib.name in Cargo.toml), but when not set it uses the package name. That meant that dashes snuck in and rust-lang/cargo#12783 fixes that. The targets Corrosion creates correspond to the library name, so after that change it's slint_cpp. To make this work with stable cargo, explicitly switch lib.name to slint_cpp.
Cargo disallows dashes in library target names (lib.name in Cargo.toml), but when not set it uses the package name. That meant that dashes snuck in and rust-lang/cargo#12783 fixes that. The targets Corrosion creates correspond to the library name, so after that change it's slint_cpp. To make this work with stable cargo, explicitly switch lib.name to slint_cpp.
Cargo disallows dashes in library target names (lib.name in Cargo.toml), but when not set it uses the package name. That meant that dashes snuck in and rust-lang/cargo#12783 fixes that. The targets Corrosion creates correspond to the library name, so after that change it's slint_cpp. To make this work with stable cargo, explicitly switch lib.name to slint_cpp.
Cargo disallows dashes in library target names (lib.name in Cargo.toml), but when not set it uses the package name. That meant that dashes snuck in and rust-lang/cargo#12783 fixes that. The targets Corrosion creates correspond to the library name, so after that change it's slint_cpp. To make this work with stable cargo, explicitly switch lib.name to slint_cpp.
Cargo disallows dashes in library target names (lib.name in Cargo.toml), but when not set it uses the package name. That meant that dashes snuck in and rust-lang/cargo#12783 fixes that. The targets Corrosion creates correspond to the library name, so after that change it's slint_cpp. To make this work with stable cargo, explicitly switch lib.name to slint_cpp.
The TOML parser of Cargo currently refuses
lib.nameentries that contain dashes. Unfortunately, it uses the package-name as default if no explicitlib.nameentry 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.