location "make" variable expansions should avoid external

[DavidANeil]

Description of the problem:

The $(location label) make variable (and both rootpath and execpath) will give the path to an external file through the external directory. When --nolegacy_external_runfiles is enabled then this external directory will not exist, which will lead to a runtime error for the particular executable in most cases.

Change request:

These expansions should resolve to the path without pathing through external. Especially if --nolegacy_external_runfiles is specified.

For example:
WORKSPACE:

workspace(name = "myworkspace")

http_file(
    name = "some_file",
    downloaded_file_path = "filename",
    urls = ["someurl"],
)

The expansion currently resolves as follows:
$(rootpath @some_file//file) => external/some_file/file/filename

It should resolve as:
$(rootpath @some_file//file) => ../some_file/file/filename

This would fix most cases where --nolegacy_external_runfiles cannot be enabled, which is blocking this from being the default (#9306)

Current workarounds:

• Manually replace $(location @some_file//file) with ../some_file/file/filename

• Transform the argument inside of the executable itself, e.g. ${1/#external\//..\/} (Fix jq path in :pin runnable bazel-contrib/rules_jvm_external#454)

Both of these suffer from new breakages often.

[meisterT]

Cc @lberki @jin

[jin]

@gyias

[lberki]

Definitely. Unfortunately, it's a also a Very Incompatible Change so we'll either have to consciously do such a change or live with it.

[DavidANeil]

If we just made --incompatible_disable_legacy_external_runfiles do this location change plus what --nolegacy_external_runfiles currently does, it would be doable, right?

[lberki]

Yep, that sounds like it (although I admit I haven't thought it through very carefully). Whether it's worth the churn is another question.

[lberki]

I'm moving this to P4 because it's a good idea, but it's also very incompatible so we can't accept full requests (without further thought, anyway)

[gyias]

Forgot to mention it on this thread, but 181bd1a has added the behavior.

@lberki Isn't the change enough to close this issue? Or, are there any other concerns?

[lberki]

My understanding is that 181bd1a only applies when --experimental_sibling_repository_layout is in effect. One could make the case that the runfiles path should be the "external-less" one even if that flag is set to false. So let's keep this open.

[github-actions]

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 2+ years. It will be closed in the next 14 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-bazeler". Please reach out to the triage team (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

[github-actions]

This issue has been automatically closed due to inactivity. If you're still interested in pursuing this, please reach out to the triage team (@bazelbuild/triage). Thanks!