Use symlink instead of changing output path

[mering]

What type of PR is this?

Feature

What does this PR do? Why is it needed?

Always write the binary to the mode specific directory. Create a symlink when out is specified. This reduces cache invalidation.

The output path is determined via

rules_go/go/private/actions/binary.bzl

Line 55 in cd29704

executable = go.declare_file(go, path = name, ext = extension)

rules_go/go/private/context.bzl

Lines 147 to 161 in cd29704

	def _child_name(go, path, ext, name):
	if not name:
	name = go.label.name
	if path or not ext:
	# The '_' avoids collisions with another file matching the label name.
	# For example, hello and hello/testmain.go.
	name += "_"
	if path:
	name += "/" + path
	if ext:
	name += ext
	return name
	def _declare_file(go, path = "", ext = "", name = ""):
	return go.actions.declare_file(_child_name(go, path, ext, name))

This means for a go_binary target //path/to:target it would be path/to/target_/target instead of the exptected /path/to/target. This behavior was introduced in #2466.
In most of the cases, the old behavior is expected (most rules create a file matching the rule name). Only when using Gazelle where one doesn't have direct control, this can cause problems. This change introduces an additional attribute to explicitly opt back into having the binary available at the expected path by creating a symlink without duplicating the target name as out.

In the future this could be explicitly set to False by Gazelle and then default to True to align better with user expectations while still working around issues with auto-generated targets.

Which issues(s) does this PR fix?

Other notes for review

[fmeum]

I generally like this change, I just want to make sure we get the defaults right without breaking anyone too much.

Only when using Gazelle where one doesn't have direct control, this can cause problems.

Could you explain what kind of problems this could cause?

then default to True to align better with user expectations while still working around issues with auto-generated targets.

I wonder whether we could unconditionally create the symlink, but when out_auto == False we only add it to DefaultInfo(files = ...) without passing it into executable. This would immediately allow consumers to discover the binary in bazel-bin or in runfiles under the more friendly path without changing the behavior for existing users. What do you think?

CC @jayconrod

[mering]

I generally like this change, I just want to make sure we get the defaults right without breaking anyone too much.

Only when explicitly setting out, the result changes from the binary being at that path to a symlink being at that path.

Only when using Gazelle where one doesn't have direct control, this can cause problems.

Could you explain what kind of problems this could cause?

I only tried to reconstruct past issues starting from the linked #2466 which introduced the current behavior.

then default to True to align better with user expectations while still working around issues with auto-generated targets.

I wonder whether we could unconditionally create the symlink, but when out_auto == False we only add it to DefaultInfo(files = ...) without passing it into executable. This would immediately allow consumers to discover the binary in bazel-bin or in runfiles under the more friendly path without changing the behavior for existing users. What do you think?

Unconditionally creating the symlink would be my preferred approach.

I am not sure if I fully understand what you are proposing, but when I tried to always create the symlink without introducing out_auto in a previous version of this PR, there was a test failure with a conflict between //tests/core/go_binary:prefix and //tests/core/go_binary/prefix:prefix.

[jayconrod]

@mering Could you please create an issue describing what you want to do? It would be better to discuss the change in behavior before we get to the implementation.

Some concerns:

• Symbolic links aren't well supported on Windows. You need to be an administrator to enable them, and I'm not sure if Bazel will create a real symbolic link here or a junction. There are several kinds of link, and different software interprets them differently.
• Something that expects a real binary instead of a symbolic link may break. go_binary typically produces a single binary file, so it would be easy for some dependent rule to pick up the link without the binary it points to.
• I'm not clear on the cache invalidation concern. Currently, if you change out or basename, it triggers a relink for that target, but I wouldn't expect that to happen very often. What am I missing?
• I'd rather not introduce a new out_auto attribute. out and basename are already kind of redundant, so this makes the situation more confusing.

I wonder if a better solution for #2463 would have been for Gazelle to set out or basename explicitly when there's a subdirectory with the same name. Then go_binary by default could produce an executable with the same name as the target.

[mering]

@mering Could you please create an issue describing what you want to do? It would be better to discuss the change in behavior before we get to the implementation.

Created #4350.

• Symbolic links aren't well supported on Windows. You need to be an administrator to enable them, and I'm not sure if Bazel will create a real symbolic link here or a junction. There are several kinds of link, and different software interprets them differently.

I know that for runfiles and for pkg_tar, ctx.action.symlink targets are resolved and the destination binary used directly.
So this should only change the behavior of bazel-out which at least on Linux already creates symlinks most of the time anyways.

• Something that expects a real binary instead of a symbolic link may break. go_binary typically produces a single binary file, so it would be easy for some dependent rule to pick up the link without the binary it points to.

IIUC bazel rules should correctly resolve the symlinks.

• I'm not clear on the cache invalidation concern. Currently, if you change out or basename, it triggers a relink for that target, but I wouldn't expect that to happen very often. What am I missing?

This is referring to the warning in the previous docstring of the out attribute.

• I'd rather not introduce a new out_auto attribute. out and basename are already kind of redundant, so this makes the situation more confusing.

I would like to avoid having to specify the value of name twice for every rule. The out_auto could easily be applied to the whole code base via buildozer but copying values is not supported AFAIK.

I wonder if a better solution for #2463 would have been for Gazelle to set out or basename explicitly when there's a subdirectory with the same name. Then go_binary by default could produce an executable with the same name as the target.

Yes, go_binary should by default produce an executable with the same name as the target. It makes much more sense to work around the edge cases where they appear instead of changing the default.

[jayconrod]

Thanks for creating #4350, let's move discussion there.