Fix issue where libtool_check_unique isn't found for sandbox builds #12046

michaeleisel · 2020-09-04T05:05:38Z

It seems that for sandbox builds, the executable isn't copied over. libtool still succeeds, but it prints out an error message about the missing executable. This change, which I just made to mimic wrapped_clang, appears to fix it. I have no idea if there are other changes that should also be made for this, e.g. in tools/osx/crosstool/cc_toolchain_config.bzl wrapped_clang is referenced a lot but libtool_check_unique is not.

michaeleisel · 2020-09-04T05:06:17Z

@trybka @keith FYI

once it looks good, i can run tests with rules_apple

keith · 2020-09-04T05:14:35Z

I'm also not sure if there are other places this is needed. Probably worth grepping or doing a rdeps query on libtool itself, but this seems like a solid fix and likely the only other place

keith · 2020-09-04T05:15:05Z

Maybe worth editing the script to error if this is missing vs treating that as the same type of failure?

michaeleisel · 2020-09-04T12:28:09Z

in a way, i'm glad the script kept working without it. but if we feel confident enough in this change, or at least that doing a simple build with + without the sandbox is enough to test it properly, i can have it fail early

trybka · 2020-09-04T13:35:56Z

There's also https://github.com/bazelbuild/bazel/blob/master/tools/objc/BUILD#L14 ?

(That is referenced in java code here: https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcRuleClasses.java#L531)

Alternatively, unless we're certain that we have all the uses necessary, we could also make the script warn and continue gracefully if it is not present--considering it is just an optimization.

tools/objc/BUILD

michaeleisel · 2020-09-09T22:48:29Z

@trybka @keith updated, does this look good? it passes rules_apple tests for me. i'm not sure if i did the new rule right

michaeleisel · 2020-09-09T22:49:06Z

to be clear, i don't know when that missing rule in tools/objc/BUILD would cause a problem anyways

keith · 2020-09-09T23:09:51Z

tools/osx/crosstool/BUILD.tpl

@@ -50,6 +50,7 @@ cc_toolchain_suite(
            ":builtin_include_directory_paths",
            ":cc_wrapper",
            ":libtool",
+            ":libtool_check_unique",


+1 on this one, seems like this label references the binary in the toolchain setup

keith · 2020-09-09T23:10:08Z

tools/objc/BUILD

    ],
 )

