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

Add tool name for objcopy #16158

Closed

Conversation

m3rcuriel
Copy link
Contributor

@m3rcuriel m3rcuriel commented Aug 24, 2022

It is not clear to me why this constant is in src/main/starlark/builtins_bzl/common/cc/action_names.bzl but not in tools/build_defs/cc/action_names.bzl, but it makes get_tool_for_action really annoying.

Furthermore, it seems that if we are willing to define llvm-cov for everyone we should do the same for objcopy.

@google-cla
Copy link

google-cla bot commented Aug 24, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@m3rcuriel m3rcuriel marked this pull request as draft August 24, 2022 05:41
@m3rcuriel m3rcuriel marked this pull request as ready for review August 24, 2022 06:22
@sgowroji sgowroji added team-Rules-CPP Issues for C++ rules awaiting-review PR is awaiting review from an assigned reviewer labels Aug 24, 2022
@oquenchil oquenchil self-assigned this Dec 16, 2022
@oquenchil oquenchil added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Jan 20, 2023
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Jan 23, 2023
hvadehra pushed a commit that referenced this pull request Feb 14, 2023
It is not clear to me why this constant is in [src/main/starlark/builtins_bzl/common/cc/action_names.bzl](https://github.com/bazelbuild/bazel/blob/master/src/main/starlark/builtins_bzl/common/cc/action_names.bzl) but not in [tools/build_defs/cc/action_names.bzl](https://github.com/bazelbuild/bazel/blob/master/tools/build_defs/cc/action_names.bzl), but it makes `get_tool_for_action` really annoying.

Furthermore, it seems that if we are willing to define `llvm-cov` for everyone we should do the same for `objcopy`.

Closes #16158.

PiperOrigin-RevId: 503922270
Change-Id: Ic0803de27150bb6da02a9daf5bb0337ba5ccc17c
@tpudlik
Copy link
Contributor

tpudlik commented Oct 17, 2023

Following up on this, is there a reason we have no action name associated with objdump? That means you can't use get_tool_for_action to get the objdump tool and need to use the apparently-deprecated (per https://bazel.build/configure/integrate-cpp#generate-command-lines-toolchain) tool-specific getter. @comius would you mind if I added it?

@oquenchil
Copy link
Contributor

Is the question related to: #8438 (comment) ?

If it is, it doesn't look like you need the action name in action_names.bzl.

@oquenchil
Copy link
Contributor

In any case it should be fine to add the action name if you want it for the default toolchain.

@comius
Copy link
Contributor

comius commented Oct 18, 2023

I don’t mind if objdump is added

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants