search

bazel-contrib / toolchains_llvm

LLVM toolchain for bazel
Apache License 2.0
291 stars 209 forks source link

Allow user provided platform constraints #371

Closed jkurland-roku closed 1 week ago

jkurland-roku commented 1 month ago

An implementation of the fix suggested in https://github.com/bazel-contrib/toolchains_llvm/issues/361 to allow users to specify additional platform constraints for each toolchain.

My personal use case was building some targets with musl and the toolchains here were interferring.

fmeum commented 1 month ago

Could you add a test usage of these new attributes to https://github.com/bazel-contrib/toolchains_llvm/blob/master/tests/MODULE.bazel? You could add synthetic constraint_settings with default values so that the default platform matches them. I would just like to get some coverage of the new Starlark code.

jkurland-roku commented 1 month ago

I've added a test, the only issue I think is that it causes another llvm toolchain to be downloaded and extracted (or just extracted if there's a repository cache), so up to you if you think it's worth the extra CI time.

fmeum commented 4 weeks ago

@jkurland-roku I fixed the reference to the host platform, but tests are still failing

jkurland-roku commented 4 weeks ago

Looks like the ubuntu failures are when bzlmod is disabled which makes sense and I'll look to fix that. The macos failures might be to do with the cxxflags, maybe there's another mechanism to detect which toolchain was selected

jkurland-roku commented 4 weeks ago

I think those changes should fix both issues (although not sure about macos, I don't really know anything about macos). I've tested with bzlmod disabled so at least that should work

jkurland-roku commented 3 weeks ago

So I think the issue is that on the platforms which only support up to llvm 13.0.0 the c++20 toolchain fails to build. So I'm thinking that maybe I get rid of the current c++20 check and instead make a new platform for zlib supporting linker (a better name would be helpful) and then switch to llvm 13 automatically based on that. So replacing the "extra_toolchain" arg in the CI with "--host_platform=@//:archlinux_docker" for example.

What do you think?

fmeum commented 2 weeks ago

That's one approach. You could also try to disable the test for these platforms. Generic starlark changes that work on two common platforms are very unlikely to break on untested platforms.

jkurland-roku commented 1 week ago

I've made this test manual and now only run it when the toolchain isn't overriden

fmeum commented 1 week ago

Thanks for carrying this through!

JKurland commented 1 week ago

No worries, thanks for the help