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

Move Zig /tmp cache to a Bazel repository #183

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions toolchain/defs.bzl
Original file line number Diff line number Diff line change
@@ -77,6 +77,10 @@ def toolchains(
mirror_format = original_format.replace("https://ziglang.org/", "https://mirror.bazel.build/ziglang.org/")
url_formats = [mirror_format, original_format]

zig_repository_cache(
name = "zig_sdk_cache",
)

zig_repository(
name = "zig_sdk",
version = version,
@@ -147,14 +151,8 @@ def _zig_repository_impl(repository_ctx):

cache_prefix = repository_ctx.os.environ.get("HERMETIC_CC_TOOLCHAIN_CACHE_PREFIX", "")
if cache_prefix == "":
if os == "windows":
cache_prefix = "C:\\\\Temp\\\\zig-cache"
elif os == "macos":
cache_prefix = "/var/tmp/zig-cache"
elif os == "linux":
cache_prefix = "/tmp/zig-cache"
else:
fail("unknown os: {}".format(os))
label = Label("@zig_sdk_cache//:zig.env")
cache_prefix = str(repository_ctx.path(label).dirname)

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?

Copy link
Author

@vardaro vardaro Jun 6, 2024

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.

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_DIR and ZIG_GLOBAL_CACHE_DIR are 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.

Copy link
Contributor

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?


repository_ctx.template(
"tools/zig-wrapper.zig",
@@ -235,6 +233,15 @@ zig_repository = repository_rule(
implementation = _zig_repository_impl,
)

def _zig_repository_cache_impl(ctx):
ctx.file("zig.env", "")
ctx.file("BUILD.bazel", 'exports_files(["zig.env"])')

zig_repository_cache = repository_rule(
implementation = _zig_repository_cache_impl,
attrs = {},
)

def filegroup(name, **kwargs):
native.filegroup(name = name, **kwargs)
return ":" + name