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

fix: search for the sysroot in compile args #313

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lukasoyen
Copy link

Wit the new cc_rules based toolchains, the sysroot on the toolchain is not set. But we can infer it from the command line args a normal compilation with that toolchain would get.

Wit the new `cc_rules` based toolchains, the `sysroot` on the
toolchain is not set. But we can infer it from the command line
args a normal compilation with that toolchain would get.
@@ -243,7 +274,7 @@ def _create_common(ctx):

merged_cc_info = cc_common.merge_cc_infos(cc_infos = [dep[CcInfo] for dep in all_cc_deps])

cc_toolchain = find_cpp_toolchain(ctx)
sysroot = _get_sysroot(ctx)
Copy link
Author

Choose a reason for hiding this comment

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

I am quite sure that this is the wrong place and should be moved into _impl of cuda_toolchain_config. Currently this get_memory_inefficient_command_line() is called for every? cuda_compile() action.

Moving that would require plumbing through a new sysroot attribute and possibly the sysroot feature in cuda_toolchain_config has never been tested. So I hope I can leave that improvement for someone else.

@cloudhan
Copy link
Collaborator

@lukasoyen Do you know anyway to query the info from cc_toolchain directly. This creates an unused args on each action run. And is quite wasteful... Better way should fuse it into the toolchain configuration.

@cloudhan
Copy link
Collaborator

Seems to be designed to be accessible from cc_toolchain directly

https://github.com/bazelbuild/rules_cc/blob/6be85c266b1df3a5bd38290c814d83ee643185dd/cc/private/rules_impl/cc_flags_supplier_lib.bzl#L57-L62

Then we can attach it to our cuda_info then add a corresponding feature for it.

@lukasoyen
Copy link
Author

Seems to be designed to be accessible from cc_toolchain directly

I read through that code while figuring things out. I think that is different and not meant for the rule based toolchains.

  • This is the corresponding code for the rule based toolchains. It (and the old variant) use CC_FLAGS_MAKE_VARIABLE_ACTION_NAME
  • CC_FLAGS_MAKE_VARIABLE_ACTION_NAME is not used in cc_sysroot
  • cc_toolchain_config does not set builtin_sysroot

I think the rule based toolchains are designed in a way to make the builtin_sysroot obsolete.

@lukasoyen
Copy link
Author

Do you know anyway to query the info from cc_toolchain directly.

As said in #313 (comment) I agree this is wasteful and slow. I don't see a reason to put the detection into the toolchain config. The CC toolchain is an attribute already at the detection already queries the CC toolchain for the compiler binary.

I am just not sure how all that plumbing would turn out. But I am up to try if you prefer it that way.

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.

2 participants