Skip to content

Conversation

@birunts
Copy link
Contributor

@birunts birunts commented Feb 12, 2025

@zig_sdk repository to hold all toolchain definitions.

Simplify toolchain registering API.

MODULE.bazel:

toolchain module_extension now creates repositories of:

  • @zig_sdk: holds all common platform, constraint and all supported toolchain definitions
  • @zig_config-{os}-{arch}: contains toolchain config rules and all needed targets to register toolchains for particular platform

WORKSPACE:

Comparing to above, the difference here is that by default (means no exec_platforms attr passed to zig_toolchains() call) only one @zig_config repository will be created for the host platform @zig_sdk is the same as for bzlmod case.

Possible toolchains registration with:

register_toolchains(
    "@zig_sdk//...",
)

@zig_config* repo(s) is/are not visible to main repository.

Resolves #204 & Closes #204

@CLAassistant
Copy link

CLAassistant commented Feb 12, 2025

CLA assistant check
All committers have signed the CLA.

@birunts birunts force-pushed the sandbox/zig_toolchains_api branch from 08c2ab4 to 29c90bd Compare February 17, 2025 09:54
@linzhp
Copy link
Contributor

linzhp commented Feb 17, 2025

Do you think it possible to keep the toolchain in their current locations, and register with:

register_toolchains(
    "@zig_sdk//toolchain/...",
    "@zig_sdk//libc_aware/toolchain/...",
)

@birunts
Copy link
Contributor Author

birunts commented Feb 18, 2025

Hi @linzhp, thanks for having a look.
Do you mean to move all from @zig_toolchains to @zig_sdk where exec platform is a subpackage of toolchain & libc_aware/toolchain, i.e.:

register_toolchains(
    "@zig_sdk//toolchain/linux-amd64/...",
    "@zig_sdk//libc_aware/toolchain/windows-arm64/...",
)

Yeah, I guess this is doable 😄 I will try to merge both repos then.

@birunts birunts changed the title refactor: zig_toolchains API refactor: register toolchains API with @zig_sdk//... Feb 18, 2025
@birunts birunts changed the title refactor: register toolchains API with @zig_sdk//... refactor: register toolchains with @zig_sdk//... Feb 18, 2025
@linzhp
Copy link
Contributor

linzhp commented Feb 18, 2025

When zig_toolchains() is called without an argument of exec_platforms, hermetic_cc_toolchain creates only one repository with configs for the HOST, i.e. @zig_config

Can we have the same behavior for MODULE.bazel? I saw toolchains like @zig_sdk//toolchain:linux_amd64_gnu.2.38 are missing in bazel modules.

@birunts
Copy link
Contributor Author

birunts commented Feb 20, 2025

I saw toolchains like @zig_sdk//toolchain:linux_amd64_gnu.2.38 are missing in bazel modules.

Yes, that was because module extension has always pass _COMMON_EXEC_PATFORMS dict and there was no @zig_config for the HOST but @zig_config-linux-amd64 instead, thus toolchains places in ../linux-amd64/... subpackage.
After all this has made me think this was not the best approach.

With the latest change @zig_config is always created for the HOST, thus all toolchains will be same as before, so in a way of:

"@zig_sdk//toolchain/...",
"@zig_sdk//libc_aware/toolchain/...",

Only toolchains for an additional execution platforms will be placed in corresponding subpackage, e.g. for windows-amd64 those will be in:

"@zig_sdk//toolchain/windows-amd64/...",
"@zig_sdk//libc_aware/toolchain/windows-amd64/...",

BREAKING CHANGE: `zig_toolchains` reposotory
holds all toolchains definitions.

Simplify toolchain registering API.

MODULE.bazel:
`toolchain` module_extension now creates repositories:
 - `@zig_skd`: holds all common platforms & constraints
   definitions
 - `zig_config-{os}-{arch}`: contains toolchain config rules
   and all needed targets to register toolchains for particular
   platform
 - `zig_toolchains`: holds all supported toolchain
   definitions.

