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

feat: create cc_toolchains for multiple exec platforms #196

Merged
merged 3 commits into from
Nov 28, 2024

Conversation

BrandonThomasJonesARM
Copy link
Contributor

This change aims to allow hermetic_cc_toolchain to create toolchains for multiple exec platforms. It now generates repositories of the format {exec}-zig_sdk and registers each toolchain to only be exec_compatible_with those specific platform constraints.

I want this PR to be a WIP as we discuss how we want this to look and if you're happy with it's general flow.

This is an attempt at fixing #148

So far I have tested that my local client (mac) can build c code using the toolchains registered on a x86_64 remote executor and have seen success. It'll will be hard for me to test every combination; but I will be able to local (x86_64) -> remote (arm64) tests as well.

@CLAassistant
Copy link

CLAassistant commented Oct 11, 2024

CLA assistant check
All committers have signed the CLA.

@BrandonThomasJonesARM
Copy link
Contributor Author

@linzhp @motiejus Apologies, not sure who is best to ping, but some early eyes on this would be great to understand if you're happy with it's direction.

@BrandonThomasJonesARM
Copy link
Contributor Author

BrandonThomasJonesARM commented Oct 11, 2024

I know that a valid usecase for this project is using --platforms @zig_sdk//platforms:amd64_linux and we use the mechanism to rely on specific libc versions too if we register >1.

I suggest that the target platform definitions should either be defined inside of hermetic_cc_toolchain itself or a separate generated repository. For the purpose of toolchain resolution, only their constraint_values matter, so should be decoupled from the toolchain logic that generates exec platform aware toolchains.

e.g.
@amd64-linux-zig_sdk//platforms:amd64_linux matches the constraints of
@arm64-linux_zig_sdk//platforms:amd64_linux as they're both describing the target platform only.

so it's likely best this PR does some minimum further work to generate these platforms somewhere central.

toolchain/defs.bzl Outdated Show resolved Hide resolved
toolchain/ext.bzl Outdated Show resolved Hide resolved
toolchain/defs.bzl Show resolved Hide resolved
@BrandonThomasJonesARM
Copy link
Contributor Author

Thanks for the initial look @linzhp

I have addressed the comments made, I have added a local repository rule for zig_sdk that symlinks to the exec repo that matches the host. This is to try and ensure backwards compatibility somewhat. I am not entirely confident with it; it might cause more problems than it solves.

@BrandonThomasJonesARM BrandonThomasJonesARM marked this pull request as ready for review October 24, 2024 08:46
rules/zig_binary.bzl Outdated Show resolved Hide resolved
toolchain/defs.bzl Outdated Show resolved Hide resolved
toolchain/defs.bzl Outdated Show resolved Hide resolved
@BrandonThomasJonesARM
Copy link
Contributor Author

I have tested the latest iteration with RE where host was x86 and my remote was arm64, and got successful compilations. I cannot run the full test suite yet as i'm running into a problem with rules_go that I am looking into.

@BrandonThomasJonesARM
Copy link
Contributor Author

@linzhp ping/bump , is there anything else outstanding on this PR you currently want me to address?

@linzhp
Copy link
Contributor

linzhp commented Nov 18, 2024

I can take a look sometime this week

@linzhp
Copy link
Contributor

linzhp commented Nov 25, 2024

There is some issues with Windows' platform names though

@BrandonThomasJonesARM
Copy link
Contributor Author

BrandonThomasJonesARM commented Nov 25, 2024

There is some issues with Windows' platform names though

Let me investigate.

Hopefully fixed, I wasn't doing correct substitution for windows/macos for the local repository symlinking logic as I dont have a machine to test those on necessarily.

@BrandonThomasJonesARM BrandonThomasJonesARM force-pushed the exec_platforms branch 2 times, most recently from b8ad790 to b3cd108 Compare November 25, 2024 15:37
@BrandonThomasJonesARM
Copy link
Contributor Author

Pushed a change that hopefully fixed the failing worksflows, could we try again please? Is there any way for me to run the majority of it locally outside of bazel test //... to ensure I am not wasting unnecessary resource time testing it in your CI flow?

@linzhp
Copy link
Contributor

linzhp commented Nov 26, 2024

That's the best bet. I think the main problem is the support for different platforms. I just changed the permission so hopefully your updates can automatically trigger CI without my approval. That makes your iteration faster.

@BrandonThomasJonesARM BrandonThomasJonesARM force-pushed the exec_platforms branch 2 times, most recently from 641da30 to 46689dc Compare November 28, 2024 10:33
@BrandonThomasJonesARM
Copy link
Contributor Author

BrandonThomasJonesARM commented Nov 28, 2024

Looks like macos fails for reasons outside my control; see #131

This seems to be because macos isn't allowed to cross-compile the other toolchains that are being registered. For Macos I suppose we need to not register the other platforms, I am unsure the pattern for that.

@BrandonThomasJonesARM BrandonThomasJonesARM force-pushed the exec_platforms branch 6 times, most recently from 09a831d to d68999b Compare November 28, 2024 15:00
@BrandonThomasJonesARM
Copy link
Contributor Author

BrandonThomasJonesARM commented Nov 28, 2024

Fixed all failures outside of the releaase checks, I have attempted to change the API for the WORKSPACE file, but it fails on trying to use the released version of hermetic_cc_toolchain as it does not have the new change.

@linzhp Can you recommend the correct approach to fixing this? I don't see a way of doing this without breaking the API for toolchains() inside of defs/toolchains.bzl

In the meantime I'll attempt a refactor that is fully backwards compatible.

UPDATE: Added backwards compatibility, was simpler than I thought, I don't usually interact with WORKSPACE files. Hopefully it's in a good state now. Thanks again for you patience while I fight the multiple platform issues.

This change aims to allow hermetic_cc_toolchain to create
toolchains for multiple exec platforms. It now generates
repositories of the format {exec}-zig_sdk and registers
each toolchain to only be exec_compatible_with those
specific platform constraints.
@linzhp linzhp merged commit aefb7a1 into uber:main Nov 28, 2024
8 checks passed
@linzhp
Copy link
Contributor

linzhp commented Nov 28, 2024

Thanks for the contribution!

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