Detect configuration for LLVM during setup #77756

alarsyo · 2020-10-09T16:33:55Z

This is a first draft to address #77579, setting download-ci-llvm to true on Linux, but I could also implement the if-available setting mentioned in the issue.

On other platforms I was thinking about using the which crate, if adding a dependency on it is considered okay of course, to detect the presence of llvm-config in the path, and use it if found. Still a work in progress of course.

rust-highfive · 2020-10-09T16:33:59Z

r? @Mark-Simulacrum

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

Mark-Simulacrum · 2020-10-09T16:40:50Z

I would not want to add a dependency for this sort of thing, it shouldn't be necessary to just scan PATH and append llvm-config to it.

I don't think appending to the file is going to work well, I think the if-available approach is the one we should pursue.

alarsyo · 2020-10-09T21:10:25Z

Is that better? Hopefully the "if-available" option is now handled everywhere needed, and I just need to write the llvm-config detection logic for other platforms

src/bootstrap/setup.rs

bors · 2020-10-12T21:34:32Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

src/bootstrap/bootstrap.py

Mark-Simulacrum · 2020-10-13T13:49:06Z

src/bootstrap/setup.rs

+[llvm]
+# Will download LLVM from CI if available on your platform (Linux only for now)
+# https://github.com/rust-lang/rust/issues/77084 tracks support for more platforms
+download-ci-llvm = "if-available"


Could we just duplicate this across the actual default files? I would prefer to avoid adding even more layers of places where defaults are getting set.

Yeah, in my mind setup.rs is for setting defaults that are not the same for everyone, while src/bootstrap/defaults are for things that can reasonably be shared.

Done! Added to all default configs. I just have a doubt for the config.codegen.toml, maybe people working close to codegen don't want the CI built one by default?

Ah, it seems moving these lines to the default profiles broke the bootstrap, because bootstrap.py doesn't know about the profile setting (I'm guessing it's only handled in the config.rs file)

Looking further into this, it seems bootstrap only has a basic regex mechanism to lookup toml keys, and returns the first key that matches, which means appending the contents of our default file should work as expected, allowing the main config.toml to override these defaults if needed. Definitely feels like a hack though :D I'll push some code soon

Got it, thanks.

@Mark-Simulacrum how hard would it be to move that into rustbuild instead of bootstrap.py? Doesn't need to block this, but I'd like to do as little as possible in python anyway.

That error comes from the Rust portion of rustbuild, I think? It looks like a Rust error...

Presumably LLVM was not downloaded or not unpacked which led to this problem.

FWIW, I'd be interested in tackling this to get more familiar with the bootstrap code, if you decide to move this to rustbuild :)

Presumably LLVM was not downloaded or not unpacked which led to this problem.

Yes, the rust code doesn't find the executable because it wasn't downloaded by bootstrap.py, because bootstrap.py didn't know about the default file, so the get_toml('download-ci-llvm') returned None

Let's tackle this in a follow-up, it seems reasonable to have bootstrap.py read profiles anyway.

src/bootstrap/setup.rs

jyn514 · 2020-10-13T16:03:37Z

src/bootstrap/bootstrap.py

+        with open(include_path) as included_toml:
+            build.config_toml += '\n' + included_toml.read()


I'm 90% sure this will break - this causes invalid syntax if there's already anything in the config.toml. #76628 (comment)

It would break if parsed by a real TOML parser, but that's not the case, the basic regex parser will just match the first relevant key

