Skip to content
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

cargo: fix inconsistent dashes in lib.name #12640

Closed
wants to merge 1 commit into from

Conversation

dvdhrm
Copy link
Contributor

@dvdhrm dvdhrm commented Sep 7, 2023

Change the cargo TOML parser to allow dashes in lib.name.

Cargo currently refuses explicit library names if they contain a dash. However, if no library name is specified, the package-name will be used, which itself can contain dashes. This leads to packages with lib.name containing dashes, if their package-name contains dashes.

Drop this requirement and allow explicit lib.name configurations to contain dashes.

The current ecosystem clearly allows lib.name to contain dashes, given that cargo metadata already reports such names for any package with a dash. It looks like an oversight that any explicit configuration is not allowed to include dashes.

All consumers of lib.name already use Target.crate_name() rather than Target.name() if a sanitized name is required. Therefore, they already replace dashes with underscores.

Last but not least, the current documentation is simply wrong in stating that lib.name defaults to the package-name with dashes replaced by underscores. Cargo never replaces dashes for lib.name, and cargo metadata and friends already happily show lib.name with dashes. Cargo merely replaces dashes with underscores to retrieve the crate-name of a target, which is then used as stem for library artifacts.

@rustbot
Copy link
Collaborator

rustbot commented Sep 7, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @epage (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

@rustbot rustbot added A-documenting-cargo-itself Area: Cargo's documentation S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 7, 2023
Change the cargo TOML parser to allow dashes in `lib.name`.

Cargo currently refuses explicit library names if they contain a dash.
However, if no library name is specified, the package-name will be used,
which itself can contain dashes. This leads to packages with `lib.name`
containing dashes, if their package-name contains dashes.

Drop this requirement and allow explicit `lib.name` configurations to
contain dashes.

The current ecosystem clearly allows `lib.name` to contain dashes,
given that `cargo metadata` already reports such names for any package
with a dash. It looks like an oversight that any explicit configuration
is not allowed to include dashes.

All consumers of `lib.name` already use `Target.crate_name()` rather
than `Target.name()` if a sanitized name is required. Therefore, they
already replace dashes with underscores.

Last but not least, the current documentation is simply wrong in stating
that `lib.name` defaults to the package-name with dashes replaced by
underscores. Cargo never replaces dashes for `lib.name`, and `cargo
metadata` and friends already happily show `lib.name` with dashes. Cargo
merely replaces dashes with underscores to retrieve the crate-name of a
target, which is then used as stem for library artifacts.
@dvdhrm
Copy link
Contributor Author

dvdhrm commented Sep 7, 2023

(update: dropped the test that verifies build-failure for lib.name with explicit dashes (but not implicit ones))

@hi-rustin
Copy link
Member

Related to #2775

@epage
Copy link
Contributor

epage commented Sep 26, 2023

@dvdhrm how did you discover the discrepancy?

We discussed this in the cargo team meeting and didn't really see how this discrepancy would affect people and want to better understand the use cases where people are noticing this so we can better evaluate the compatibility aspect to this (we were leaning towards making the code match the docs).

ps As this is a weightier PR, affecting policy and/or compatibility, we recommend starting something like this off as an Issue and not moving forward until it is "needs mentor" (little help) or "accepted" (mentor available). If nothing else, (1) we are much more likely to look for discussions like this among the issues and (2) its not uncommon for there to be multiple PRs for one "topic" and focusing on the issue keeps the discussion in one place, making it easier to follow.

@dvdhrm
Copy link
Contributor Author

dvdhrm commented Sep 27, 2023

@dvdhrm how did you discover the discrepancy?

We discussed this in the cargo team meeting and didn't really see how this discrepancy would affect people and want to better understand the use cases where people are noticing this so we can better evaluate the compatibility aspect to this (we were leaning towards making the code match the docs).

I am trying to deduce the artifacts a crate produces from its metadata. I was looking at cargo help metadata and trying to understand what packages[].targets[].name actually contains. And then figure out how to deduce the artifact name from it.

We currently take the value, underscorify it, and then use it as stem for the library artifact. I don't think this would break if you decide to do that in cargo, since the 'underscorify' can be safely applied multiple times.

@epage
Copy link
Contributor

epage commented Sep 27, 2023

If cargo metadata is the only way this is exposed and to make it work with anything else, you have to do a conversion that will still work if we pre-apply it, it seems like we could change this behavior and make cargo match the docs.

@epage
Copy link
Contributor

epage commented Oct 6, 2023

Closing in favor of #12783

@epage epage closed this Oct 6, 2023
bors added a commit that referenced this pull request Mar 16, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documenting-cargo-itself Area: Cargo's documentation S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants