rustbuild: Eliminate duplication of dist tarballs #38468
Conversation
xen0n
changed the title
| host.push(build.clone()); | ||
| } | ||
| host | ||
| }; |
There was a problem hiding this comment.
I think I'd prefer to omit this change to keep the invocations of the build system predictable by ensuring that --host always overrides the list of hosts. I also think it's fine if ./x.py dist --host ... doesn't work as precisely what dist packages are created when is already kinda squishy, and I'm fine just saying that the invocation for doing that is slightly different (e.g. doesn't require this workaround)
There was a problem hiding this comment.
OK, will remove the whole commit.
| if host != build.config.build { | ||
| println!("\tskipping, not a build host"); | ||
| return | ||
| } |
There was a problem hiding this comment.
Could this be added to step.rs instead actually?
There was a problem hiding this comment.
I was just copying what the other dist functions did. How do I squeeze this into step.rs, by putting into the run closure without the println?
There was a problem hiding this comment.
Ah right yeah there's some other filters in here by this point. In that case let's leave this here for a future refactoring to take care of.
| @@ -814,6 +814,9 @@ invalid rule dependency graph detected, was a rule added and maybe typo'd? | |||
| Subcommand::Clean => panic!(), | |||
| }; | |||
|
|
|||
| // Only construct our plan from build triple steps for dist builds. | |||
| let from_build_only = kind == Kind::Dist; | |||
There was a problem hiding this comment.
Could this actually get moved down to the let hosts statement? It can have a comment like:
// For 'dist' steps we only distribute artifacts built from
// the build platform, so only consider that in the hosts
// array.There was a problem hiding this comment.
(that'd avoid the need for the extra filter on the loop below)
There was a problem hiding this comment.
I originally did something like you suggested, but was faced with borrow checker rage saying the vector slice or anything like that doesn't outlive 'a up there in the fn sig. Maybe I was just not trying harder? Anyway I'll try to figure it out this time...
There was a problem hiding this comment.
Okay, I've finally figured out something that'll make the borrow checker happy. Is it acceptable to rely on the fact that config.rs explicitly puts the build triple first in the hosts array? Otherwise it seems anything that involves cloning self.build.config.build would irritate the borrow checker. (Completely moving to iterator-based tricks would enrage the type checker instead, btw.)
|
As for the testing, I used the same Docker image and command as the buildbot slaves, only reducing the number of targets. Here is the output of |
|
📌 Commit 5c69530 has been approved by |
We only want to package each host/target once for `dist`. The obvious solution takes the form of step dependency, which is implemented at least for the `dist-rustc` step. Unfortunately since the steps are created from `hosts x targets` during planning and *not* de-duplicated afterwards, the problem still persists. We therefore move the check inside `plan()` instead, to avoid creating the duplicate steps in the first place.
xen0n
force-pushed
the
tarball-wrangling
branch
from
5c69530
to
8e38b2d
Compare
|
Sorry to require another r+ but I've squashed down the commits for better git history. That r+ was quick! |
|
@bors: r+ No worries! |
|
📌 Commit 8e38b2d has been approved by |
…hton rustbuild: Eliminate duplication of dist tarballs Fixes rust-lang#38365 by not constructing the duplicate steps in the first place, as suggested. The source package step is lacking the check as in other steps, so it is added as well. Tested locally with the `alexcrichton/rust-slave-linux-cross:2016-11-11` container (with the build slave init replaced with no-op, of course). r? @alexcrichton
Rollup of 29 pull requests - Successful merges: #37761, #38006, #38131, #38150, #38158, #38171, #38208, #38215, #38236, #38245, #38289, #38302, #38315, #38346, #38388, #38395, #38398, #38418, #38432, #38451, #38463, #38468, #38470, #38471, #38472, #38478, #38486, #38493, #38498 - Failed merges: #38271, #38483
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.
`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.
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
Fixes #38365 by not constructing the duplicate steps in the first place, as suggested. The source package step is lacking the check as in other steps, so it is added as well.
Tested locally with the
alexcrichton/rust-slave-linux-cross:2016-11-11container (with the build slave init replaced with no-op, of course).r? @alexcrichton