When cross-compiling, also build target-arch tarballs for libstd. (Closes: #42320) #45322
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
kennytm
added
the
S-waiting-on-review
|
r? @alexcrichton -- I know nothing of this code. |
|
Updated the commit, previous one was based on faulty testing not cleaning out all build products. This one is also "more obvious" in what it does. However it will (harmlessly) try to install both native and foreign architectures twice, which I haven't figured out how to avoid: What I have in config.toml is: |
src/bootstrap/install.rs
Outdated
| compiler: builder.compiler(self.stage, self.host), | ||
| target: self.target | ||
| }); | ||
| for target in &builder.build.targets { |
There was a problem hiding this comment.
This feels wrong to me. In general, we shouldn't have to think about targets within build steps, instead the step should be configured to run for all targets. Perhaps the actual change should be on the previous line, where only_hosts: true is currently -- maybe that should be false? I'm not too certain what we're trying to accomplish here, either, but mostly am happy to take changes as long as @alexcrichton is happy.
There was a problem hiding this comment.
That may be better, however I was not sure of the exact meaning of that flag. Let me try to explain this commit: I was only mirroring the already-existing logic in install_std further above, which explicitly calls install_sh over all targets. Because it does this, I thought that it should also explicitly call ensure(dist::Std ...) explicitly over all targets.
You are right that it would be neater to do this declaratively though, to declare "depends on all architectures" on a meta-level outside of the step definitions. This would also allow (and maybe require) install_std to not loop over all target arches.
If I understand correctly, the overall goal of "x.py install" is to install all targets' libstd into DESTDIR. Now there is a further issue that actually rustbuild supports multiple hosts, so in fact this process is run for all hosts, installing into the same DESTDIR, which might explain the 2x2 issue I described in my other post. In more detail:
From the documentation of config.toml.example, it seems that rust turns my config
build = "x86_64-unknown-linux-gnu"
host = ["powerpc64le-unknown-linux-gnu"]
target = ["powerpc64le-unknown-linux-gnu"]
effectively into
build = "x86_64-unknown-linux-gnu"
host = [build + "powerpc64le-unknown-linux-gnu"]
target = host + ["powerpc64le-unknown-linux-gnu"]
and this is what gets run in the "install" process. (Hopefully someone can confirm; I'm just guessing here based on what the documentation says.) So "install" gets run for all hosts, and each of these installs all targets' libstd, at least after my patch is applied to fix the errors.
Obviously this is not ideal (multiple /usr/bin/rustc gets installed into the same location, etc), but this currently does not affect me negatively, so I'm prepared to defer the solution to this for another day - since the $build architecture install process gets run first, the foreign architecture (what I'm actually interested in) ends up clobbering all the previous files, and I end up with what I actually want. I'm also not interested in the $build-architecture libstd, but that gets installed into a architecture-specific path so I can just ignore it.
There was a problem hiding this comment.
That all seems correct to me -- I'm happy to take this for now, I think, since it seems to be following the pre-existing logic we have in the install step anyway. Going to leave to @alexcrichton's final approval, but sounds good to me.
infinity0
force-pushed
the
master
branch
2 times, most recently
from
d5d50ad to
6642f76
Compare
|
Thanks for the PR @infinity0! I agree with @Mark-Simulacrum that this may not be precisely the best solution here, but I think you've got a good idea with calling For installing and multiple configured hosts, I don't think we've basically ever had a way to get that to work. If you've got suggestions of how to support it, they'd be most welcome! Otherwise for now you'll need to run |
|
@alexcrichton is there something that needs to be done on this PR before you'll r+? |
|
I forgot to mention, I'd be happy adding something to the I'm not sure how it's "supposed" to work for cross-builds however. So if what I'm doing only works "by accident" then it would be good either to make it formal or fix it and the docs up properly. |
|
@infinity0 what do you think about calling Also I'd imagine that if you want to install a cross-compiler you'd probably want to pass |
|
Sorry, I did not quite understand that comment. Do you mean I should combine the two loops - the one I added plus the existing one inside |
|
Sorry yeah what I mean is that we have a suite of installation functions, and they should all |
shepmaster
added
S-waiting-on-author
|
I've made those changes, please review! |
This fixes cross-compile installation. Half of the logic is actually in there already in install.rs:install_std but it fails with an error like: sh: 0: Can't open /<<BUILDDIR>>/rustc-1.21.0+dfsg1/build/tmp/dist/rust-std-1.21.0-powerpc64le-unknown-linux-gnu/install.sh because the target-arch dist tarball wasn't built as well.
|
@bors: r+ Thanks! |
|
📌 Commit 32cf6e6 has been approved by |
When cross-compiling, also build target-arch tarballs for libstd. (Closes: #42320) Half of the logic is actually in there already in install.rs:install_std but it fails with an error like: sh: 0: Can't open /<<BUILDDIR>>/rustc-1.21.0+dfsg1/build/tmp/dist/rust-std-1.21.0-powerpc64le-unknown-linux-gnu/install.sh because the target-arch dist tarball wasn't built as well. This commit fixes that so the overall install works. There is one minor bug in the existing code which this commit doesn't fix - the install.log from multiple runs of the installer gets clobbered, which seems like it might interfere with the uninstall process (I didn't look very deeply into this, because it doesn't affect what I need to do.) The actual installed files under DESTDIR seem fine though - either they are installed under an arch-specific path, or the multiple runs will clobber the same path with the same arch-independent file.
|
☀️ Test successful - status-appveyor, status-travis |
Half of the logic is actually in there already in install.rs:install_std but it fails with an error like:
sh: 0: Can't open /<>/rustc-1.21.0+dfsg1/build/tmp/dist/rust-std-1.21.0-powerpc64le-unknown-linux-gnu/install.sh
because the target-arch dist tarball wasn't built as well. This commit fixes that so the overall install works.
There is one minor bug in the existing code which this commit doesn't fix - the install.log from multiple runs of the installer gets clobbered, which seems like it might interfere with the uninstall process (I didn't look very deeply into this, because it doesn't affect what I need to do.) The actual installed files under DESTDIR seem fine though - either they are installed under an arch-specific path, or the multiple runs will clobber the same path with the same arch-independent file.