Fix dynamic library lookup with remotely executed tools

[fmeum]

When a cc_binary tool is executed directly (e.g. in a genrule) with
remote execution, its dynamic dependencies are only available from the
solib directory in the runfiles directory. This commit adds an
additional rpath entry for this directory.

Since target names may contain commas and the newly added rpaths contain
target names, the -Xlinker compiler is now used everywhere instead of
the -Wl flag, which doesn't support commas in linker arguments.

[fmeum]

@EdSchouten This is essentially your #14600 with an Integration test and support for commas. Could you verify that I got this right based on the changes in #16214?

@oquenchil Could you review after #16214?

[EdSchouten]

Thanks for addressing this!

[fmeum]

@lberki This is the follow-up to #16214 that handles the remaining cases (including commas and --experimental_sibling_repository_layout) by switching to -Xlinkereverywhere. I don't think that-Wl` provides an escape character to escape commas with, so I found this to be the only way.

[oquenchil]

LGTM

[fmeum]

@oquenchil @sgowroji This should be merged separately after #16214 so that the former can be cherry-picked into 5.3.1.

[fmeum]

@sgowroji You can go ahead with the merge, #16214 has been merged and I rebased the PR.

[sgowroji]

@fmeum Thank you for very quick changes! Can you please fix the build kite failures.

[fmeum]

@sgowroji Pretty sure those were flakes. I restarted the pipeline.

[lberki]

@meteorcloudy I'll add this to the list of issues for the 6.0 milestone to be considered as a requirement for 6.0 . I personally don't think this should block 6.0 (it's been delayed enough), but it's useful to keep track of the "nice to haves" for that release.

[buildbreaker2021]

@fmeum The change from our side is not straightforward.

So our change which is required for the fix is instant(does not require a release to be picked up).
However the rest of the changes in the PR require a release.
Also the fix which I have in mind cannot work without the changes in the PR.

To sum it up:

1. We release the fix with the PR - fix is picked up instantly the PR is not, so the fix without PR is breaking tests.
2. We release fix first PR second - same problem as in 1.
3. We release PR first and the fix second - this is the reason for the current rollback.

I am pretty sure there is a way around this, it is not just straightforward.

[buildbreaker2021]

The solution which I currently have in mind, would require at least 1-2 weeks, depending on the internal Bazel release time.

[fmeum]

@buildbreaker2021 In case that would help, I'm happy to make my PR public domain (or whatever is needed) and have you integrate it in your fix CL so that you can merge both simultaneously without going through the import step.

[buildbreaker2021]

I will ping the PR once we are ready for the merge - import step is fine from my side if you are fine with it.

[buildbreaker2021]

Hey @fmeum ,

During further testing I discovered that my fix(even though it fixed current issue) breaks something else.
So it would require more time commitment from my side compared to what I initially expected.

To unblock you and to keep the things moving I suggest the following solution:

1. Could you add an incompatible flag like this: https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java#L62-L69 ?
   Maybe that is not a right file for the flag to live in, this is just an example. So the flag will serve as a switch between your PR behavior vs old behavior. This flag will also make it easier to fix breakages from our side.
2. You can set the flag to True, so that changes in this PR are picked up by Bazel, and assuming it does not break anyone, it should be fine. I will flip the flag to False internally so that we still use the old implementation until I find a proper fix.

[fmeum]

@buildbreaker2021 I pushed a new commit that gates the change behind a flag that defaults to true.

[fmeum]

@buildbreaker2021 I forgot to carry the new flag over in getHost(). I force-pushed a new version, please import that one.

[buildbreaker2021]

A quick update.

After seeing @fmeum 's latest commit I realized that going in a flag direction would be quite tedious, since on top of the flag we would need to add the logic which would split implementation in two branches etc.
I found some time on Friday and I think I fixed the underlying issue without adding the flag. So I rolled forward the initial PR without a flag - let's hope it sticks this time.

[meteorcloudy]

I'm cleaning up the 6.0.0 nice to have label, do we still want to cherry pick this change into the next 6.x release? If so, feel free to mark it!

[fmeum]

This was merged before the baseline cut, so it's already in 6.0.0.

[meteorcloudy]

Nice!