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

Add x.py option to skip rebuilding LLVM #65612

Closed
varkor opened this issue Oct 19, 2019 · 11 comments · Fixed by #67437
Closed

Add x.py option to skip rebuilding LLVM #65612

varkor opened this issue Oct 19, 2019 · 11 comments · Fixed by #67437
Assignees
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. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@varkor
Copy link
Member

varkor commented Oct 19, 2019

The compiler LLVM is rebuilt whenever the built version of LLVM doesn't have the right hash. (Previously it was rebuilt based on a special marker file, which was easier to work around.)

if done_stamp.exists() {
if let Some(llvm_commit) = llvm_info.sha() {
let done_contents = t!(fs::read(&done_stamp));
// If LLVM was already built previously and the submodule's commit didn't change
// from the previous build, then no action is required.
if done_contents == llvm_commit.as_bytes() {
return build_llvm_config;
}
} else {
builder.info(
"Could not determine the LLVM submodule commit hash. \
Assuming that an LLVM rebuild is not necessary.",
);
builder.info(&format!(
"To force LLVM to rebuild, remove the file `{}`",
done_stamp.display()
));
return build_llvm_config;
}
}

However, most of the time, having the latest LLVM version is not necessary, but makes rebuilding rustc after rebasing take significantly longer, which is a pain. It would be good to be able to disable this as an option in x.py for convenience.

This issue has been assigned to @matthew-healy via this comment.

@varkor varkor added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels Oct 19, 2019
@varkor
Copy link
Member Author

varkor commented Oct 19, 2019

To fix this, you'll want to add another option: you should be able to look at the logic for something like --keep-stage.

rust/src/bootstrap/flags.rs

Lines 130 to 131 in f042687

opts.optmulti("", "keep-stage", "stage(s) to keep without recompiling \
(pass multiple times to keep e.g., both stages 0 and 1)", "N");

@jonas-schievink jonas-schievink added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Oct 19, 2019
@Centril
Copy link
Contributor

Centril commented Oct 20, 2019

It would be good if this could be changed in the config file so you don't have to pass in the option all the time.

@varkor
Copy link
Member Author

varkor commented Oct 20, 2019

@Centril: if it's a config file option, then there should be an x.py option to rebuild it if necessary. I.e. it shouldn't be necessarily to temporarily change the Config.toml to rebuild.

@Centril
Copy link
Contributor

Centril commented Oct 20, 2019

@varkor sure that seems good, although they can be implemented separately.

@spadaval
Copy link

The line you would need to change is near this one: (line 735 of bootstrap.py)

if self.get_toml('llvm-config') and self.get_toml('lld') != 'true':
        continue

It would probably be much easier to add a check against a key in the config, than to add a new flag.

@Walther
Copy link

Walther commented Oct 26, 2019

@rustbot claim 🙂

@rustbot
Copy link
Collaborator

rustbot commented Oct 26, 2019

Error: Parsing assign command in comment failed: ...tbot claim|error: expected end of command at >| 🙂 ...

Please let @rust-lang/release know if you're having trouble with this bot.

@Walther
Copy link

Walther commented Oct 26, 2019

...sorry, my bad!

@rustbot claim

EDIT: oh dear, I'm making it worse 🤦‍♂

@rustbot rustbot assigned rustbot and unassigned rustbot Oct 26, 2019
@matthew-healy
Copy link
Contributor

@Walther are you still working on this? I wouldn't mind picking it up if not.

(Sorry - not really sure what the etiquette is re: dormant tickets!)

@matthew-healy
Copy link
Contributor

matthew-healy commented Dec 19, 2019

Just saw that @Walther's PR (#65848) lapsed. I'm gonna claim this, if that's okay. Will work on something based on the existing comments there.

@rustbot claim

@Walther
Copy link

Walther commented Dec 19, 2019

Thank you for picking this up! Getting things fixed is the important thing, and I'm sure I'll find something else to fix in the near future when I have more time 🙂

aidanhs added a commit to aidanhs/rust that referenced this issue Dec 24, 2019
…Mark-Simulacrum

Add LLVM `skip-rebuild` option to `x.py`

This PR reimplements parts of @Walther's work from rust-lang#65848, and closes rust-lang#65612.

I decided not to implement the [arguments to override this setting](rust-lang#65612 (comment)) in this PR. If there's strong feeling that this change shouldn't be merged without the overrides then I'm happy to close this until I've had a chance to add them in. Otherwise I'll aim to submit a second PR with those this weekend.

I'd have liked to have tested the change in `native.rs`, but there didn't seem to be any existing test infrastructure. I ran this a few times manually and it _worked on my machine_ though... 😬
@bors bors closed this as completed in 41501a6 Dec 27, 2019
Centril added a commit to Centril/rust that referenced this issue Jan 11, 2020
…ion, r=Centril

Add `llvm-skip-rebuild` flag to `x.py`

This PR follows on from rust-lang#67437 to complete the feature request from rust-lang#65612.

Specifically it adds a new command-line flag, `--llvm-skip-rebuild`, which overrides both any value set in `config.toml` and the default value (`false`).

I'm not 100% confident that I've implemented the override in the "best" way, but I've checked it locally and it seems to work at least.

This option isn't currently mentioned in the Guide to Rustc Development. I'd be happy to write something on it if folk think that's worthwhile.
Centril added a commit to Centril/rust that referenced this issue Jan 11, 2020
…ion, r=Centril

Add `llvm-skip-rebuild` flag to `x.py`

This PR follows on from rust-lang#67437 to complete the feature request from rust-lang#65612.

Specifically it adds a new command-line flag, `--llvm-skip-rebuild`, which overrides both any value set in `config.toml` and the default value (`false`).

I'm not 100% confident that I've implemented the override in the "best" way, but I've checked it locally and it seems to work at least.

This option isn't currently mentioned in the Guide to Rustc Development. I'd be happy to write something on it if folk think that's worthwhile.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 5, 2023
…ulacrum

Remove `llvm.skip-rebuild` option

This was added to in 2019 to speed up rebuild times when LLVM was modified. Now that download-ci-llvm exists, I don't think it makes sense to support an unsound option like this that can lead to miscompiles; and the code cleanup is nice too.

r? `@Mark-Simulacrum` cc `@varkor` rust-lang#65612
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. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants