ctx.attr.tools_deps and ctx.attr.additional_tools is redundant - rename to a single ctx.attr.tools?

[jsharpe]

There is no real distinction between ctx.attr.tools_deps and ctx.attr.additional_tools. Most other rules canonically use ctx.attr.tools so I am proposing that we should deprecate the former two and move to the latter for tools used within the build.

[whs-dot-hk]

Please, don't... first you will break api... second they are really different look at the https://github.com/bazelbuild/rules_foreign_cc/blob/7c4490a2ee289fae3ce78e647772fd0311006cdc/tools/build_defs/framework.bzl#L531

[jsharpe]

My proposal is to provide a transition path by marking the existing attributes as deprecated and providing a path to transition to using tools only. We could provide a buildozer command to aid the migration but these attributes don't seem widely used at least in the open source uses from what I can see from searching GitHub and the fact that until recently (PR #423 ) the code for handling the additional_tools attribute was broken on recent versions of bazel.

I don't follow what is different from the link you provided? Could you expand on this a bit so I can understand where you think the difference is between these two attributes.

Note that currently ctx.attr.additional_tools is configured for target rather than exec/host which I think is a bug as anything passed into additional_tools can't be run portably in the context of platform support. Also anything that could be passed here intended for target could equally be passed to ctx.attr.additional_inputs. I think the original intention may have been for one of the two attributes to be the public API and the other an internal 'framework' api for specific rules to add tools that they need.

[attilaolah]

+1 for renaming this to tools and redirecting both existing attrs with a deprecation warning (but also fix wrt cfg = "exec", or cfg = "host" until #491 is fixed).

People have to pin Bazel workspaces to commits anyway, and providing support with the deprecation warning for ~6m seems reasonable, esp. given that there hasn't even been any official release tag for this repo. (Well, since the 0.0.8 which seems to have been around forever.)

[whs-dot-hk]

additional_tools do not go into PATH, while tools_deps does

[jsharpe]

additional_tools do not go into PATH, while tools_deps does

Do you have a use case where this behaviour is required?

[whs-dot-hk]

It agrees with the docs "Not used by the shell script part..."

[UebelAndre]

Do you have a use case where this behaviour is required?

There may be projects that are built assuming something is installed on the machine and a particular program is available via the PATH variable. I think in doing this we'll have to add an additional flag for controlling what the PATH looks like during a build. Not sure what best representation of that is.

[jsharpe]

What PATH looks like is either action_env or can be explicitly set using the env attribute?

[UebelAndre]

That sounds good to me (and what I was thinking as well).

[jsharpe]

Just to update this issue - we now have build_data which is in the exec configuration and so tools can be passed via this attribute but we currently only have data that is a non-deprecated attribute to pass a dep that is in target configuration. (deps is only for targets with a CcInfo provider)