go_download_sdk: apply extraction workaround to zips on non-windows OSs

[jayconrod]

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

The Go distribution contains at least one test file with an invalid unicode name. Bazel cannot extract the distribution archive on some operating systems and file systems; Darwin with AFS at least is affected.

For .tar.gz files, we workaround the failure in ctx.download_and_extract by using the native system tar.

This PR applies a similar workaround for .zip files on non-Windows OSs. Windows itself is not affected (ctx.download_and_extract works), so the workaround is not applied there. This is only really needed when you have a Darwin host and a Windows executor (don't ask).

Which issues(s) does this PR fix?

For #2771

Other notes for review

bazelbuild/bazel#16052 might help implement a simpler workaround in the future, but it might also require a minimum bazel version bump.

[jayconrod]

The presubmit failure in Ubuntu 20.04 seems unrelated:

(20:54:22) WARNING: Symlinks in action outputs are not yet supported by --remote_download_minimal, falling back to downloading all action outputs due to output symlink /var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/execroot/io_bazel_rules_go/bazel-out/k8-fastbuild/bin/tests/core/go_path/link_path/src/example.com/repo/cmd/bin/bin.go
(20:54:22) ERROR: /workdir/tests/core/go_path/BUILD.bazel:5:9: output 'tests/core/go_path/link_path/pkg/linux_amd64/github.com/bazelbuild/rules_go/go/tools/bzltestutil.a' is a dangling symbolic link
(20:54:22) ERROR: /workdir/tests/core/go_path/BUILD.bazel:5:9: GoPath tests/core/go_path/link_path/src/example.com/repo/cmd/bin/bin.go failed: not all outputs were created or valid
(20:54:22) ERROR: /workdir/tests/core/go_path/BUILD.bazel:59:8 Middleman _middlemen/tests_Score_Sgo_Upath_Sgo_Upath_Utest_U_Sgo_Upath_Utest-runfiles failed: not all outputs were created or valid

[fmeum]

The workaround isn't free, it requires unzip to be installed on the host.

I think that I have a fix for the underlying Bazel issue and would be able to get it into 6.3.0.

[fmeum]

@jayconrod Could you test whether this is fixed by bazelbuild/bazel#18448?

[jayconrod]

@fmeum That fix works for me. Thanks!

Do you think it's worth continuing this PR to support older versions of Bazel? It should apply to a narrow set of folks (Darwin host, Windows executor), and I think unzip is always pre-installed on Darwin.

Alternatively (perhaps better): I could try applying bazelbuild/bazel#16052 to rename the weird file names into something reasonable. I think they're only used for cmd/go integration testing, so it shouldn't affect the output.

[fmeum]

Given that this fix only applies to a rare use case, I would prefer a solution based on renaming rather than unzip. Maybe we could expose the rename argument so that users can specify their own dict? We can detect the presence of the parameter with bazel_features.

[jayconrod]

Given that this fix only applies to a rare use case, I would prefer a solution based on renaming rather than unzip.

That sounds good. I've updated this PR to use rename_files.

Maybe we could expose the rename argument so that users can specify their own dict?

I think it's better not to expand the API. I can't think of a case where renaming would be useful, other than these two files.

We can detect the presence of the parameter with bazel_features.

Could you guide me a bit on how to do that? I'm not familiar with bazel_features. I found https://github.com/bazel-contrib/bazel_features. I don't think there's a way to detect whether this argument is supported or not except by testing the Bazel version. Empirically, 6.0.0 seems to be the first version that supports it.

Is it safe to reference bazel_features here? I think we couldn't guarantee it would be loaded until after go_rules_dependencies is called, which may be after go_download_sdk. I expect there's a better way with modules, but we can't rely on everyone using those yet.

[fmeum]

I didn't think of the extra complication that comes with this being called before our dependency macro. In that case I think we should just use native.bazel_version directly to check for the correct major version of Bazel.

[jayconrod]

Fixed. I'm using the vendored copy of Skylib to compare the versions. Verified the test passes with 6.2.0 and fails as expected with 5.4.0 on L547, taking the old branch not the new workaround.