Migrate internal diagnostic registration to macro_rules #64139
Merged
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
|
Some changes occurred in diagnostic error codes |
rust-highfive
added
the
S-waiting-on-review
Mark-Simulacrum
commented
Sep 4, 2019
src/bootstrap/builder.rs
Outdated
| @@ -848,6 +848,7 @@ impl<'a> Builder<'a> { | |||
| // things still build right, please do! | |||
| match mode { | |||
| Mode::Std => metadata.push_str("std"), | |||
| Mode::Codegen => metadata.push_str("codegen"), | |||
There was a problem hiding this comment.
@alexcrichton I was getting "found two different crates with name num_cpus that are not distinguished by differing -C metadata. This will result in symbol conflicts between the two." and a similar error for cc without this change; does it seem reasonable to just add this? I didn't change Cargo.lock at all so my assumption is that how we're linking things together changed due to differing uses of syntax or something like that.
There was a problem hiding this comment.
A better fix might actually to be to just remove the cc and num_cpus dependencies from the Cargo.toml of librustc_codegen_llvm, that way it just pulls in the sysroot crates used by the compiler already
There was a problem hiding this comment.
Hm so this is a bit painful due to build scripts being built with the beta compiler always I believe which leads us to not being able to load cc and such from the sysroot there -- this addition works in practice so I'd like to land it for now.
There was a problem hiding this comment.
I don't think that's quite right, if you're getting conflicting crates then something in the rustc_driver dependency graph already links to the cc crate, and that's what's being loaded. If you delete this addition and delete the lines in Cargo.toml the build should work just fine.
There was a problem hiding this comment.
I guess maybe we can't ever conflict with build deps since those are always built separately? I'll try that.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
matklad
reviewed
Sep 4, 2019
Mark-Simulacrum
commented
Sep 4, 2019
| ); | ||
| ($($code:tt: $description:tt),*,) => ( | ||
| $($crate::register_diagnostic! { $code, $description })* | ||
| ($($ecode:ident: $message:expr,)* ; $($code:ident,)*) => ( |
There was a problem hiding this comment.
I would love to drop the ; here to separate long and short error codes but wasn't able to quickly come up with a way to do so. It seems reasonable to me to leave further improvement to a future PR.
Mark-Simulacrum
force-pushed
the
strip-legacy-proc-macro
branch
from
0556499 to
87dbf92
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The failure seems spurious? |
|
📌 Commit 87dbf92a22ce87cfda271635ab963a656dc8c64f has been approved by |
bors
added
S-waiting-on-bors
|
Yeah, failure is almost certainly spurious. |
Mark-Simulacrum
force-pushed
the
strip-legacy-proc-macro
branch
from
87dbf92 to
eff353e
Compare
|
r? @alexcrichton on the codegen dep commit (c124683 in the current rebase, I'll try to edit if I need to rebase again) |
Mark-Simulacrum
added
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Mark-Simulacrum
force-pushed
the
strip-legacy-proc-macro
branch
from
eff353e to
d63433d
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
I don't think the dependencies necessarily need to be moved, only if they happen to already be in librustc they should be loaded there, otherwise it's fine for the codegen backend to depend on them. |
|
That's true, certainly, but this in theory nets us slightly faster compile times since the pipe is wider/deeper in librustc compilation and hopefully stops this from re-arising in the future when someone goes and adds tempfile to librustc itself. But I can move the non-used ones in librustc back. |
|
Yeah let's just keep the non rustc ones in llvm, these all have inconsequential compile times |
Mark-Simulacrum
force-pushed
the
strip-legacy-proc-macro
branch
from
d63433d to
e39f566
Compare
|
Turns out there were no non-rustc ones afaict, so removed all of them. We've seen the create-dir-all failure twice in the PR builder on this PR but there's no way this PR is responsible that I can think of so I'm going to chalk it up to spurious failures. However, also going to not roll this up. @bors r=petrochenkov rollup=never |
|
📌 Commit e39f5666317d217b2ec7815a03536f239367c370 has been approved by |
bors
removed
the
S-waiting-on-review
bors
added
the
S-waiting-on-bors
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☔ The latest upstream changes (presumably #64172) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
S-waiting-on-author
The global restriction is 100, but since error codes are printed out via --explain we want to restrict them to just 80 columns.
Not doing this leads to building two copies of e.g. num_cpus in the sysroot and _llvm deps, leading to conflicts between the two when compiling librustc_codegen_llvm. It's not entirely clear why this is the case after the changes in this PR but likely has something to do with a subtle difference in ordering or similar.
Mark-Simulacrum
force-pushed
the
strip-legacy-proc-macro
branch
from
e39f566 to
41b39fc
Compare
|
@bors r=petrochenkov I was able to reproduce the UI test failure locally in the docker container once, but not more than that (have tried ~15 times since then, though admittedly without deleting cache), so I assume it's spurious. |
|
📌 Commit 41b39fc has been approved by |
bors
added
S-waiting-on-bors
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
I'm leaning towards deleting this test. There's no way that code in this PR could be responsible for this failure I think, unless I'm missing something? @alexcrichton What do you think about that? Is it worth trying to get a more concrete reason for the failure? |
|
It seems to be happening pretty consistently here so I would say it's probably related to this. IIRC something about error indexes used |
|
Hm. I guess that makes sense then why I stopped being able to reproduce after the first attempt (something created the temp dir). I'll go ahead and add a create_dir_all on the current directory too. |
|
@bors r=petrochenkov |
|
📌 Commit 5153db1 has been approved by |
Centril
added a commit
to Centril/rust
that referenced
this pull request
Sep 7, 2019
…acro, r=petrochenkov Migrate internal diagnostic registration to macro_rules Review is best done commit-by-commit. Fixes rust-lang#64132.
bors
added a commit
that referenced
this pull request
Sep 7, 2019
Rollup of 7 pull requests Successful merges: - #64023 (libstd fuchsia fixes) - #64098 (Ensure edition lints and internal lints are enabled with deny-warnings=false) - #64139 (Migrate internal diagnostic registration to macro_rules) - #64226 (Aggregation of cosmetic changes made during work on REPL PRs: libsyntax) - #64227 (Aggregation of cosmetic changes made during work on REPL PRs: librustc) - #64235 (Upgrade env_logger to 0.6) - #64258 (compiletest: Match suffixed environments) Failed merges: 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.
Review is best done commit-by-commit.
Fixes #64132.