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

Make system module map generation faster and fully hermetic #280

Merged
merged 1 commit into from
Mar 14, 2024

Conversation

fmeum
Copy link
Member

@fmeum fmeum commented Mar 3, 2024

  • system_module_maps no longer performs any IO.
  • The generated module map no longer references any non-hermetic paths and can thus be cached remotely, even when toolchain or sysroot are provided as absolute paths.

@fmeum fmeum force-pushed the improved-module-map branch 5 times, most recently from 3b4d52b to f2ee72f Compare March 4, 2024 11:02
toolchain/toolchains.bzl.tpl Outdated Show resolved Hide resolved
@fmeum fmeum marked this pull request as ready for review March 4, 2024 11:56
Copy link
Contributor

@siddharthab siddharthab left a comment

Choose a reason for hiding this comment

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

Please land whenever you are ready. Let's make a release when this lands.

toolchain/internal/system_module_map.bzl Outdated Show resolved Hide resolved
toolchain/toolchains.bzl.tpl Outdated Show resolved Hide resolved
@siddharthab siddharthab mentioned this pull request Mar 13, 2024
@fmeum fmeum force-pushed the improved-module-map branch from f2ee72f to 1a11e9f Compare March 14, 2024 09:49
@fmeum fmeum enabled auto-merge (squash) March 14, 2024 10:21
@fmeum fmeum merged commit 81f85c0 into bazel-contrib:master Mar 14, 2024
35 checks passed
@fmeum fmeum deleted the improved-module-map branch March 14, 2024 10:23
jsharpe added a commit to jsharpe/bazel-toolchain that referenced this pull request Jun 5, 2024
@@ -586,3 +593,9 @@ native_binary(
llvm_dist_label_prefix = llvm_dist_label_prefix,
host_dl_ext = host_dl_ext,
)

def _is_hermetic_or_exists(rctx, path, sysroot_prefix):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please help me understand how to make RBE work in this case? For example when running bazel from macOS and executing in a remote Linux executor, /usr/include does not exist on macOS but exists on Linux, this filter will remove /usr/include from cxx_builtin_include_directories and that could result in bazel validation failure ...includes the following non-builtin files with absolute paths (if these are builtin files, make sure these paths are in your toolchain).

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't have this scenario in mind when I wrote this, my assumption being that you would only use this toolchain for RBE together with a hermetic sysroot.

This function only makes sense if you are actually targetting the host, so we should probably disable it when cross compiling and just use all directories unfiltered. If you want to give that a try, I can help you implement and test this approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I'm not sure of a reasonable implementation. What would you suggest?

Copy link
Member Author

Choose a reason for hiding this comment

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

The _is_hermetic_or_exists function should be fed the information whether exec_os equals target_os and only perform the existence check if so.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I was looking at those, but not sure that is reliable. For RBE, exec_os could be the same as target_os, right? For example I can define a toolchain like the following and while produce target for linux as well.

llvm.toolchain(
    name = "llvm_toolchain_linux_exec",
    exec_arch = "amd64",
    exec_os = "linux",
    llvm_version = LLVM_VERSION,
)

Copy link
Member Author

Choose a reason for hiding this comment

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

You are completely right, I guess we need to match the host OS (obtained e.g. from repository_ctx.os) against the target OS (this determines the shape of the sysroot).

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me give that a try.

Copy link
Contributor

@honnix honnix Nov 7, 2024

Choose a reason for hiding this comment

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

rctx.os.name for mac is like mac os x, which is not the same as the convention used by target_os :( But otherwise this should be a reasonable approach. We just need to do some formalisation.

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