-
Notifications
You must be signed in to change notification settings - Fork 13k
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
rustc: rename -Z emit-directives to -Z emit-artifact-notifications and simplify the output. #60464
Conversation
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
"compiler-artifact" is also used by cargo, so it's an ok name. |
I don't personally have a preference one way or another on this, so long as Cargo gets the notification for metadata seems reasonable to me! |
4107447
to
098f96a
Compare
This comment has been minimized.
This comment has been minimized.
098f96a
to
fa9e8c8
Compare
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.
Looks reasonable to me! There's some stray changes I don't know why are included though
&self.testpaths.file, | ||
TargetLocation::ThisFile(self.make_exe_name()), | ||
); | ||
// Only use `make_exe_name` when the test ends up being executed. |
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.
Where did these changes come from and/or how are they related to the PR's title?
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.
See the PR description - they remove the need for normalization, by fixing the compiletest
bugs that affected #60006. IMO, the resulting tests are more clearly correct.
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.
Ah ok, sorry for missing that!
@bors: r+ |
📌 Commit fa9e8c829fdcfb08085a60e2d34367414b50a103 has been approved by |
⌛ Testing commit fa9e8c829fdcfb08085a60e2d34367414b50a103 with merge c940d25e110a9aabec4917bb262f1a6bc4ef9ed1... |
💔 Test failed - status-appveyor |
cc @davidtwco (#53196) looks like |
@eddyb I'll kick off a build of this on Windows and see if I can figure out what it's doing wrong. I probably won't get to that until tomorrow though. From a quick glance, I notice that the rust/src/tools/compiletest/src/runtest.rs Line 3169 in a19cf18
Above that, the rust/src/tools/compiletest/src/runtest.rs Line 3160 in a19cf18
I wonder if this would be as simple as changing the |
It took me multiple goes to get the normalization right due to various platform-specific differences. I suspect you might get to go through the same experience I had. I suspect you'll have a wasm-related failure at some point. |
@nnethercote I fixed the underlying issue ( |
Got distracted looking at the history of UI test normalization, I'm thinking 0cfc173 can be partially or fully reverted if the problems in question were caused by this bug. Anyway, in this case, the failure makes a lot of sense, because |
fa9e8c8
to
c89a131
Compare
@bors r=alexcrichton |
📌 Commit c89a131 has been approved by |
…ichton rustc: rename -Z emit-directives to -Z emit-artifact-notifications and simplify the output. This is my take on #60006 / #60419 (see #60006 (comment)). I'm not too attached the "notifications" part, it's pretty much bikeshed material. **EDIT**: for "artifact", @matklad pointed out Cargo already uses it (in #60464 (comment)) The first two commits are fixes that could be landed independently, especially the `compiletest` one, which removes the need for any of the normalization added in #60006 to land the test. The last commit enables the emission for all outputs, which was my main suggestion for #60006, mostly to show that it's minimal and not really a "scope creep" (as suggested in #60006 (comment)). cc @alexcrichton @nnethercote
☀️ Test successful - checks-travis, status-appveyor |
This is my take on #60006 / #60419 (see #60006 (comment)).
I'm not too attached the "notifications" part, it's pretty much bikeshed material.
EDIT: for "artifact", @matklad pointed out Cargo already uses it (in #60464 (comment))
The first two commits are fixes that could be landed independently, especially the
compiletest
one, which removes the need for any of the normalization added in #60006 to land the test.The last commit enables the emission for all outputs, which was my main suggestion for #60006, mostly to show that it's minimal and not really a "scope creep" (as suggested in #60006 (comment)).
cc @alexcrichton @nnethercote