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

Handle git info in rustbuild differently #43771

Closed
alexcrichton opened this issue Aug 9, 2017 · 8 comments
Closed

Handle git info in rustbuild differently #43771

alexcrichton opened this issue Aug 9, 2017 · 8 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@alexcrichton
Copy link
Member

Right now we encode git information directly into the rustc binary that we build during the normal build process, and this information powers rustc -vV and such. At this time, however, this git information is loaded from the git repository every time you execute ./x.py build. Consequently, whenever a new commit is made, lots of rustc libs need to be rebuilt!

My personal workflow consists of making commits over time as I fix things (especially tests), and having everything get rebuilt every time I make a commit is a bit of a bummer! I think a better strategy here would be to cache git information from the first time of invocation of rustbuild into the out directory. That way we keep reporting the same commit (even though it may not exist or be the actual commit) when there's a persisted build directory.

I believe a strategy like this won't affect the builds on CI, but may hopefully improve the developer experience!

@alexcrichton alexcrichton added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Aug 9, 2017
@est31
Copy link
Member

est31 commented Aug 9, 2017

What about not tracking git info for the dev profile? No git commit is better than a wrong one imo.

@alexcrichton
Copy link
Member Author

@est31 oh that's an even better idea! I'm 👍 to doing that.

@Mark-Simulacrum
Copy link
Member

My pull request with fixes to rustbuild has a flag (ignore-git) that just ignores git info; making it default to true is maybe not ideal, but seems fine for the dev profile.

@alexcrichton
Copy link
Member Author

Oh right, awesome! Once that lands I'll tag this as E-easy.

@Mark-Simulacrum Mark-Simulacrum added C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Aug 10, 2017
@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Aug 13, 2017

The relevant PR landed, so tagging this as E-easy and E-mentor. I believe what needs to happen is config.ignore_git needs to be set to true if config.channel is equal to "dev", but this can only happen after the line that loads it. Should be a relatively easy change to make, but don't forget to document it in config.toml.example in the root of the tree. Let us know if you need any help with this!

alexcrichton added a commit to alexcrichton/rust that referenced this issue Aug 15, 2017
This commit automatically sets `ignore-git` to `true` in configuration if the
configured channel is set to "dev" and otherwise no value for `ignore-git` has
been specified. This'll help rebuilds locally whenever a new commit is made by
ensuring that git information never makes its way to compiler crates in the
first place.

Closes rust-lang#43771
@JeremySorensen
Copy link
Contributor

I would like to try to take this on, if that's OK. I have a question right out of the gate however; looking at these lines:

set(&mut config.channel, rust.channel.clone());
set(&mut config.ignore_git, rust.ignore_git);

In the case that config.channel is equal to dev, do we force config.ignore_git to true, or only set it to true if it is not present in the rust variable with the deserialized toml?

It seems like either way I would add code to set config.ignore_git to true if config.channel is dev, but if it is a force override I would put it after set(&mut config.ignore_git, rust.ignore_git); and if it is a default, I would just put it before that line.

@alexcrichton
Copy link
Member Author

Oh sorry about this @JeremySorensen, I think I made a PR just before you commented! That being said, @Mark-Simulacrum had some comments on that PR and it looks like what you're thinking is more along the lines of what he's thinking, so I'm gonna close that PR and let you take over if you'd like.

I believe what you're thinking is spot on though!

@JeremySorensen
Copy link
Contributor

@alexcrichton Thanks for letting me take a crack at this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

No branches or pull requests

4 participants