WORKSPACE:
The difference here is that only one
`@zig_config` repository will be created for the
host platform. `@zig_sdk` & `zig_toolchains` are the
same as for bzlmod case.

Possible toolchain registration with:

```py
register_toolchains(
    "@zig_toolchains//...",
)
```
Make toolchain for zig wrapper target.
Define zig wrapper toolchain targets
for every supported platform.
Use zig wrapper toolchain for `zig_binary` rule.
As we now registers all available toolchains with
`register_toolchains("@zig_toolchains//...")` the first
one registered who will match is `linux_amd64_gnu.2.17`.

Reference:
https://bazel.build/extending/toolchains#toolchain-resolution
Update documentation with the `@zig_sdk`,
`@zig_toolchains` and `@zig_config-{os}-{arch}`
repository changes.
Move/rename definitions/files to make all more
consistent as a whole.
Fix bazel queries to print particular targets of
`@zig_sdk` & `@zig_toolchains` repositories.
`hermetic_cc_toolchain` does not supports MacOS SDK
thus do not register affected toolchains
(uber#10).

Add missing alias for host `zig`:
 - `@zig_sdk//:zig`
Separate `@zig_toolchains` repository makes to big noise
thus all toolchain definitions should go to `@zig_sdk`.

Merge `@zig_toolchains` repository into `@zig_sdk`.
@birunts birunts force-pushed the sandbox/zig_toolchains_api branch from 3045429 to 2c79f8e Compare February 20, 2025 07:05
Copy link
Contributor

@linzhp linzhp left a comment

Choose a reason for hiding this comment

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

Currently, rules can depend on the zig binary with something like:

        "_compiler": attr.label(
            executable = True,
            cfg = "exec",
            default = "@zig_sdk//:zig",
            allow_single_file = True,
        ),

Can we continue to support this usage? With this PR, such rules would fail with:

alias '@@zig_sdk//:zig' referring to target '@@zig_config//:zig' is not visible from target '//some/package:target'. Check the visibility declaration of the former target if you think the dependency is legitimate. To set the visibility of that source file target, use the exports_files() function

Create toolchains for HOST execution platform in
default location of:

```
"@zig_sdk//toolchain/...",
"@zig_sdk//libc_aware/toolchain/...",
```

Add `sdk` tag class for module extention to specify
additional exwcution platforms than HOST. Those will
have additional _subpackage_ of `{os}-{arch}` i.e.

```
"@zig_sdk//toolchain/{os}-{arch}/...",
"@zig_sdk//libc_aware/toolchain/{os}-{arch}/...",
```
@birunts birunts force-pushed the sandbox/zig_toolchains_api branch from 2c79f8e to 1e3a563 Compare February 24, 2025 08:34
@birunts
Copy link
Contributor Author

birunts commented Feb 24, 2025

Can we continue to support this usage?

        "_compiler": attr.label(
            executable = True,
            cfg = "exec",
            default = "@zig_sdk//:zig",
            allow_single_file = True,
        ),

We can, but this would be wrong choice.

Having _compiler attribute who points always to @zig_sdk//:zig will work only for HOST execution platform. That is why I have introduced zig_toolchain toolchain, as also every @zig_config* will create one suitable for its execution platform. Thus you do not need to pass (which now is impossible as attribute is hidden) correct zig binary, but will always get correct one when registered toolchains 🙂

E.g. you can have:

@zig_sdk//toolchain:zig  # HOST execution platform 
@zig_sdk//toolchain/macos-arm64:zig  # macos-arm64 execution platform

This way rules/zig_binary can be called freely in every registered execution platform.

When it comes to @zig_sdk//:zig target, it's an alias for zig-wrapper binary for the HOST - who can be run directly as it's done across CI.
I've added missing ["//visibility:public"] for its package, which should resolve the error you've mentioned.

@linzhp linzhp merged commit 892973b into uber:main Feb 26, 2025
8 checks passed
@birunts birunts deleted the sandbox/zig_toolchains_api branch March 7, 2025 08:56
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.

Change of toolchain registering API to simplify multiple execution platforms existing for toolchains.

3 participants