bazel icon indicating copy to clipboard operation
bazel copied to clipboard

Consider action_config tools for cc_toolchain make variables

Open ChewyGumball opened this issue 8 months ago • 6 comments

This change makes cc_helper.get_toolchain_global_make_variables consider action_configs for tool paths before looking at tool_paths. This allows toolchains that can't or don't specify tool_paths to have make variables such as $CC and $LD specified correctly.

The mapping from make variable to action name is:

$AR -> ACTION_NAMES.cpp_link_static_library $CC -> ACTION_NAMES.c_compile $LD -> ACTION_NAMES.cpp_link_executable $OBJCOPY -> ACTION_NAMES.objcopy_embed_data $STRIP -> ACTION_NAMES.strip

All other toolchain make variables are unchanged either because they don't map to tools, or the mapping wasn't obvious to me. I would be happy to change the mappings if there are better ones.

My understanding is that cc_helper is not exposed publicly, so this is not a public API breaking change. It is possibly a change of output for any existing cc_toolchains, but I believe the toolchains in rules_cc won't change as they either don't specify action_configs for these actions, or they don't specify tool_paths at all and these variables just don't work.

Fixes #25887

ChewyGumball avatar Apr 18 '25 04:04 ChewyGumball

Just a little bump on this one. What are the next steps?

ChewyGumball avatar Apr 29 '25 05:04 ChewyGumball

@armandomontanez for the interaction with rules_cc - this would help to fully migrate to action configs, right?

fmeum avatar May 01 '25 07:05 fmeum

@comius @trybka @pzembrod just another bump on this PR. Is there anything I can do to help move this along?

ChewyGumball avatar May 08 '25 03:05 ChewyGumball

@pzembrod @comius just another bump for review of this PR. I have been using a locally built Bazel with this change for quite a while now with no issue, and I would love to not have to continue using a fork :)

ChewyGumball avatar May 18 '25 00:05 ChewyGumball

Just another friendly bump to try and get this PR moving again.

ChewyGumball avatar Jun 12 '25 02:06 ChewyGumball

@pzembrod Friendly ping

fmeum avatar Jun 12 '25 18:06 fmeum

Just another friendly bump to try and get this PR moving again. Is there something blocking this from being reviewed and/or merged?

ChewyGumball avatar Jun 25 '25 03:06 ChewyGumball

Just another friendly bump to try and get this PR moving again. Is there something blocking this from being reviewed and/or merged?

There's nothing in particular blocking this; working on my backlog now. :-)

pzembrod avatar Jul 04 '25 15:07 pzembrod

I have added a commit that moves the tool names in to constants, and inlines calls to _tool_path() since it is a single line function that doesn't really add more clarity.

ChewyGumball avatar Jul 11 '25 02:07 ChewyGumball

I'm not sure why theres so many failures in CI. It looks like there are C++ errors, and it can't find python? Perhaps there is a temporary issue with CI unrelated to this change?

ChewyGumball avatar Jul 11 '25 03:07 ChewyGumball

I tried the failing tests locally, and they do not fail for me. I think CI just had a blip, but I can't rerun the jobs myself to find out.

ChewyGumball avatar Jul 12 '25 02:07 ChewyGumball

The failure should go away if you rebase onto master.

fmeum avatar Jul 12 '25 15:07 fmeum

Just a friendly bump to try and get this PR going again.

ChewyGumball avatar Aug 14 '25 01:08 ChewyGumball