+cc_binary(
+    name = "libtool_check_unique",
+    srcs = ["libtool_check_unique.cc"],


I can't figure out how this stuff is actually used, so I can't tell if it works or not.

Doing some archaeology, I think this was a remnant of the pre-crosstool ObjcCompilation rule implementations. Which is presumably why you can't find it.

I bet this can actually be removed, but I am not knowledgeable enough about the Bazel CI to know if we'd catch any issues.

related #12073

I do think some of it looks more used in j2objc stuff tho

It looks like they pass it along in the J2ObjcAspect, but I can't find any references to it being used after that. Did you catch something else? (We can also take this to your other PR)

EDIT: link

sounds like we can pretty safely ignore this piece for this use case and fix it if anything comes up, so I think this PR should be good to go!

so then i can leave it as-is and we can merge? or does it need to be deleted?

I would delete it for now unless we can prove it is needed otherwise.

Up to whoever can merge, but unless mine merges first I do think this should work, so we could leave it to try and get this in 3.6, and I can remove it if it's really unused.

michaeleisel · 2020-09-10T11:48:11Z

tools/objc/libtool.sh

@@ -37,7 +37,9 @@ WRAPPER="${MY_LOCATION}/xcrunwrapper.sh"
 # Ensure 0 timestamping for hermetic results.
 export ZERO_AR_DATE=1

-if "${MY_LOCATION}"/libtool_check_unique "$@"; then
+if [ ! -f "${MY_LOCATION}"/libtool_check_unique ] ; then


my bash is not fantastic, but this at least works for me

tetromino · 2020-09-11T20:26:13Z

@michaeleisel, thanks for the PR. We'd really like to have a test for what exactly this PR fixes to make sure that it stays fixed. Would you mind either writing a test or providing a small case to reproduce the problem so that we can add a test based on that?

michaeleisel · 2020-09-11T21:28:05Z

@tetromino sure, here's a test case. if we have a folder with the following three files:
BUILD.bazel

objc_library(
    name = "a",
    srcs = ["a.c"],
)

a.c

void a() {
}

WORKSPACE (empty)

if we run bazel build a without this fix, the resulting output for a clean build will include an error message along the lines of

INFO: From Linking liba.a:
external/local_config_cc/libtool: line 40: /private/var/tmp/_bazel_meisel/1054548cf645506d169069fe6cc12477/sandbox/darwin-sandbox/8/execroot/__main__/external/local_config_cc/libtool_check_unique: No such file or directory

e.g., a test could check for libtool_check_unique: No such file or directory not being present

michaeleisel · 2020-09-11T21:34:52Z

@keith to resolve the merge conflict i just undid any changes for that file

keith · 2020-09-11T22:00:46Z

a problem with a test case is it doesn't fail, it just follows the slow path

michaeleisel · 2020-09-15T11:30:47Z

@tetromino curious what your thoughts are

tetromino · 2020-09-18T18:38:55Z

@michaeleisel, I'm really sorry for the delayed response. Unfortunately, I cannot reproduce the problem (the error message you are seeing) using bazel-3.5.0 on my Mac (macOS 10.15.6, Xcode 11.6) and the example you gave. I see the following:

$ cat BUILD.bazel
objc_library(
    name = "a",
    srcs = ["a.c"],
)
$ cat WORKSPACE
$ cat a.c
void a() {
}
$ bazelisk clean --expunge
INFO: Starting clean.
$ bazelisk info
bazel-bin: /private/var/tmp/_bazel_arostovtsev/7a6b4a7b43eb9194232ce1fb3493a8c6/execroot/__main__/bazel-out/darwin-fastbuild/bin
bazel-genfiles: /private/var/tmp/_bazel_arostovtsev/7a6b4a7b43eb9194232ce1fb3493a8c6/execroot/__main__/bazel-out/darwin-fastbuild/bin
bazel-testlogs: /private/var/tmp/_bazel_arostovtsev/7a6b4a7b43eb9194232ce1fb3493a8c6/execroot/__main__/bazel-out/darwin-fastbuild/testlogs
character-encoding: file.encoding = ISO-8859-1, defaultCharset = ISO-8859-1
command_log: /private/var/tmp/_bazel_arostovtsev/7a6b4a7b43eb9194232ce1fb3493a8c6/command.log
committed-heap-size: 100MB
execution_root: /private/var/tmp/_bazel_arostovtsev/7a6b4a7b43eb9194232ce1fb3493a8c6/execroot/__main__
gc-count: 7
gc-time: 167ms
install_base: /var/tmp/_bazel_arostovtsev/install/23356377d522b84e85cc6f4126877e6b
java-home: /private/var/tmp/_bazel_arostovtsev/install/23356377d522b84e85cc6f4126877e6b/embedded_tools/jdk
java-runtime: OpenJDK Runtime Environment (build 11.0.6+10-LTS) by Azul Systems, Inc.
java-vm: OpenJDK 64-Bit Server VM (build 11.0.6+10-LTS, mixed mode) by Azul Systems, Inc.
max-heap-size: 32178MB
output_base: /private/var/tmp/_bazel_arostovtsev/7a6b4a7b43eb9194232ce1fb3493a8c6
output_path: /private/var/tmp/_bazel_arostovtsev/7a6b4a7b43eb9194232ce1fb3493a8c6/execroot/__main__/bazel-out
package_path: %workspace%
release: release 3.5.0
repository_cache: /var/tmp/_bazel_arostovtsev/cache/repos/v1
server_log: /private/var/tmp/_bazel_arostovtsev/7a6b4a7b43eb9194232ce1fb3493a8c6/java.log.arostovtsev-imacpro.arostovtsev.log.java.20200918-142934.33009
server_pid: 33009
used-heap-size: 38MB
workspace: /Users/arostovtsev/bzl/pr12046
$ bazelisk build --genrule_strategy=sandboxed --spawn_strategy=sandboxed a
Starting local Bazel server and connecting to it...
INFO: Analyzed target //:a (19 packages loaded, 173 targets configured).
INFO: Found 1 target...
Target //:a up-to-date:
  bazel-out/apl-darwin_x86_64-fastbuild/bin/liba.a
INFO: Elapsed time: 5.554s, Critical Path: 0.18s
INFO: 2 processes: 2 darwin-sandbox.
INFO: Build completed successfully, 4 total actions
$ grep libtool_check_unique $(bazelisk info server_log) || echo "nothing in the log"
nothing in the log

What version of Bazel are you seeing this in? What build flags are you using (if not --genrule_strategy=sandboxed --spawn_strategy=sandboxed)?

tetromino · 2020-09-18T18:50:54Z

I see - the problem only occurs in Bazel built after d8723b4. I should have read more carefully...

tetromino · 2020-09-18T19:36:59Z

I wrote a small test for this which I'm not entirely happy with (would like to get it reviewed). But for this sort of highly user visible error message, I'm ok with accepting the PR without a test.

Thank you for the test case!

This is a regression test for the issue in #12046 - ensure that in sandboxed mode, we don't get error messages about missing tools from our toolchain.

This is a regression test for the issue in bazelbuild#12046 - ensure that in sandboxed mode, we don't get error messages about missing tools from our toolchain.

meteorcloudy · 2020-09-22T07:42:19Z

Looks like this CL is breaking rules_cc in downstream:
https://buildkite.com/bazel/bazel-auto-sheriff-face-with-cowboy-hat/builds/283

Culprit Finder: https://buildkite.com/bazel/culprit-finder/builds/556

Please take a look

michaeleisel · 2020-09-22T11:46:16Z

@meteorcloudy discussion is at bazelbuild/rules_cc#80

michaeleisel · 2020-09-23T15:17:05Z

based on that discussion, there will be some syncing of rules_cc with this, but no changes are needed in bazel. rules_cc just has to catch up (which will be worked on shortly)

laurentlb · 2020-09-23T16:58:51Z

Can you explain more why this should be cherrypicked in Bazel 3.6?
I'm slightly reluctant because it seems to break rules_cc (which hasn't been updated yet).
Is it a regression? Can this wait one more month?

