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

Confusing error for invalid feature names #7250

Closed
Boddlnagg opened this issue Sep 3, 2016 · 13 comments
Closed

Confusing error for invalid feature names #7250

Boddlnagg opened this issue Sep 3, 2016 · 13 comments
Labels
C-bug 🐞 Category: unintended, undesired behavior

Comments

@Boddlnagg
Copy link

Boddlnagg commented Sep 3, 2016

I just tried to publish a crate containing features with names that have dots in them (see e.g. here).
On cargo publish I got the following: api errors: invalid upload request: ApplicationError("invalid crate name specified: windows.web"), and there are actually several issues:

  1. The message confusingly complains about the crate name
  2. It should complain earlier, not on publish, when you think that everything should work
  3. Is there a good reason that dots are not allowed?

By the way this also affects @retep998's 0.3 rewrite of winapi, where I originally got the idea for these features names: https://github.com/retep998/winapi-rs/blob/dev/Cargo.toml

@retep998
Copy link
Member

retep998 commented Sep 4, 2016

This issue should probably be in https://github.com/rust-lang/crates.io

@Boddlnagg
Copy link
Author

I think the right people will see it here, and at least my point (2) is directly related to Cargo itself.

@retep998
Copy link
Member

retep998 commented Sep 5, 2016

cc @alexcrichton

@retep998
Copy link
Member

retep998 commented Sep 5, 2016

It appears that the list of features in a crate on crates.io is actually a HashMap<CrateName, ...> which means the feature name is being interpreted as a crate name, hence why the error says it is an invalid crate name.

pub features: HashMap<CrateName, Vec<Feature>>,

@alexcrichton
Copy link
Member

Yes the actual change that needs to be made here is on crates.io, which is where this error message originates from.

@Boddlnagg
Copy link
Author

Do I understand this correctly that there is no hope that more characters will be allowed in feature names at some point?

@alexcrichton
Copy link
Member

@Boddlnagg currently we have no plans to allow more characters, but that doesn't necessarily mean we never will!

@dwijnand
Copy link
Member

@Boddlnagg Given the fix needs to happen in crates.io, is there a ticket tracking this in crates.io?

@Boddlnagg
Copy link
Author

@dwijnand I did not open a ticket for crates.io, so probably the answer is no

@epage
Copy link

epage commented Oct 6, 2023

Based on this discussion, I'm moving this over to crates.io.

At this point, cargo can't directly add new validation to feature names. We could offer a transition on an edition but then we'd need a way to refer to the old feature names, much like Rust did with keywords.

@epage epage transferred this issue from rust-lang/cargo Oct 6, 2023
@Turbo87
Copy link
Member

Turbo87 commented Oct 8, 2023

interesting, I just read through https://doc.rust-lang.org/cargo/reference/features.html#the-features-section and I wasn't aware that we had restrictions on feature names that only apply to the crates.io side. I was under the impression that the same restrictions existed on the cargo side too.

I'm open to lifting some of these restrictions, but we will first need to find out why they exist in the first place so that we don't accidentally break something. if anyone feels like working on this, you are very welcome to help figure this out. git blame will likely give a few hints :)

RE the error message: this had been fixed quite some time ago already. it now warns about the feature name not being valid. though I guess we could give some more context with a link to the docs or something like that.

I agree with @epage that clientside validation by cargo could be tricky with the different format restrictions.

@Rustin170506
Copy link
Member

I went through the git history, I think we don't have a special reason to refuse . in the feature name.

  1. We introduced the basic check in this PR. Called name_to_slug 45131f3
  2. We renamed it to valid_name. 89c4079
  3. We added support for / in the feature name. d64528f
  4. We allowed feature names to begin with numbers. 4bf9efa
  5. We added support for + in the feature name. 5f842f7

So I think there is no reason to refuse . in the feature name. I will try to add and test it.

@Rustin170506
Copy link
Member

Fixed by #7500

@Rustin170506 Rustin170506 added the C-bug 🐞 Category: unintended, undesired behavior label Nov 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug 🐞 Category: unintended, undesired behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants