rustbuild: Hotfix to unbreak nightly #38639
Conversation
|
Thanks for the PR! And no worries about the nightly breakage, it happens :) |
| // Keep the full projection from build triple to targets | ||
| // for the dist steps, now that the hosts array is truncated | ||
| // to avoid duplication of work. | ||
| targets |
There was a problem hiding this comment.
I think we'll still want to draw from the array of hosts here rather than use the array of targets. If rule.host is true then we don't want to try to package compilers for only-target targets. For example if you configure rustbuild with --build=A --host=B --target=C, then we should see:
- std for A, B, C
- rustc for A, B
- rust-docs for A, B, C
- etc
I think this will cause rustc to be package for A, B, and C, which'd cause failures.
I believe one way to solve this though would be to leave the logic as-is from before but shadow the outer hosts variable with the same definition above but minus the Dist logic. Does that make sense?
There was a problem hiding this comment.
I noticed this at the same time! Fixed now. 😄
|
Hold on for a minute... the desired list should be the original hosts array, not the targets. I'm re-submitting. |
xen0n
force-pushed
the
nightly-dist-hotfix
branch
from
b9a3401 to
d7662cd
Compare
| // Keep the full projection from build triple to the hosts | ||
| // for the dist steps, now that the hosts array is truncated | ||
| // to avoid duplication of work. | ||
| &self.build.config.host |
There was a problem hiding this comment.
I think we'll want to keep the same logic as below where if --target is specified without --host even the dist steps will avoid the matrix multiplication.
Basically I think we'll just want to shadow hosts but still hit the path below for checking for --target
There was a problem hiding this comment.
Oh, so the check for dist should happen inside the else clause of the --target check, right? I submitted the edit before seeing your review comment, it seems GitHub is not automatically refreshing code reviews but only regular comments.
There was a problem hiding this comment.
Why is this doing &hosts[..0] rather than &[]?
There was a problem hiding this comment.
Ah I think we can avoid the check for Dist entirely during the clause for --target. We basically just need to filter down an array of hosts without the Dist clause
There was a problem hiding this comment.
@arielb1 Historical wart I think. Going to clean up this in a side commit then.
There was a problem hiding this comment.
@alexcrichton Got it! Just replace all the hosts with self.build.config.host. I had in my brain the hosts array processing overly conservative.
The comment touched, as originally written, only concerned itself with the `test` steps. However, since rust-lang#38468 the `arr` variable actually has gained an indirect relationship with the `dist` steps too. The comment failed to convey the extra meaning, contributing to the misunderstanding which eventually lead to rust-lang#38637. Fix that by moving the comment into the right place near the relevant condition, and properly documenting `arr`'s purpose.
xen0n
force-pushed
the
nightly-dist-hotfix
branch
2 times, most recently
from
fc83132 to
ff3307f
Compare
| } else { | ||
| hosts | ||
| &self.build.config.host[..] |
There was a problem hiding this comment.
And and as the final thing I think here we'll want the same logic where if --host is specified then we use that here. For that it should mean that we just check if self.build.flags.host.len() != 0 then we use that array here (no matter) what. You could probably structure this as:
if self.build.flags.host.len() > 0 {
&self.build.flags.host[..]
} else if self.build.flags.target.len() > 0 {
&[]
} else {
&self.build.config.host[..]
}
|
Thanks for helping to fix nightlies @xen0n ! |
`arr` is the actual list of targets participating in steps construction, but due to rust-lang#38468 the hosts array now consists of only the build triple for the `dist` steps, hence all non-build-triple targets are lost for the host-only rules. Fix this by using the original non-shadowed hosts array in `arr` calculation. This should unbreak the nightly packaging process. Fixes rust-lang#38637.
xen0n
force-pushed
the
nightly-dist-hotfix
branch
from
ff3307f to
cf89453
Compare
|
Comment addressed, should be good to go. BTW sorry for missing today's nightly, my last (penultimate?) push was local time 4 a.m. and I really needed some |
|
@xen0n I can't wait for this to hit the next nightly! There are a number of breaking changes in Rust that need to be published. |
|
@bors r+ p=100 |
|
📌 Commit cf89453 has been approved by |
|
@xen0n thank you again for being so diligent with Rust maintenance. We'll get this in tonight for sure. |
rustbuild: Hotfix to unbreak nightly Fixes an oversight unnoticed in #38468 that eventually broke nightly packaging. I didn't realize this until some moments ago, when I finally found out the failure is actually deterministic. Many apologies for eating 3 nightlies during the holidays. r? @alexcrichton
|
☀️ Test successful - status-appveyor, status-travis |
Fixes an oversight unnoticed in #38468 that eventually broke nightly packaging. I didn't realize this until some moments ago, when I finally found out the failure is actually deterministic. Many apologies for eating 3 nightlies during the holidays.
r? @alexcrichton