(and since the aggregation of the two files is only stored as a string in memory, not written to a toml file somewhere, I don't think it should break anything else?)

src/bootstrap/bootstrap.py

alarsyo · 2020-10-14T11:10:52Z

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

jyn514 · 2020-10-14T17:33:37Z

@bors r+

In a follow-up I'd like to move handling of LLVM from bootstrap.py to rustbuild, but that doesn't need to block this change.

bors · 2020-10-14T17:33:39Z

📌 Commit b8ae4c5 has been approved by jyn514

bors · 2020-10-14T17:33:40Z

🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened

alarsyo · 2020-10-14T17:42:52Z

@jyn514 should I open an issue about a follow-up change for rustbuild ?

Mark-Simulacrum · 2020-10-14T17:49:09Z

It would mean a curl or other "http talker" dep in rustbuild which I'm opposed to at this point I think, but I don't mind an issue.

bors · 2020-10-15T02:10:17Z

⌛ Testing commit b8ae4c5 with merge 19e1aac...

bors · 2020-10-15T04:17:03Z

☀️ Test successful - checks-actions, checks-azure
Approved by: jyn514
Pushing 19e1aac to master...

rust-highfive assigned Mark-Simulacrum Oct 9, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 9, 2020

alarsyo force-pushed the setup-llvm-detect branch from 050b60a to 004fd4c Compare October 9, 2020 21:03

alarsyo force-pushed the setup-llvm-detect branch from 004fd4c to 8f88847 Compare October 9, 2020 21:18

jyn514 added A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Oct 10, 2020

jyn514 reviewed Oct 10, 2020

View reviewed changes

src/bootstrap/setup.rs Outdated Show resolved Hide resolved

src/bootstrap/setup.rs Outdated Show resolved Hide resolved

src/bootstrap/setup.rs Outdated Show resolved Hide resolved

alarsyo force-pushed the setup-llvm-detect branch from 8f88847 to b3f6336 Compare October 10, 2020 13:59

Mark-Simulacrum reviewed Oct 13, 2020

View reviewed changes

Mark-Simulacrum 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 Oct 13, 2020

alarsyo force-pushed the setup-llvm-detect branch 2 times, most recently from 7345c61 to c39c869 Compare October 13, 2020 14:59

Implement "if-available" option for download-ci-llvm

ab614b0

alarsyo force-pushed the setup-llvm-detect branch from 7a8deec to e65f720 Compare October 13, 2020 16:00

jyn514 reviewed Oct 13, 2020

View reviewed changes

Handle included config in bootstrap.py

b8ae4c5

alarsyo force-pushed the setup-llvm-detect branch from e65f720 to b8ae4c5 Compare October 13, 2020 16:08

jyn514 reviewed Oct 13, 2020

View reviewed changes

src/bootstrap/bootstrap.py Show resolved Hide resolved

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 14, 2020

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 14, 2020

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 14, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 15, 2020

bors merged commit 19e1aac into rust-lang:master Oct 15, 2020

rustbot added this to the 1.49.0 milestone Oct 15, 2020

alarsyo deleted the setup-llvm-detect branch October 15, 2020 14:20

alarsyo mentioned this pull request Oct 15, 2020

move download-ci-llvm logic from bootstrap.py to rustbuild #77977

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect configuration for LLVM during setup #77756

Detect configuration for LLVM during setup #77756

alarsyo commented Oct 9, 2020

rust-highfive commented Oct 9, 2020

Mark-Simulacrum commented Oct 9, 2020

alarsyo commented Oct 9, 2020

bors commented Oct 12, 2020

Mark-Simulacrum Oct 13, 2020

jyn514 Oct 13, 2020

alarsyo Oct 13, 2020

alarsyo Oct 13, 2020 •

edited

Loading

alarsyo Oct 13, 2020

jyn514 Oct 13, 2020

Mark-Simulacrum Oct 13, 2020

alarsyo Oct 13, 2020

alarsyo Oct 13, 2020 •

edited

Loading

jyn514 Oct 14, 2020

jyn514 Oct 13, 2020

alarsyo Oct 13, 2020

alarsyo Oct 13, 2020

alarsyo commented Oct 14, 2020

jyn514 commented Oct 14, 2020

bors commented Oct 14, 2020

bors commented Oct 14, 2020

alarsyo commented Oct 14, 2020

Mark-Simulacrum commented Oct 14, 2020

bors commented Oct 15, 2020

bors commented Oct 15, 2020

		with open(include_path) as included_toml:
		build.config_toml += '\n' + included_toml.read()

Detect configuration for LLVM during setup #77756

Detect configuration for LLVM during setup #77756

Conversation

alarsyo commented Oct 9, 2020

rust-highfive commented Oct 9, 2020

Mark-Simulacrum commented Oct 9, 2020

alarsyo commented Oct 9, 2020

bors commented Oct 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alarsyo Oct 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alarsyo Oct 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alarsyo commented Oct 14, 2020

jyn514 commented Oct 14, 2020

bors commented Oct 14, 2020

bors commented Oct 14, 2020

alarsyo commented Oct 14, 2020

Mark-Simulacrum commented Oct 14, 2020

bors commented Oct 15, 2020

bors commented Oct 15, 2020

alarsyo Oct 13, 2020 •

edited

Loading

alarsyo Oct 13, 2020 •

edited

Loading