Skip to content
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

rustbuild: Add a way to build all tools as part of 'x.py build' #42979

Closed
wants to merge 2 commits into from

Conversation

Keruspe
Copy link
Contributor

@Keruspe Keruspe commented Jun 30, 2017

With these two patches and the feature turned on, running ./x.py install after ./x.py build doesn't need to compile anything. Only the doc/dist/install steps are actually doing something.

This does not change what we install (as opposed to extended).
This only causes all the tools to be built as part of './x.py build'.
This is useful to check that all the tools compile fine, and will avoid
some compilation during './x.py install' if it is enabled.

Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

Some components are only built when they're being pulled in
as dependencies with .stage(0) specified.
They now can also be pulled in with build-all-tools, hence
only build them as stage 0.

Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
@Keruspe
Copy link
Contributor Author

Keruspe commented Jun 30, 2017

Fixed the tidy issue.

@shepmaster shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 30, 2017
@alexcrichton
Copy link
Member

Hm sorry I think I've lost context in the meantime by why is it again that ./x.py dist isn't run before ./x.py install? Otherwise I find it odd that it's undesirable to build tools during ./x.py install but it's ok to run xz/gz compression to create and unpack tarballs during ./x.py install

@aidanhs aidanhs added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 5, 2017
@Keruspe
Copy link
Contributor Author

Keruspe commented Jul 6, 2017

Well, running ./x.py dist then ./x.py installwould do the tarballs generation twice.
I think rebuilding the tarballs each time the dist-* steps are called is the right thing to do as generating them "only when needed" would probably be quite complex and would lead to problems sooner or later.
As for building during the install phase, it's no big deal when developing, but just taking as a sample the list of packages I maintain for Exherbo, or even the list of packages that are installed on my system, rust is the only one building stuff during the install phase.
Building everything in the build phase just fits better the way packaging works in source-based distributions (and matches better what other software usually do) and can also help catching more bugs more quickly.
It's still odd indeed to have to generate the tarballs in the install phase (even if we were to run dist first), but it sort of makes sense due to how this part works in rustbuild. On the other hand, avoiding the compilation in the install part is way easier that avoiding the tarball generation, and it's already a great improvement.

I tried to keep the diff minimal. I had to add the "only-stage" thing because otherwise the tools in stage2 would be pulled in by ./x.py build and the the tools in stage0 would be pulled in as dependencies of ./x.py install. As those tools are only supposed to be built as stage0 anyways, this just states it clearly in the step definition.

I hope that helps clarify things, and doesn't make it even more confuse.

@alexcrichton
Copy link
Member

Hm ok. I'm wary of this because of how much default behavior it hardcodes into the step rules which seems very brittle and likely to not work into the future. It seems to me like the real solution here is to get ./x.py dist followed by ./x.py install to not rebuild tarballs twice? Presumably we can have very simple heuristics like "check all files in the source tree to see if any are newer" or something like that to prevent double-creation of tarballs?

In terms of future-proofing the logic that seems to me like the better option to solving this than hardcoding all of these tools at stage0?

@Keruspe
Copy link
Contributor Author

Keruspe commented Jul 6, 2017

That would be a solution too indeed, but wouldn't that be a little "overkill"?
What about an option to only run the install-* steps without their dependencies?
Then ./x.py dist would run all the dist-* steps and their dependencies, and ./x.py install --no-dependencies would run the install-* steps without the dist-* steps it depends on (which already ran)

@alexcrichton
Copy link
Member

I think it'd make more sense to just add dependency tracking to the dist steps. I don't think it'd be very hard to make something naive and coarse

@alexcrichton
Copy link
Member

ping @Keruspe, just wanted to make sure this doesn't fall off your radar!

@Keruspe
Copy link
Contributor Author

Keruspe commented Jul 14, 2017

Yeah sure, just haven't had time to look into it lately as I'm currently moving, but will try to make that happen early in the next cycle, for 1.21

@alexcrichton
Copy link
Member

ping @Keruspe, just another ping to make sure this isn't forgotten!

@bors
Copy link
Contributor

bors commented Jul 22, 2017

☔ The latest upstream changes (presumably #43059) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

Ok I'm going to close this out of inactivity to help clear up the queue, but plese feel free to resubmit with a rebase!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants