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

CcToolchainInfo does not fill *_executable and correctly when using action_config #22561

Open
matts1 opened this issue May 28, 2024 · 6 comments
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-CPP Issues for C++ rules type: bug

Comments

@matts1
Copy link
Contributor

matts1 commented May 28, 2024

Description of the bug:

When calling find_cc_toolchain, if you defined a toolchain using action_config instead of tool_path, bazel comes up with an arbitrary path for the *_executable fields. For example, my compiler_executable is bazel/module_extensions/toolchains/cc/gcc (the package that the toolchain is defined in is //bazel/module_extensions/toolchains/cc, but we define a toolchain that uses clang in a different repo rule, and gcc doesn't even exist there, so this is definitely incorrect).

Which category does this issue belong to?

C++ Rules

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

No response

Which operating system are you running Bazel on?

Linux

What is the output of bazel info release?

7.1.1 (also repro'd on last_green (cc4290a1f4790ad07955202c660994f801db0f68)

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse HEAD ?

No response

Is this a regression? If yes, please try to identify the Bazel commit where the bug was introduced.

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

@matts1
Copy link
Contributor Author

matts1 commented May 29, 2024

Note: this affects cargo_build_script rules in rules_rust (source), and probably affects rules_foreign_cc, though I haven't checked that.

@comius
Copy link
Contributor

comius commented May 31, 2024

Do you have a small repro?

@matts1
Copy link
Contributor Author

matts1 commented May 31, 2024

I don't currently have a small repro, I can see if I can come up with one next week.

matts1 added a commit to matts1/rules_rust that referenced this issue Jun 3, 2024
This allows it to work with new-style toolchains that use action configs (see bazelbuild/bazel#22561).
It also allows you to specify a seperate C compiler and C++ compiler.
matts1 added a commit to matts1/rules_rust that referenced this issue Jun 3, 2024
This allows it to work with new-style toolchains that use action configs (see bazelbuild/bazel#22561).
It also allows you to specify a seperate C compiler and C++ compiler.
matts1 added a commit to matts1/rules_rust that referenced this issue Jun 3, 2024
This allows it to work with new-style toolchains that use action configs (see bazelbuild/bazel#22561).
It also allows you to specify a seperate C compiler and C++ compiler.
matts1 added a commit to matts1/rules_rust that referenced this issue Jun 3, 2024
This allows it to work with new-style toolchains that use action configs (see bazelbuild/bazel#22561).
It also allows you to specify a seperate C compiler and C++ compiler.
@matts1
Copy link
Contributor Author

matts1 commented Jun 3, 2024

Upon digging deeper, maybe we should just be deprecating these attributes. They don't make much sense with action configs

  • We have a single cc_toolchain.compiler_executable, but with action configs, we have a separate c and c++ compiler action, each potentially having their own tool.
  • With action configs, the tools can have feature requirements. This means that there is not a single static c compiler binary, but instead it depends on the feature configuration.

I believe that the correct code should be:

feature_configuration = cc_common.configure_features(
    ctx = ctx,
    cc_toolchain = find_cc_toolchain(ctx),
    requested_features = ctx.features,
    unsupported_features = [...],
)

# Optional: Use cc_common.action_is_enabled to check if the following will throw.
c_compiler = cc_common.get_tool_for_action(
    feature_configuration = feature_configuration,
    action_name = ACTION_NAMES.c_compile,
)

matts1 added a commit to matts1/rules_rust that referenced this issue Jun 3, 2024
This allows it to work with new-style toolchains that use action configs (see bazelbuild/bazel#22561).
It also allows you to specify a seperate C compiler and C++ compiler.
matts1 added a commit to matts1/rules_rust that referenced this issue Jun 3, 2024
This allows it to work with new-style toolchains that use action configs (see bazelbuild/bazel#22561).
It also allows you to specify a seperate C compiler and C++ compiler.
github-merge-queue bot pushed a commit to bazelbuild/rules_rust that referenced this issue Jun 3, 2024
This allows it to work with new-style toolchains that use action configs
(see bazelbuild/bazel#22561).

It also allows you to specify a seperate C compiler and C++ compiler.
@comius
Copy link
Contributor

comius commented Jun 14, 2024

I'd agree with that. actions_config seems to be superior to the *_executable attribute.

@comius comius added P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed untriaged labels Jun 14, 2024
@matts1 matts1 closed this as not planned Won't fix, can't repro, duplicate, stale Jun 18, 2024
@matts1 matts1 reopened this Jun 18, 2024
@matts1
Copy link
Contributor Author

matts1 commented Jun 18, 2024

Was going to close this since it didn't seem to be an issue, but just realized I should probably reopen it for the deprecation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-CPP Issues for C++ rules type: bug
Projects
None yet
Development

No branches or pull requests

5 participants