search

bazel-contrib / rules_go

Go rules for Bazel
Apache License 2.0
1.38k stars 656 forks source link

fix: properly expand absolute path placeholder for link calls #4144

Open voxeljorge opened 1 day ago

voxeljorge commented 1 day ago

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

PR #4009 added a mechanism that uses a sentinel value as a placeholder for paths which need to be expanded to absolutes for compile operations. These paths end up embedded in .a files and come back to the linker during link operations. The previous PR replaces the CC tool with a pointer to builder cc which in turn substitutes the placeholder value for actual paths. Unfortunately this wasn't done for the linker, so the linker is receiving parameters like --sysroot=__GO_BAZEL_CC_PLACEHOLDER__external/_main~_repo_rules~ubuntu_20_04_sysroot/ which is not valid. When a sysroot is configured this means that internally the linker won't be able to find the sysroot in some cases.

In almost comically bad luck, this only affects the go tool link calls to linkerFlagSupported because for some reason linkerFlagSupported puts the -extldflags at the beginning of the linker command, while the main host link command has them at the end. So this means the linker detects all flags as unsupported but then proceeds to run the linker "correctly" which results in really confusing linker errors. In my case this resulted in the linker trying to run with -rdynamic incorrectly.

Which issues(s) does this PR fix?

Fixes #3886

It could be reasonable to open a bug against go tool link describing the difference between the host link and the flag checks. I suspect it's not really intended behavior and was just sort of happenstance that things ended up that way.

I also really have no idea how to write a test for this. I'm willing to give it a shot but would really appreciate a suggestion as to where/how.

voxeljorge commented 1 day ago

Clearly this naive approach won't work on windows, I guess we could detect the OS write an OS specific script? This all feels pretty messy so i'm hoping someone out there has ideas for a better solution.

voxeljorge commented 6 hours ago

One idea for a different approach would be to use a symlink named something like builder-cc and have the builder inspect os.Argv[0] to set the command.