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

Support custom options for LLVM build #93756

Merged
merged 1 commit into from
Feb 10, 2022
Merged

Conversation

tmandry
Copy link
Member

@tmandry tmandry commented Feb 8, 2022

The LLVM build has a lot of options that rustbuild doesn't need to know about. We should allow the user to customize the LLVM build directly.

Here are some example customizations we'd like to do.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 8, 2022
config.toml.example Outdated Show resolved Hide resolved
@Mark-Simulacrum
Copy link
Member

Hm. So, part of me is enthusiastic about supporting something like this, on the other hand, I wonder if it's worth some kind of disclaimer of "at your own risk" or similar -- part of the strategy I've at least sort of tried to steer by is that if folks are trying to extra-customize LLVM, they may just want to build out of tree entirely and provide an llvm-config. (Obviously, that has its own problems).

But I think this is sufficiently general and minimally invasive that I am in general on board with it -- I do wonder if you might be able to ask around and/or have some ideas on other projects embedding LLVM to see if there's some general guidance around the appropriate way to expose this? In particular, with my relatively limited knowledge I'm wondering if just CMAKE defines are typically enough? Or do we need support for general environment configuration / something beyond that?

I will also add that if we do land this it's pretty likely we'll want to drop support for at least some of the existing flag in config.toml that we aren't ourselves interested in using/supporting (e.g., polly build).

@Mark-Simulacrum
Copy link
Member

cc #93629 and #93640 as well, which seem like prime PRs I would "not accept" in favor of using this feature, though as noted on one of those I might have pushed for just leaving LLVM builds external as well.

@catap -- curious if you feel this PR would be enough for you or it's missing something you'd need?

@catap
Copy link

catap commented Feb 8, 2022

@Mark-Simulacrum definitely!

@Mark-Simulacrum
Copy link
Member

r=me with the comment adjusted and llvm_from_ci! gate added.

@Mark-Simulacrum 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 Feb 9, 2022
@tmandry
Copy link
Member Author

tmandry commented Feb 9, 2022

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Feb 9, 2022

📌 Commit 69cd826 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 9, 2022
@tmandry
Copy link
Member Author

tmandry commented Feb 9, 2022

@bors rollup=always

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 10, 2022
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#92670 (add kernel target for RustyHermit)
 - rust-lang#93756 (Support custom options for LLVM build)
 - rust-lang#93802 (fix oversight in the `min_const_generics` checks)
 - rust-lang#93808 (Remove first headings indent)
 - rust-lang#93824 (Stabilize cfg_target_has_atomic)
 - rust-lang#93830 (Refactor sidebar printing code)
 - rust-lang#93843 (kmc-solid: Fix wait queue manipulation errors in the `Condvar` implementation)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 584948d into rust-lang:master Feb 10, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 10, 2022
@tmandry tmandry deleted the llvm-build-config branch February 14, 2022 19:48
@jonhoo
Copy link
Contributor

jonhoo commented Mar 7, 2022

I think this also needs an update to src/bootstrap/configure.py to allow setting this property through an argument, since as far as I can tell it doesn't have a meaningful way to set dictionaries at the moment.

@jonhoo
Copy link
Contributor

jonhoo commented Mar 7, 2022

Ah, or perhaps it'll just work by including the key for the dictionary in --set (so --set llvm.build-config.LLVM_ENABLE_ZLIB=FORCE_ON for example)?

@catap
Copy link

catap commented Mar 7, 2022

@jonhoo you may use ./configure --set='llvm.build-config={A = 2, C = D}' when you need multiple options

@Mark-Simulacrum
Copy link
Member

I would expect --set llvm.config.foo=bar to work - do we have something hard coded to two elements in the path?

@catap
Copy link

catap commented Mar 7, 2022

@Mark-Simulacrum unfortently it is a bit more tricky. Special when someone needs a few LLVM options. Anyway, some ugly configure set make the job done.

@jonhoo
Copy link
Contributor

jonhoo commented Mar 7, 2022

Yeah, I just tried:

--set "llvm.build-config.LLVM_TEMPORARILY_ALLOW_OLD_TOOLCHAIN=1" \
--set "llvm.build-config.LLVM_ENABLE_ZLIB=FORCE_ON"

and got

Traceback (most recent call last):
  File "./src/bootstrap/configure.py", line 467, in <module>
    configure_section(sections[section_key], section_config)
  File "./src/bootstrap/configure.py", line 447, in configure_section
    lines[i] = "{} = {}".format(key, to_toml(value))
  File "./src/bootstrap/configure.py", line 436, in to_toml
    raise RuntimeError('no toml')
RuntimeError: no toml

@catap
Copy link

catap commented Mar 7, 2022

@jonhoo --set='llvm.build-config={LLVM_TEMPORARILY_ALLOW_OLD_TOOLCHAIN=1,LLVM_ENABLE_ZLIB=FORCE_ON}' works as you expected

@jonhoo
Copy link
Contributor

jonhoo commented Mar 7, 2022

I can also confirm that I get the same error even if I just set a single key this way. Will try your way now @catap.

@jonhoo
Copy link
Contributor

jonhoo commented Mar 7, 2022

@catap That doesn't look quite right either. It gives me

# Custom CMake defines to set when building LLVM.
build-config = '{LLVM_TEMPORARILY_ALLOW_OLD_TOOLCHAIN=1,LLVM_ENABLE_ZLIB=FORCE_ON}'

Which sets build-config to a string, not a dictionary, which will then fail to deserialize into a Map when the bootstrap logic picks it up.

@jonhoo
Copy link
Contributor

jonhoo commented Mar 7, 2022

I don't think that's true — I'm running it in bash, and I think that's the right quoting for the command. The problem is that set expects the value to always be set to either a string, a list, or bool. There's no support for emitting it as a dict.

@catap
Copy link

catap commented Mar 7, 2022

@jonhoo let me dig, how I've used it. You're right.

@jonhoo
Copy link
Contributor

jonhoo commented Mar 8, 2022

Separately, I also noticed that the way this is currently implemented, llvm.build-config doesn't apply to the LLD and sanitizer buildes. I wonder if

for (key, val) in &builder.config.llvm_build_config {
cfg.define(key, val);
}

should move to

fn configure_cmake(

which all of the three LLVM-based steps use?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants