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

Allow user provided platform constraints #371

Merged
merged 7 commits into from
Sep 6, 2024

Conversation

jkurland-roku
Copy link
Contributor

An implementation of the fix suggested in #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.

Copy link
Member

@fmeum fmeum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up, looks pretty good overall already.

toolchain/internal/repo.bzl Outdated Show resolved Hide resolved
toolchain/internal/repo.bzl Outdated Show resolved Hide resolved
toolchain/extensions/llvm.bzl Outdated Show resolved Hide resolved
toolchain/internal/repo.bzl Outdated Show resolved Hide resolved
@fmeum
Copy link
Member

fmeum commented Aug 16, 2024

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
Copy link
Contributor Author

jkurland-roku commented Aug 16, 2024

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.

Copy link
Member

@fmeum fmeum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice test!

tests/MODULE.bazel Outdated Show resolved Hide resolved
tests/MODULE.bazel Outdated Show resolved Hide resolved
@fmeum
Copy link
Member

fmeum commented Aug 18, 2024

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

@jkurland-roku
Copy link
Contributor Author

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

auto-merge was automatically disabled August 19, 2024 09:50

Head branch was pushed to by a user without write access

@jkurland-roku
Copy link
Contributor Author

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

@fmeum fmeum force-pushed the master branch 2 times, most recently from 8fe5e8e to 55ac2d3 Compare August 22, 2024 14:44
@jkurland-roku
Copy link
Contributor Author

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
Copy link
Member

fmeum commented Aug 29, 2024

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
Copy link
Contributor Author

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

…erent tests, add host_platform repo in non bzlmod tests
@fmeum fmeum merged commit 1cd9e36 into bazel-contrib:master Sep 6, 2024
35 checks passed
@fmeum
Copy link
Member

fmeum commented Sep 6, 2024

Thanks for carrying this through!

@JKurland
Copy link

JKurland commented Sep 6, 2024

No worries, thanks for the help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants