Remove use of -undefined dynamic_lookup on darwin

[keith]

This flag ignores undefined symbols at link time and forces them to be
looked up dynamically. This is incompatible with the newer fixup chains
macho format and theoretically shouldn't be necessary assuming your
dependencies are well defined.

Done in bazel's toolchain here bazelbuild/bazel#16414

[siddharthab]

I considered removing the flag in #177 but decided against it. If you think there is more clarity around this issue compared to then, then we can merge this change now.

[keith]

What made you decide to keep it then? I think given that it has been removed from the default toolchain, it isn't as necessary to keep here. Also in general while I understand there are use cases for it, I think it's a poor default as it pushes things to runtime crashes instead of link time failures, so for folks who need it I think them having to add it explicitly is a bit nicer overall.

[siddharthab]

I kept it because of the ongoing discussion on the GitHub issue I documented next to the code. I did not understand it completely but it looked like there were problems on both sides. I trust your judgement better though, so let's merge it now.

[siddharthab]

Ah and also, I was unable to fix the test failures. Will you be able to look into a few of them? If you can share the general pattern of fixing them, I can fix the rest.

[keith]

I think the failures likely indicate that there are some missing dependencies in rules deps, that probably have to be fixed upstream (which likely also means they'll be broken with a version of bazel removing this is released). I will have to dig in more to know for sure, I will try to do it soon

[siddharthab]

@keith Any suggestions on what we should do here?

[sluongng]

FYI the Github Action log expired.
So I rerun this in a draft PR and pasted the log into a gist
https://gist.github.com/sluongng/7510de54fc356f291e56f371918e395e

This should help future contributors to review and find a solution for this.

[sluongng]

When I was playing around with this, I added -s --sandbox_debug and noticed that the sandbox for linker action was very sparse. Test binaries which was depending on gtest would not have gtest files inside the sandbox

bazel-toolchain/tests> bazel build -s --sandbox_debug @com_google_googletest//:gtest_samples

ERROR: /private/var/tmp/_bazel_sluongng/6883a164badb9d498ab85be8e1483162/external/com_google_googletest/BUILD.bazel:152:11: Linking external/com_google_googletest/libgtest_main.dylib failed: (Exit 1): sandbox-exec failed: error executing command
  (cd /private/var/tmp/_bazel_sluongng/6883a164badb9d498ab85be8e1483162/sandbox/darwin-sandbox/967/execroot/com_grail_bazel_toolchain_tests && \
  exec env - \
    PATH=<long stuff> \
    PWD=/proc/self/cwd \
    TMPDIR=/var/folders/ly/gjsqx9wd0cv1ck7xdslrt6t00000gn/T/ \
  /usr/bin/sandbox-exec -f /private/var/tmp/_bazel_sluongng/6883a164badb9d498ab85be8e1483162/sandbox/darwin-sandbox/967/sandbox.sb /var/tmp/_bazel_sluongng/install/202e556d99f024ca401d77f7be1827ad/process-wrapper '--timeout=0' '--kill_delay=15' '--stats=/private/var/tmp/_bazel_sluongng/6883a164badb9d498ab85be8e1483162/sandbox/darwin-sandbox/967/stats.out' external/llvm_toolchain/bin/cc_wrapper.sh @bazel-out/darwin_arm64-fastbuild/bin/external/com_google_googletest/libgtest_main.dylib-2.params)
Undefined symbols for architecture arm64:
  "testing::InitGoogleMock(int*, char**)", referenced from:
      _main in gmock_main.pic.o
  "testing::UnitTest::GetInstance()", referenced from:
      RUN_ALL_TESTS() in gmock_main.pic.o
  "testing::UnitTest::Run()", referenced from:
      RUN_ALL_TESTS() in gmock_main.pic.o
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
Target @com_google_googletest//:gtest_samples failed to build
Use --verbose_failures to see the command lines of failed build steps.

bazel-toolchain/tests>   (cd /private/var/tmp/_bazel_sluongng/6883a164badb9d498ab85be8e1483162/sandbox/darwin-sandbox/967/execroot/com_grail_bazel_toolchain_tests && \
  exec env - \
    PATH=<really long> \
    PWD=/proc/self/cwd \
    TMPDIR=/var/folders/ly/gjsqx9wd0cv1ck7xdslrt6t00000gn/T/ \
  /usr/bin/sandbox-exec -f /private/var/tmp/_bazel_sluongng/6883a164badb9d498ab85be8e1483162/sandbox/darwin-sandbox/967/sandbox.sb /var/tmp/_bazel_sluongng/install/202e556d99f024ca401d77f7be1827ad/process-wrapper '--timeout=0' '--kill_delay=15' '--stats=/private/var/tmp/_bazel_sluongng/6883a164badb9d498ab85be8e1483162/sandbox/darwin-sandbox/967/stats.out' tree -L 3)

.
|-- bazel-out
|   `-- darwin_arm64-fastbuild
|       `-- bin
`-- external
    |-- bazel_tools
    |   `-- tools
    |-- llvm_toolchain
    |   `-- bin
    `-- llvm_toolchain_llvm
        |-- bin
        `-- lib

12 directories, 0 files

# run "tree bazel-out"
bazel-out
`-- darwin_arm64-fastbuild
    `-- bin
        `-- external
            `-- com_google_googletest
                |-- _objs
                |   `-- gtest_main
                |       `-- gmock_main.pic.o -> /private/var/tmp/_bazel_sluongng/6883a164badb9d498ab85be8e1483162/execroot/com_grail_bazel_toolchain_tests/bazel-out/darwin_arm64-fastbuild/bin/external/com_google_googletest/_objs/gtest_main/gmock_main.pic.o
                `-- libgtest_main.dylib-2.params -> /private/var/tmp/_bazel_sluongng/6883a164badb9d498ab85be8e1483162/execroot/com_grail_bazel_toolchain_tests/bazel-out/darwin_arm64-fastbuild/bin/external/com_google_googletest/libgtest_main.dylib-2.params
                
                
bazel-toolchain/tests> cat /private/var/tmp/_bazel_sluongng/6883a164badb9d498ab85be8e1483162/execroot/com_grail_bazel_toolchain_tests/bazel-out/darwin_arm64-fastbuild/bin/external/com_google_googletest/libgtest_main.dylib-2.params
-shared
-o
bazel-out/darwin_arm64-fastbuild/bin/external/com_google_googletest/libgtest_main.dylib
bazel-out/darwin_arm64-fastbuild/bin/external/com_google_googletest/_objs/gtest_main/gmock_main.pic.o
-Wl,-S
--target=aarch64-apple-macosx
-lm
-no-canonical-prefixes
-headerpad_max_install_names
-L/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/lib
-lc++
-lc++abi
-Lexternal/llvm_toolchain_llvm/lib
--sysroot=/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk

Because of this, the linker would only work with dynamic_lookup.

[sluongng]

wait... Linking external/com_google_googletest/libgtest_main.dylib is dynamic linking. 🤔

[keith]

it's likely that we shouldn't be dynamically linking that

[siddharthab]

Thank you so much for working on this!

[keith]

I think it's actually green for real now! let me add a comment to the dynamic linker feature tho

[keith]

the debian failures look unrelated, i think it's good to be squashed and merged now if you're good with it!

[siddharthab]

Please ignore the Debian test and merge as and when you want.