-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
compiletest: Make aux-crate directive explicitly handle --extern modifiers
#151353
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
Conversation
|
Some changes occurred in src/tools/compiletest cc @jieyouxu |
|
Ah, so we're talking about these?
“Options” seems like a needlessly confusing term to have been used on that unstable-book page; calling them “modifiers” (as the tracking issue does) is a lot clearer. |
| /// With `aux-crate: noprelude:foo=bar.rs` this will be `noprelude`. | ||
| pub extern_opts: Option<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I think we should ignore the unstable book and the compiler source, and call these “extern modifiers” instead.
- This field comment really needs to give a brief explanation of what extern modifiers actually are, perhaps with a link to the tracking issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree "extern modifiers" is a better name than "extern options". I added some more info and linked to the tracking issue. I don't think we should give too many details though since we don't want that comment to become outdated soon.
I also added references this functionality in the rustc-dev-guide.
| let opts_and_name = parts.next().expect("missing aux-crate name (e.g. log=log.rs)").to_string(); | ||
| let path = parts.next().expect("missing aux-crate value (e.g. log=log.rs)").to_string(); | ||
| let (opts, name) = match opts_and_name.split_once(':') { | ||
| None => (None, opts_and_name), | ||
| Some((opts, name)) => (Some(opts.to_string()), name.to_string()), | ||
| }; | ||
| AuxCrate { extern_opts: opts, name, path } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This parsing step is getting complex enough that a regex might be an improvement. Seems worth a try at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried porting it to a regex but didn't think it ended up being easier to understand, so I kept it as is. I hope that's OK. The diff is small at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent some more time on it and found a way to use a regex that I like. Hopefully you'll like it too.
|
We should also mention |
|
The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes. |
5028acf to
41c6301
Compare
aux-crate directive explicitly handle --extern optionsaux-crate directive explicitly handle --extern modifiers
41c6301 to
8d3e25a
Compare
| // Matches: | ||
| // name=path | ||
| // modifiers:name=path | ||
| let caps = static_regex!(r"^(?:(?P<modifiers>[^=]*?):)?(?P<name>[^=]*)=(?P<path>.*)$") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I believe ?P<name> is just a longer form of ?<name>, and in that case we might as well use the shorter syntax without P.
(I notice that compiletest currently uses an inconsistent mixture of both styles.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I wondered if I should have P or not and noticed it was used in other places, so I kept it. I don't mind removing it though. Done.
| // modifiers:name=path | ||
| let caps = static_regex!(r"^(?:(?P<modifiers>[^=]*?):)?(?P<name>[^=]*)=(?P<path>.*)$") | ||
| .captures(r.trim()) | ||
| .unwrap_or_else(|| panic!("missing aux-crate value (e.g. log=log.rs)")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, this error is shown if the value couldn't be parsed, so it should probably say “couldn't parse aux-crate value” and maybe include the value string, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intentionally didn't want to make this PR change behavior or error message, but I agree it is nicer to print what value that couldn't be parse, so I added that.
| let name = caps.name("name").map(|m| m.as_str().to_string()).unwrap_or_default(); | ||
| let path = caps.name("path").map(|m| m.as_str().to_string()).unwrap_or_default(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the regex match succeeded, then name and path should always be present, so these can probably just be caps["name"].to_owned() or similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I don't think there is something like to_owned()? .name("name") returns an Option<Match>. If there is a simpler way to write this I wasn't able to figure it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regex::Captures implements Index<&str>, so instead of writing caps.get("name") it should be possible to write caps["name"] to get a &str (panicking if that named group didn't match), and then convert that to String.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh... Thanks. Done.
|
Could you please also add at least one unit test for (Doesn't need to be super thorough; I just want some explicit assurance that the modifiers don't accidentally end up in the name after all.) |
|
Should be good to go after this last round of nitpicks. @rustbot author |
8d3e25a to
be2b322
Compare
Sure, no problem. Done. Note however that you can be confident that parsing works even without the new tests, because if e.g. modifiers failed to parse properly, a lot of tests that depend on them would fail (like @rustbot ready |
be2b322 to
fde37c3
Compare
|
Thanks. @bors r+ |
compiletest: Make `aux-crate` directive explicitly handle `--extern` modifiers With `-Zunstable-options` it is possible to pass options to `--extern`. See here for an exhaustive list of possible options: https://github.com/rust-lang/rust/blob/b5dd72d2921500c9d9e15f074e1d831adcaa3dee/compiler/rustc_session/src/config.rs#L2356-L2367 Using these options works with the `aux-crate` directive, but only because the options pretend to be part of the name. Make it clearer what `aux-crate` supports by explicitly handling `--extern` options. This PR is step one of splitting up rust-lang#151258 into smaller pieces. r? @Zalathar
|
📋 Only unclosed PRs can be unapproved. |
|
(Whoops did not mean to close this) |
compiletest: Make `aux-crate` directive explicitly handle `--extern` modifiers With `-Zunstable-options` it is possible to pass options to `--extern`. See here for an exhaustive list of possible options: https://github.com/rust-lang/rust/blob/b5dd72d2921500c9d9e15f074e1d831adcaa3dee/compiler/rustc_session/src/config.rs#L2356-L2367 Using these options works with the `aux-crate` directive, but only because the options pretend to be part of the name. Make it clearer what `aux-crate` supports by explicitly handling `--extern` options. This PR is step one of splitting up rust-lang#151258 into smaller pieces. r? @Zalathar
…modifiers To make it clearer what happens. In other words, do not silently keep modifiers as part of `AuxCrate::name`.
fde37c3 to
6f767b6
Compare
|
|
|
Trivial fix. @bors r=Zalathar |
compiletest: Make `aux-crate` directive explicitly handle `--extern` modifiers With `-Zunstable-options` it is possible to pass options to `--extern`. See here for an exhaustive list of possible options: https://github.com/rust-lang/rust/blob/b5dd72d2921500c9d9e15f074e1d831adcaa3dee/compiler/rustc_session/src/config.rs#L2356-L2367 Using these options works with the `aux-crate` directive, but only because the options pretend to be part of the name. Make it clearer what `aux-crate` supports by explicitly handling `--extern` options. This PR is step one of splitting up rust-lang#151258 into smaller pieces. r? @Zalathar
Rollup of 11 pull requests Successful merges: - #149962 (Promote powerpc64-unknown-linux-musl to tier 2 with host tools) - #150138 (Add new Tier 3 targets for ARMv6) - #150905 (Fix(lib/win/net): Remove hostname support under Win7) - #151094 (remote-test-server: Fix compilation on UEFI targets) - #151346 (add `simd_splat` intrinsic) - #151353 (compiletest: Make `aux-crate` directive explicitly handle `--extern` modifiers) - #151538 (std: `sleep_until` on Motor and VEX) - #151098 (Add Korean translation to Rust By Example) - #151157 (Extend build-manifest local test guide) - #151403 (std: use 64-bit `clock_nanosleep` on GNU/Linux if available) - #151571 (Fix cstring-merging test for Hexagon target)
Rollup merge of #151353 - Enselic:aux-crate-opts, r=Zalathar compiletest: Make `aux-crate` directive explicitly handle `--extern` modifiers With `-Zunstable-options` it is possible to pass options to `--extern`. See here for an exhaustive list of possible options: https://github.com/rust-lang/rust/blob/b5dd72d2921500c9d9e15f074e1d831adcaa3dee/compiler/rustc_session/src/config.rs#L2356-L2367 Using these options works with the `aux-crate` directive, but only because the options pretend to be part of the name. Make it clearer what `aux-crate` supports by explicitly handling `--extern` options. This PR is step one of splitting up #151258 into smaller pieces. r? @Zalathar
Rollup of 11 pull requests Successful merges: - rust-lang/rust#149962 (Promote powerpc64-unknown-linux-musl to tier 2 with host tools) - rust-lang/rust#150138 (Add new Tier 3 targets for ARMv6) - rust-lang/rust#150905 (Fix(lib/win/net): Remove hostname support under Win7) - rust-lang/rust#151094 (remote-test-server: Fix compilation on UEFI targets) - rust-lang/rust#151346 (add `simd_splat` intrinsic) - rust-lang/rust#151353 (compiletest: Make `aux-crate` directive explicitly handle `--extern` modifiers) - rust-lang/rust#151538 (std: `sleep_until` on Motor and VEX) - rust-lang/rust#151098 (Add Korean translation to Rust By Example) - rust-lang/rust#151157 (Extend build-manifest local test guide) - rust-lang/rust#151403 (std: use 64-bit `clock_nanosleep` on GNU/Linux if available) - rust-lang/rust#151571 (Fix cstring-merging test for Hexagon target)
With
-Zunstable-optionsit is possible to pass options to--extern. See here for an exhaustive list of possible options:rust/compiler/rustc_session/src/config.rs
Lines 2356 to 2367 in b5dd72d
Using these options works with the
aux-cratedirective, but only because the options pretend to be part of the name. Make it clearer whataux-cratesupports by explicitly handling--externoptions.This PR is step one of splitting up #151258 into smaller pieces.
r? @Zalathar