keith · 2020-09-23T17:25:37Z

The rules_cc fix is here bazelbuild/rules_cc#80

The tl;dr of this change is that the introduction of this optimization here: #11958 didn't work for sandboxed builds, that wasn't noticed because builds still pass, they just take the non-optimized path (test for that here #12136) and without this change it leads to significant log spam for every static archive action:

INFO: From Linking liba.a:
external/local_config_cc/libtool: line 40: /private/var/tmp/_bazel_meisel/1054548cf645506d169069fe6cc12477/sandbox/darwin-sandbox/8/execroot/__main__/external/local_config_cc/libtool_check_unique: No such file or directory

The reason rules_cc broke is because there's no automated path (or even process AFAICT) for updating when there are breaking changes with bazel's crosstool, which seems like something that should also be fixed.

IMO this is worth the cherry pick since it's a very localized change that without will discourage people from using sandboxing even more than they already are because of the performance implications

laurentlb · 2020-10-01T13:27:50Z

@oquenchil What's the status of this?

keith · 2020-10-01T17:42:48Z

@laurentlb note that rules_cc is broken regardless of whether or not this cherry pick is added to 3.6 since that was broken by the first PR, not this one

laurentlb · 2020-10-01T18:54:20Z

Thanks for the precision!
I've made the cherrypick and created the rc, but I'd like this issue to be resolved before I can complete the release.

keith · 2020-10-01T20:04:39Z

I don't see this cherry pick in the list of commits here: #12040 (comment)

It seems that for sandbox builds, the executable isn't copied over. libtool still succeeds, but it prints out an error message about the missing executable. This change, which I just made to mimic wrapped_clang, appears to fix it. I have no idea if there are other changes that should also be made for this, e.g. in tools/osx/crosstool/cc_toolchain_config.bzl wrapped_clang is referenced a lot but libtool_check_unique is not. Closes #12046. PiperOrigin-RevId: 332511362

laurentlb · 2020-10-02T13:50:04Z

