fix(embedded): Align package name sanitization with cargo-new #12255
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @weihanglo (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
Command-new
S-waiting-on-review
epage
force-pushed
the
sanitize
branch
2 times, most recently
from
697edc0
to
570c5df
Compare
Merged
rustbot
added
A-cli
bors
added a commit
that referenced
this pull request
Jun 13, 2023
refactor(embedded) ### What does this PR try to resolve? This is trying to make it easier to have multiple active PRs touching `embedded.rs` by separating out the parts of the code that will be touched in each PR. This is done by making the code roughly follow how the future code will be structured. e.g. see #12255 ### How should we test and review this PR? Commit at a time. This is just a refactor, so no tests are changed.
|
☔ The latest upstream changes (presumably #12269) made this pull request unmergeable. Please resolve the merge conflicts. |
weihanglo
reviewed
Jun 16, 2023
BTW, I guess you meant |
This was originally split out because before rust-lang#12269, it was needed elsewhere.
33 tasks
weihanglo
changed the title
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
1 similar comment
|
☀️ Test successful - checks-actions |
|
👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request. |
bors
added a commit
that referenced
this pull request
Jun 17, 2023
fix(embedded): Don't create an intermediate manifest ### What does this PR try to resolve? More immediately, this is to unblock rust-lang/rust#112601 More generally, this gets us away from hackily writing out an out-of-line manifest from an embedded manifest. To parse the manifest, we have to write it out so our regular manifest loading code could handle it. This updates the manifest parsing code to handle it. This doesn't mean this will work everywhere in all cases though. For example, ephemeral workspaces parses a manifest from the SourceId and these won't have valid SourceIds. As a consequence, `Cargo.lock` and `CARGO_TARGET_DIR` are changing from being next to the temp manifest to being next to the script. This still isn't the desired behavior but stepping stones. ### How should we test and review this PR? A Commit at a time ### Additional information In production code, this does not conflict with #12255 (due to #12262) but in test code, it does.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 20, 2023
Update cargo 12 commits in 0c14026aa84ee2ec4c67460c0a18abc8519ca6b2..dead4b8740c4b6a8ed5211e37c99cf81d01c3b1c 2023-06-14 18:43:05 +0000 to 2023-06-20 20:07:17 +0000 - Convert valid feature name warning to an error. (rust-lang/cargo#12291) - fix(embedded): Don't pollute script dir with lockfile (rust-lang/cargo#12284) - fix: remove `-Zjobserver-per-rustc` again (rust-lang/cargo#12285) - docs: some tweaks around `cargo test` (rust-lang/cargo#12288) - Enable `doctest-in-workspace` by default (rust-lang/cargo#12221) - fix(embedded): Don't auto-discover build.rs files (rust-lang/cargo#12283) - fix(embeded): Don't pollute the scripts dir with `target/` (rust-lang/cargo#12282) - feat: prepare for the next lockfile bump (rust-lang/cargo#12279) - fix(embedded): Don't create an intermediate manifest (rust-lang/cargo#12268) - refactor(embedded): Switch to `syn` for parsing doc comments (rust-lang/cargo#12258) - fix(embedded): Align package name sanitization with cargo-new (rust-lang/cargo#12255) - Clarify the default behavior of cargo-install. (rust-lang/cargo#12276) r? `@ghost`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR try to resolve?
This is a follow up to #12245 which is working to resolve the tracking issue #12207
This first aligns sanitization of package names with the central package name validation logic, putting the code next to each other so they can more easily stay in sync.
Oddly enough, cargo-new is stricter than our normal package-name validation. I went ahead and sanitized along with that as well.
In working on this, I was bothered by
-and_in file names because of sanitization, so I made it more consistent by detecting which the user is using_in bins, so I switched the default to-How should we test and review this PR?
One existing test covers a variety of sanitization needs
Another existing test hit one of the other cases (
testbeing reserved)Additional information
For implementation convenience, I changed the directory we write the manifest to. The impact of this should be minimal since