Declare non-windows executable with .sh extension

[wiwa]

Description

Add an extension for the executable runfile in non-Windows OS (.sh) to prevent an issue with ArtifactPrefixConflictException.

When foo:bar is a binary (or test), it will generate a runfile artifact, <bazel-out>/.../foo/bar.
If we also have a subdirectory foo/bar with targets, then there will be a conflict since those targets (e.g. foo/bar:baz) will need the <bazel-out>/.../foo/bar output path, but that is already taken by our first artifact, resulting in Bazel erroring out with com.google.devtools.build.lib.skyframe.ArtifactConflictFinder$ConflictException: com.google.devtools.build.lib.actions.ArtifactPrefixConflictException.

Motivation

Remove conflicts between runfile executables and directory names.

[wisechengyi]

If it's repro'able here, would recommend adding a test case. but might be good to discuss the idea first to see if it would break any other use cases.

[wiwa]

@wisechengyi added a test, thanks

[wiwa]

@simuons could you take a look? thanks!

[wiwa]

@simuons @liucijus ?

[liucijus]

Merging, as @simuons is away

[ittaiz]

This was a breaking change.
I'm not sure breaking changes are a great idea.
An alternative could have been to add a flag for this.
I know flags are yet more code and cognitive load but OTOH an innocent upgrade just broke an internal feature.
I'll deep dive now to understand the reason but my suspicion is that IJ makes some assumptions based on how java works and now that we don't work like java the feature breaks.

Which sort of leads me to another issue and that is the divergence from how java rules work...

[ittaiz]

This is the issue https://github.com/bazelbuild/intellij/blob/a5522a0f1787b667a3a966e927da773cb4ec51a2/java/src/com/google/idea/blaze/java/run/fastbuild/FastBuildTestEnvironmentCreator.java#L90
Now whether IJ will change or not I don't know...

[wiwa]

I'm not sure the line you linked is the actual issue. That line finds the <target>.runfiles directory, not the <target>.sh script. We don't change the directory name. The <target>.sh script runs the command line for the binary; the linked line(s) seem to replicate (much of) its implementation.

I agree there is likely some assumption on how Java binaries are supposed to be run by the IntelliJ Bazel plugin. We can make an issue to have a flag for this feature?

[ittaiz]

There is no <target>.sh script concept in bazel.
There is target-name of a binary rule which is by convention the executable and target-name.runfiles which is the runfiles of the executable.
Because this PR changed the executable to be target-name.sh then Bazel now generates target-name.sh.runfiles.
That is not an opinion it's a fact I've verified locally.

[ittaiz]

I think we should:

1. Revert.
2. Add this again as opt-in (meaning defaults to previous behavior)
3. Open bazel side issue to discuss the problem you saw and maybe java will also tackle it

[wiwa]

Thanks for the clarification (and debugging), that sounds entirely reasonable!

[ittaiz]

@liucijus @simuons @wiwa how do we progress from here?

[liucijus]

I think we should:

1. Revert.

2. Add this again as opt-in (meaning defaults to previous behavior)

3. Open bazel side issue to discuss the problem you saw and maybe java will also tackle it

@ittaiz could you open a reverting PR with an explanation what has been broken?
@wiwa could you take 2 & 3?

[ittaiz]

Sure

[wiwa]

Will do! Sorry, been a crazy few days 😅

[ittaiz]

Without a doubt...
Hope you're hanging in 🙏🏾
Is it ok if I do the revert PR?

[wiwa]

Yep, revert away.