Oops, thanks for noticing (I copied the previous list). I've created a new rc.

It seems that for sandbox builds, the executable isn't copied over. libtool still succeeds, but it prints out an error message about the missing executable. This change, which I just made to mimic wrapped_clang, appears to fix it. I have no idea if there are other changes that should also be made for this, e.g. in tools/osx/crosstool/cc_toolchain_config.bzl wrapped_clang is referenced a lot but libtool_check_unique is not. Closes bazelbuild#12046. PiperOrigin-RevId: 332511362

This is a regression test for the issue in bazelbuild#12046 - ensure that in sandboxed mode, we don't get error messages about missing tools from our toolchain.

This is a regression test for the issue in #12046 - ensure that in sandboxed mode, we don't get error messages about missing tools from our toolchain. Should be merged after #12046 is merged (until then, the new test is expected to fail). Closes #12136. PiperOrigin-RevId: 337369029

Add libtool check unique to tools list

0060a4f

googlebot added the cla: yes label Sep 4, 2020

keith approved these changes Sep 4, 2020

View reviewed changes

Add libtool check unique to tools list

b769b89

keith approved these changes Sep 9, 2020

View reviewed changes

keith reviewed Sep 9, 2020

View reviewed changes

tools/objc/BUILD Outdated Show resolved Hide resolved

Add libtool check unique to tools list

b9261d2

michaeleisel force-pushed the me-add-libtool-check-unique-to-tools-list branch from f2ac19c to b9261d2 Compare September 9, 2020 22:47

keith approved these changes Sep 9, 2020

View reviewed changes

keith mentioned this pull request Sep 9, 2020

Remove dead objc setup code #12073

Closed

michaeleisel commented Sep 10, 2020

View reviewed changes

keith mentioned this pull request Sep 11, 2020

Release 3.6 - September 2020 #12040

Closed

10 tasks

Add libtool check unique to tools list

5d27d7f

keith approved these changes Sep 11, 2020

View reviewed changes

tetromino added a commit that referenced this pull request Sep 18, 2020

Add a test for sandboxed cc_library builds.

97d4cfc

This is a regression test for the issue in #12046 - ensure that in sandboxed mode, we don't get error messages about missing tools from our toolchain.

tetromino mentioned this pull request Sep 18, 2020

Add a test for sandboxed cc_library builds on Mac. #12136

Closed

bazel-io closed this in 8b29b6a Sep 18, 2020

oquenchil mentioned this pull request Sep 23, 2020

Update from bazel master? bazelbuild/rules_cc#80

Closed

Fix issue where libtool_check_unique isn't found for sandbox builds #12046

Fix issue where libtool_check_unique isn't found for sandbox builds #12046

Conversation

michaeleisel commented Sep 4, 2020

michaeleisel commented Sep 4, 2020

keith commented Sep 4, 2020

keith commented Sep 4, 2020

michaeleisel commented Sep 4, 2020

trybka commented Sep 4, 2020

michaeleisel commented Sep 9, 2020

michaeleisel commented Sep 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trybka Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tetromino commented Sep 11, 2020

michaeleisel commented Sep 11, 2020

michaeleisel commented Sep 11, 2020

keith commented Sep 11, 2020

michaeleisel commented Sep 15, 2020

tetromino commented Sep 18, 2020 • edited Loading

tetromino commented Sep 18, 2020

tetromino commented Sep 18, 2020

meteorcloudy commented Sep 22, 2020 • edited Loading

michaeleisel commented Sep 22, 2020

michaeleisel commented Sep 23, 2020

laurentlb commented Sep 23, 2020 • edited Loading

keith commented Sep 23, 2020

laurentlb commented Oct 1, 2020 • edited Loading

keith commented Oct 1, 2020

laurentlb commented Oct 1, 2020

keith commented Oct 1, 2020

laurentlb commented Oct 2, 2020

trybka Sep 9, 2020 •

edited

Loading

tetromino commented Sep 18, 2020 •

edited

Loading

meteorcloudy commented Sep 22, 2020 •

edited

Loading

laurentlb commented Sep 23, 2020 •

edited

Loading

laurentlb commented Oct 1, 2020 •

edited

Loading