Uh oh!
There was an error while loading. Please reload this page.
x.py: allow configuring the build directory - #71994
Conversation
rust-highfive
commented
May 7, 2020
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 |
jyn514
commented
May 7, 2020
Hmm, it looks like the repository is read-only on CI. Could we make the build directory configurable so that it defaults to |
Out of source builds worked before, they just regress regularly due to not being tested. |
Mark-Simulacrum
commented
May 7, 2020
I would prefer not to change the default here but would not object to exposing an option to choose the build dir. I'm not sure that a change as minimal as this PR suggests will be able to do it, though. |
jyn514
commented
May 7, 2020
What's the use case for not changing it? Right now all it does is rebuild the entire compiler when you change directories.
I pushed a change that allows configuring the build directory, I'm not sure how to pass that to CI though. I guess that if we're not changing the default it won't matter. |
If we changed the default, we'd have out-of-source builds always be tested on CI, and the normal use case of not passing a flag would be tested by every developer working on rustc. |
rust-highfive
commented
May 7, 2020
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 |
It looks like changing this on CI is non-trivial since it imports the |
Mark-Simulacrum
commented
May 7, 2020
One easy answer is "don't change directories", and it can also be seen as a feature. But OTOH it's not what cargo does I believe and maybe we should match that. I'm not sure. The main argument against doing the default change is it's somewhat likely it'll break people's workflows -- if we have an option exposed that says "I want the old behavior" (possibly via configure just creates config.toml so we should be able to mv that if needed? |
Hmm so the idea is instead of making it an argument it's set in config.toml? I don't think config.toml is parsed until the Rust code, does CI run before or after
Awesome, yeah my change would allow this with |
jyn514
commented
May 7, 2020
Ok I think I have an idea what's going on here ...
So the way I would make this configurable is, instead of adding a flag to x.py, reading a section from |
Mark-Simulacrum
commented
May 7, 2020
CI runs, well, I'm not sure I follow you -- bootstrap.rs is built on CI, so "both"? Okay if I believe what you suggested makes sense but I didn't think too much about it. |
My question was unclear, I meant to ask whether |
jyn514
commented
May 7, 2020
Ok, so now what happens is |
jyn514
commented
May 7, 2020
I'd still like to change the default just because I think it will be what new developers will expect but I understand that could break workflows. Maybe there's a way to ask for feedback on this somehow? It seems a shame to always be locked into the current defaults. |
rust-highfive
commented
May 7, 2020
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 |
rust-highfive
commented
May 7, 2020
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 |
jyn514
commented
May 7, 2020
Hmm, the failure message looks like it tried to run a Also, while debugging that I noticed that there is already an |
Uh oh!
There was an error while loading. Please reload this page.
Mark-Simulacrum
commented
May 8, 2020
It's pretty surprising to me that this didn't require any changes to the Rust code in bootstrap -- can you at least briefly explain why? |
jyn514
commented
May 8, 2020
As best I understand, it's because the Rust code gets the build directory from bootstrap.py, and that was the case even before this PR. |
jyn514
commented
May 8, 2020
If you like I can try to run a full |
Mark-Simulacrum
commented
May 8, 2020
No, that's sufficient, I had forgotten about our custom logic there. Could you add a comment to that effect on the field added to the Build struct? |
jyn514
commented
May 8, 2020
Sure, I added a comment. |
Mark-Simulacrum
commented
May 9, 2020
Please update the PR description to reflect the current implementation and squash the commits into one; once that's done I think we should be able to land this. |
ollie27
commented
May 9, 2020
If the old default defined at rust/src/bootstrap/bootstrap.py Line 335 in 7ebd87a |
This allows configuring the directory for build artifacts, instead of having it always be ./build. This means you can set it to a constant location, letting you reuse the same cache while working in several different directories. The configuration lives in config.toml under build.build-dir. By default, it keeps the existing default of ./build, but it can be configured to any relative or absolute path. Additionally, it allows making outputs relative to the root of the git repository using $ROOT.
jyn514
commented
May 9, 2020
Squashed the commits and removed the unused default |
Mark-Simulacrum
commented
May 9, 2020
Thanks! @bors r+ |
bors
commented
May 9, 2020
📌 Commit df36ec0 has been approved by |
Dylan-DPC-zz
commented
May 9, 2020
@bors p=1 |
bors
commented
May 9, 2020
bors
commented
May 9, 2020
☀️ Test successful - checks-actions, checks-azure |
This allows configuring the directory for build artifacts, instead of having it always be
./build. This means you can set it to a constant location, letting you reuse the same cache while working in several different directories.The configuration lives in
config.tomlunderbuild.build-dir. By default, it keeps the existing default of./build, but it can be configured to any relative or absolute path. Additionally, it allows making outputs relative to the root of the git repository using$ROOT.r? @Mark-Simulacrum