Extra .exe suffix on windows, possible regression in 0.11.x

[cushon]

I'm updating Bazel's use of rules_graalvm from 0.10.3 to 0.11.1, and from GraalVM 20.0.2 to 21.0.2, and am seeing a failure where it looks like the handling of executable_name had changed on windows:

ERROR: C:/b/ufipaqxb/execroot/_main/_tmp/c7869347f3b0f58b8b3d901d6cb2632e/root/fqvxxemw/external/bazel_tools/tools/java/runfiles/BUILD:12:13: Compiling Java headers external/bazel_tools/tools/java/runfiles/libauto_bazel_repository-hjar.jar (1 source file) failed: missing input file '@@rules_java~~toolchains~remote_java_tools_windows//:java_tools/turbine_direct_graal.exe'

The build logs contain

========================================================================================================================
GraalVM Native Image: Generating 'turbine_direct_graal.exe' (executable)...
========================================================================================================================
...
 - '-H:Name' (alternative API option(s): -o turbine_direct_graal.exe; origin(s): command line)
...
Produced artifacts:
 C:\b\ykmfm4bk\execroot\_main\bazel-out\x64_windows-fastbuild\bin\src\java_tools\buildjar\java\com\google\devtools\build\java\turbine\turbine_direct_graal.exe.exe (executable)

The usage of native_image is configuring a windows-specific path with the .exe suffix. It's overriding the defaults to drop the -bin suffix that's added:

https://github.com/bazelbuild/bazel/blob/4420fffb00794405a334d55c9c5c53669a217f05/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/BUILD#L29-L34

native_image(
    name = "turbine_direct_graal",
    executable_name = select({
        "@bazel_tools//src/conditions:windows": "%target%.exe",
        "//conditions:default": "%target%",
    }),

It looks like an extra suffix is being added somewhere, so it's resulting in .exe.exe on windows.

I wondered if this is related to #217, but from the build logs it looks like native-image is being invoked with -o turbine_direct_graal.exe.

[cushon]

I think I have worked around the windows-only failures I was seeing with

    executable_name = select({
-        "@bazel_tools//src/conditions:windows": "%target%.exe",
+        "@bazel_tools//src/conditions:windows": "%target%",
        "//conditions:default": "%target%",
    }),

I think the default value of the attribute has a similar select, so it still seems like there's an underlying issue here that could affect other users of rules_graalvm:

rules_graalvm/graal/graal.bzl

Lines 48 to 51 in d6bde1f

	executable_name = select({
	"@bazel_tools//src/conditions:windows": "%target%-bin.exe",
	"//conditions:default": "%target%-bin",
	}),

[sgammon]

@cushon Thanks for reporting. I intend to look into this today, a bit swamped but I should be able to get to it quickly

[sgammon]

Hey @cushon,

Firstly, thanks for your patience and workaround. "Today" turned into two weeks before I even noticed.

The current behavior in the rules is to treat executable_name as the name of the bin only, so that the rules can account for the exe extension (as you've noticed). This is something we've inherited from the older rules_graal.

In the original behavior, the binary name was set unconditionally to %target%-bin, which made it difficult to customize and made it impossible to use %target-bin as its own target within the same package. As a backward-compatible change, I added the executable_name attribute and defaulted it to that same pattern.

The original rules_graal never supported Windows (to be fair I think GraalVM did not either when rules_graal was originally written), so when I added Windows support, to preserve the behavior of that attribute, I elected to manage the .exe extension for the user. It is at this point the select was added.

The rule you have cited above (under graal/graal.bzl), is the "compat" entrypoint, which tries to maintain compatibility with the older rules_graal. The newer entrypoint is at graalvm/graalvm.bzl. Subtle, I know, and in the next major version I may consider cleaning that up and merging into just the modern entrypoint.

That being said, the modern entrypoint does this same thing:

rules_graalvm/graalvm/nativeimage/rules.bzl

Lines 28 to 31 in 024d4e3

	_EXEUCTABLE_NAME_CONDITION = select({
	"@bazel_tools//src/conditions:windows": "%target%-bin.exe",
	"//conditions:default": "%target%-bin",
	})

rules_graalvm/graalvm/nativeimage/rules.bzl

Lines 66 to 68 in 024d4e3

	main_class = None,
	executable_name = _EXEUCTABLE_NAME_CONDITION,
	include_resources = None,

I suppose we could:

• Check for an existing .exe postfix and avoid adding one if it is doubled up
  This would be a change in behavior that is technically backwards incompatible, but I don't think anyone wants two .exe extension in their filename, so it's probably fine.

• Stop managing the extension altogether
  We could leave it up to callers, but that would require callsites to be updated and it would break expectations on Windows as they exist currently.

Personally I think approach 1 is fine, but I wanted to get your take first in case I might be missing something. @fmeum if you have any thoughts they are welcome.

[cushon]

Thanks for digging in to this! The approach 1 you described sounds fine to me too.

For what it's worth I'm still slightly confused about where the duplicate .exe is coming from. I would have thought the user-provided executable_name here overrode the default, and for there to be one select(). Is the duplicate .exe definitely happening in rules_graal, and not as a result of the native-image CLI automatically adding the .exe suffix on windows?

[sgammon]

Oh, I see what you mean now. That's a good question, I'll double check.