-
Notifications
You must be signed in to change notification settings - Fork 77
Move Zig /tmp cache to a Bazel repository #183
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
base: main
Are you sure you want to change the base?
Conversation
environment variables.
|
Consider updating the setup instructions in the README and adding a "fixes #83" (if I understand this PR right) |
|
This has been bugging my mind for longer than I want to admit. Thanks. :) Will leave a real review to @linzhp. |
| else: | ||
| fail("unknown os: {}".format(os)) | ||
| label = Label("@zig_sdk_cache//:zig.env") | ||
| cache_prefix = str(repository_ctx.path(label).dirname) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this inject an absolute path to Bazel's output base into the action key of every action that uses this toolchain?
This would likely induce cache misses with a shared remote cache across machines.
Also, how does this interact with remote execution setups?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out! Indeed, it does leak ugly sandboxing details into the action invocation.
Perhaps we can make this a relative path, starting from /external?. I tried this locally, where cache_prefix becomes something like external/zig_sdk_cache. This did not work OOTB, and I think the reason is because ZIG_LOCAL_CACHE_DIR and ZIG_GLOBAL_CACHE_DIR are not intended to handle relative paths. In which case, perhaps we can modify zig-wrapper.zig to reconnect the relative path to the absolute path of the output base within the action sandbox? FWIW, it appears that the wrapper does something similar to this to compute ZIG_LIB_DIR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the reason is because
ZIG_LOCAL_CACHE_DIRandZIG_GLOBAL_CACHE_DIRare not intended to handle relative paths.
FWIW I used to use relative local and global cache paths in rules_zig until aherrmann/rules_zig#253 . The reason for the switch was unrelated to the relative paths and instead had to do with Zig 0.12 producing dangling symlinks in the cache which Bazel doesn't allow for build outputs...
That said, it's not hard to believe that relative cache dirs could be problematic, e.g. I encountered ziglang/zig#19284 in a 0.12 pre-release build. Converting to an absolute path in the wrapper sounds like a good middle ground.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can modify zig-wrapper.zig to reconnect the relative path to the absolute path of the output base within the action sandbox
I like this approach. Maybe we can set ZIG_LOCAL_CACHE_DIR and ZIG_GLOBAL_CACHE_DIR to $(pwd)/$HERMETIC_CC_TOOLCHAIN_CACHE_PREFIX in zig-wrapper.zig?
|
Hi, @vardaro. We're seeing failing builds with |
Hey, I'm no longer working at the employer funding this work. I'm surprised this is still relevant, I recall it being fixed by another PR? |
|
Thanks for your response! I think it is, as If you're no longer working on this, would you mind closing this PR? I'm going to take a stab rebasing this branch on the latest |
At this time, the
hermetic_cc_toolchaintoolchain directs the Zig cache to the host machines/tmpdirectory. This directory is referenced absolutely and remains external to Bazel, leading to reproducibility issues. This change proposes directing the Zig build cache to a known Bazel repository, eliminating the dependency on/tmp.At this time, Zig input environment variables are composed in the
zig_repositoryimplementation, however I think we could move this logic intozig_repository_cache, and encode those variables in thezig.envfile it creates. This is similar to how Gazelle resolves Go environment variables. I didn't do it here to keep the PR scoped to eliminating/tmp.I tested this on Datadog's internal monorepo, but is there a more established process to test these changes?