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

Update non-ASCII crate name warning message #11017

Merged
merged 1 commit into from
Aug 23, 2022

Conversation

BlackHoleFox
Copy link
Contributor

@BlackHoleFox BlackHoleFox commented Aug 23, 2022

This PR fixes an outdated warning when initializing crates sometimes.

What does this PR try to resolve?

Per a Zulip convo on the topic, non-ASCII crate names are no longer allowed on any toolchain since rust-lang/rust#73305, during the non_ascii_idents feature's development. Cargo however tells the user that they are accepted on Nightly. Rust and Cargo should agree on this point to avoid future confusion.

How should we test and review this PR?

This should be covered by the existing test that was changed but if desired its easy to test with a checkout:

Running `/Users/fox/x/Forks/cargo/target/release/cargo init 'ああああ'`
warning: the name `ああああ` contains non-ASCII characters
Non-ASCII crate names are not supported by Rust.

@rust-highfive
Copy link

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

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 23, 2022
Copy link
Member

@Muscraft Muscraft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

@@ -253,8 +253,7 @@ fn check_name(
if restricted_names::is_non_ascii_name(name) {
shell.warn(format!(
"the name `{}` contains non-ASCII characters\n\
Support for non-ASCII crate names is experimental and only valid \
on the nightly toolchain.",
Non-ASCII crate names are not supported by Rust.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should say cargo not Rust since it is emitted by cargo

Suggested change
Non-ASCII crate names are not supported by Rust.",
Non-ASCII crate names are not supported by cargo.",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had also considered that but currently Cargo does support non-ASCII crates without much issue. Its just rustc that doesn't. For example, this works fine with cargo check:

[dependencies]
"ああああ" = { path = "../ああああ" }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure whether it's too much, but I might put something like "it will become a hard error in a future release" if we are trying to scare away folks using non-ASCII crate name 😆.

Either way, they will evenutually figure out it won't work when using as an extern crate (dependency).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I wouldn't mind if Cargo still supported it so that you could make binaries with non-ASCII names, regardless of how well everything else on the system handles it :D

@@ -253,8 +253,7 @@ fn check_name(
if restricted_names::is_non_ascii_name(name) {
shell.warn(format!(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if this should be a warning or an error. To me, it seems like it should be an error but one of the cargo team members may have a different idea.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such a package rename still works. I feel like changing it to hard error is a bit rush at this moment.

[dependencies]
hello = { path = "./你好", package = "你好" }

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to let this PR stuck in the queue too long, so I am going to merge it. Thanks for everyone in the discussion!

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Aug 23, 2022

📌 Commit b676088 has been approved by weihanglo

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 23, 2022
@bors
Copy link
Contributor

bors commented Aug 23, 2022

⌛ Testing commit b676088 with merge 6da7267...

@bors
Copy link
Contributor

bors commented Aug 23, 2022

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 6da7267 to master...

@bors bors merged commit 6da7267 into rust-lang:master Aug 23, 2022
@BlackHoleFox BlackHoleFox deleted the non-ascii-names branch August 23, 2022 23:13
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 24, 2022
Update cargo

7 commits in 9809f8ff33c2b998919fd0432c626f0f7323697a..6da726708a4406f31f996d813790818dce837161
2022-08-16 22:10:06 +0000 to 2022-08-23 21:39:56 +0000
- Update non-ASCII crate name warning message (rust-lang/cargo#11017)
- Add more tests for aggressive or precise update (rust-lang/cargo#11011)
- Ignore broken but excluded file during traversing (rust-lang/cargo#11008)
- Improve error message for wrong target names (rust-lang/cargo#10999)
- Bump snapbox to 0.3 (rust-lang/cargo#11005)
- remove missed reference to workspace inheritance in unstable.md (rust-lang/cargo#11001)
- Warning when precise or aggressive without -p flag (rust-lang/cargo#10988)
@ehuss ehuss added this to the 1.65.0 milestone Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants