Run libtool without symlinking if possible #11958

michaeleisel · 2020-08-17T13:55:14Z

If there are no duplicate .o basenames, then libtool can be invoked directly without symlinking. From #11846

michaeleisel · 2020-08-17T13:55:40Z

@trybka could you take a look? Thanks!

tools/objc/libtool_check_unique.cc

tools/objc/libtool.sh

michaeleisel · 2020-08-17T13:57:42Z

@trybka is there a way that libtool is currently tested? i didn't see any test rules in that dir

tools/objc/BUILD

tools/objc/libtool.sh

tools/objc/libtool_check_unique.cc

michaeleisel · 2020-08-17T18:58:57Z

@keith @kastiglione @trybka responded, curious about toolchain setup as per keith's comment and test practices

trybka · 2020-08-17T19:06:55Z

re: testing

I think your best bet would be to check out the rules_apple repo and run their integration tests:
https://github.com/bazelbuild/rules_apple/tree/master/test

They are quite robust, and would likely identify any issues in both toolchain setup / libtool usage.

re: toolchain setup

I suspect you will have to do something like we do for wrapped_clang.cc (see the link @keith mentioned).
Otherwise you have a dependency loop: you can't use the toolchain you're trying to build prerequisites for to build the prerequisites.

michaeleisel · 2020-08-17T21:23:40Z

@keith that line is added now for the crosstool. how can libtool.sh get the path to the executable for the c++? (currently it just uses dirname of the script itself and appends the executable name for testing purposes)

keith · 2020-08-17T21:36:22Z

If something like that works that is ok, other scripts rely on something similar

michaeleisel · 2020-08-17T21:42:14Z

it does not work afaict :P it just helped me test, but if there's no standard way to do this i can dig a bit

michaeleisel · 2020-08-17T22:38:21Z

@keith it seems like it would work if it was a script, like the python script, but not since it's c++. i see the wrapped_clang test uses $(rlocation ...) but that doesn't seem to work, so i guess make variable substitution isn't done here? i'm not sure how to get the location of the binary. i also see that there's mention of wrapped_clang in the rules_cc repo, so i'm curious if i have to make changes there as well.

michaeleisel · 2020-08-18T19:20:02Z

got it now, seems to work. @trybka i tried the rules_apple tests, but i get 52 of 289 tests failed on master as well as with this pr. they fail with could not get the user's cache directory: $HOME is not defined

trybka · 2020-08-18T19:40:15Z

@allevato have you seen that failure-mode before?

allevato · 2020-08-18T19:54:11Z

The only other instance I can find of it is here, but it's not clear what resolved it: bazelbuild/tulsi#106

If they fail on master as well as this PR, then there might be an unrelated problem on the Buildkite CI; @philwo may be able to assist or pull in someone who can.

trybka · 2020-08-18T23:20:20Z

OK, all the tests are passing for me locally, I'll patch your commit in and try to test it as well.

michaeleisel · 2020-08-18T23:35:06Z

Awesome, thank you! I'll make an issue for this local test issue

trybka · 2020-08-19T00:43:11Z

err... once I figure out how to patch your changes in. >_> I think this is a problem for tomorrow @trybka

michaeleisel · 2020-08-19T12:38:59Z

tools/objc/libtool_check_unique.cc

+int main(int argc, const char * argv[]) {
+    unordered_set<string> basenames;
+    const regex libRegex = regex(".*\\.a$");
+    const regex noArgFlags = regex("-static|-s|-a|-c|-L|-T|-no_warning_for_no_symbols");


michaeleisel · 2020-08-19T12:39:58Z

thanks to @keith i can run rules_apple tests now. they succeed on the latest commit, ad7a8bd

trybka

Looking good. Glad you got the tests running!

trybka · 2020-08-19T17:50:28Z

tools/cpp/osx_cc_configure.bzl

+            paths["@bazel_tools//tools/objc:libtool_check_unique.cc"],
+        ))
+        xcrun_result = repository_ctx.execute([
+            "env",


pull these into a common copts list or something so they can be shared with wrapped_clang?

michaeleisel · 2020-08-19T21:54:11Z

@trybka updated

trybka

Do you have any benchmarks or data on the speed improvement for some of your projects?

trybka · 2020-08-20T15:10:55Z

tools/objc/libtool.sh

+# Ensure 0 timestamping for hermetic results.
+export ZERO_AR_DATE=1
+
+if $(/usr/bin/dirname "$0")/libtool_check_unique "$@"; then


Sorry I missed this before--is it possible to refer to this relative to $MY_LOCATION?

Given that this script may be invoked in multiple contexts (according to the logic above), it might be prudent to refer to this in a canonical way.

michaeleisel · 2020-08-20T15:41:44Z

for a 700-file swift_library target, libtool_check_unique takes about 1ms. the overall time seems to be about the same between calling /usr/bin/libtool directly and doing this

michaeleisel · 2020-08-20T15:55:16Z

@trybka responded

trybka · 2020-08-20T16:06:02Z

for a 700-file swift_library target, libtool_check_unique takes about 1ms. the overall time seems to be about the same between calling /usr/bin/libtool directly and doing this

Cool. How much slower was it before? (with the always hash-symlinking?)

trybka · 2020-08-20T16:10:29Z

for a 700-file swift_library target, libtool_check_unique takes about 1ms. the overall time seems to be about the same between calling /usr/bin/libtool directly and doing this

Cool. How much slower was it before? (with the always hash-symlinking?)

Nevermind, it's in the linked issue.

trybka · 2020-08-21T18:49:04Z

OK, just some minutiae:
Please see https://bazel.build/basics/patching.html (notably, make sure you've signed the agreement here: https://cla.developers.google.com)

and could you add the license boilerplate to the .cc file? I couldn't find a pointer to it in the docs, but our presubmit nags about it.

// Copyright 2020 The Bazel Authors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//    http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

FWIW, we filed bazelbuild/bazel-website#267 to address the doc issue about this requirement.

michaeleisel · 2020-08-21T23:23:51Z

@trybka updated with the boilerplate. And I signed the CLA earlier, does that apply here? bazelbuild/rules_swift#439 (comment)

trybka · 2020-08-24T18:12:21Z

Yeah, I think that should be good. Re-running the import script now.

If there are no duplicate .o basenames, then libtool can be invoked directly without symlinking. From bazelbuild#11846 Closes bazelbuild#11958. PiperOrigin-RevId: 328185225

…ctor With the bazel version update from 3.1.0 to 3.7.2, the object file hash values are no longer added when their names are unique within an objc_library. See: * bazelbuild/bazel#11846 * bazelbuild/bazel#11958 However, object name collision can still happen when there is a source code with the same basename in a transitive dependency tree. Normally there is no problem with this, but this had an unfortunate interaction with the symbol hiding script we use for building the iOS static framework. That is, when an archive file (.a) contains more than one object file with the same name, the 'ar -x' command executed as part of the symbol hiding script would overwrite the conflicting object file, causing the overwritten object file to be not included in the final static framework. This was causing a link error at CocoaPods lint step, as it had some missing function definitions. This is now fixed by using a custom extraction script written in Python, which gracefully handles the object file name collision. This also fixes a previously existed race condition when the symbol hiding script is run in parallel for multiple static framework targets, by using separate temporary directories while extracting the object files. Verified that this fix works with CocoaPods lint. PiperOrigin-RevId: 351118550 Change-Id: Iec26e4720c21f271822785032d5fb6f4717eebca

michaeleisel added 4 commits August 17, 2020 08:58

Faster libtool

0fda2cb

Merge remote-tracking branch 'u/master' into me-faster-libtool

abd48e2

Faster libtool

ef6c5e8

Faster libtool

fad384c

googlebot added the cla: yes label Aug 17, 2020

michaeleisel commented Aug 17, 2020

View reviewed changes

tools/objc/libtool_check_unique.cc Show resolved Hide resolved

michaeleisel commented Aug 17, 2020

View reviewed changes

tools/objc/libtool.sh Show resolved Hide resolved

michaeleisel mentioned this pull request Aug 17, 2020

The OSX libtool wrapper increases incremental build times #11846

Closed

gregestren added the z-team-Apple Deprecated. Send to rules_apple, or label team-Rules-CPP + platform:apple label Aug 17, 2020

keith reviewed Aug 17, 2020

View reviewed changes

tools/objc/BUILD Outdated Show resolved Hide resolved

tools/objc/libtool.sh Outdated Show resolved Hide resolved

kastiglione reviewed Aug 17, 2020

View reviewed changes

tools/objc/libtool_check_unique.cc Outdated Show resolved Hide resolved

tools/objc/libtool_check_unique.cc Outdated Show resolved Hide resolved

tools/objc/libtool_check_unique.cc Outdated Show resolved Hide resolved

pr responses

f790ad3

Faster libtool

41b78a7

michaeleisel added 2 commits August 18, 2020 15:13

Faster libtool

c688997

Faster libtool

ad7a281

thii reviewed Aug 19, 2020

View reviewed changes

michaeleisel added 2 commits August 19, 2020 08:15

Merge remote-tracking branch 'u/master' into me-faster-libtool

a1ff612

Faster libtool

ad7a8bd

trybka reviewed Aug 19, 2020

View reviewed changes

Faster libtool

a6237ca

trybka requested changes Aug 20, 2020

View reviewed changes

Faster libtool

c1a21b8

Faster libtool

350c682

bazel-io closed this in d8723b4 Aug 24, 2020

keith mentioned this pull request Sep 23, 2020

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run libtool without symlinking if possible #11958

Run libtool without symlinking if possible #11958

michaeleisel commented Aug 17, 2020

michaeleisel commented Aug 17, 2020

michaeleisel commented Aug 17, 2020

michaeleisel commented Aug 17, 2020

trybka commented Aug 17, 2020

michaeleisel commented Aug 17, 2020

keith commented Aug 17, 2020

michaeleisel commented Aug 17, 2020

michaeleisel commented Aug 17, 2020

michaeleisel commented Aug 18, 2020

trybka commented Aug 18, 2020

allevato commented Aug 18, 2020

trybka commented Aug 18, 2020

michaeleisel commented Aug 18, 2020

trybka commented Aug 19, 2020

This comment was marked as resolved.

michaeleisel Aug 19, 2020

michaeleisel commented Aug 19, 2020

trybka left a comment

trybka Aug 19, 2020

michaeleisel Aug 19, 2020

michaeleisel commented Aug 19, 2020

trybka left a comment

trybka Aug 20, 2020

michaeleisel Aug 20, 2020

michaeleisel commented Aug 20, 2020

michaeleisel commented Aug 20, 2020

trybka commented Aug 20, 2020

trybka commented Aug 20, 2020

trybka commented Aug 21, 2020

michaeleisel commented Aug 21, 2020

trybka commented Aug 24, 2020

Run libtool without symlinking if possible #11958

Run libtool without symlinking if possible #11958

Conversation

michaeleisel commented Aug 17, 2020

michaeleisel commented Aug 17, 2020

michaeleisel commented Aug 17, 2020

michaeleisel commented Aug 17, 2020

trybka commented Aug 17, 2020

michaeleisel commented Aug 17, 2020

keith commented Aug 17, 2020

michaeleisel commented Aug 17, 2020

michaeleisel commented Aug 17, 2020

michaeleisel commented Aug 18, 2020

trybka commented Aug 18, 2020

allevato commented Aug 18, 2020

trybka commented Aug 18, 2020

michaeleisel commented Aug 18, 2020

trybka commented Aug 19, 2020

This comment was marked as resolved.

michaeleisel Aug 19, 2020

Choose a reason for hiding this comment

michaeleisel commented Aug 19, 2020

trybka left a comment

Choose a reason for hiding this comment

trybka Aug 19, 2020

Choose a reason for hiding this comment

michaeleisel Aug 19, 2020

Choose a reason for hiding this comment

michaeleisel commented Aug 19, 2020

trybka left a comment

Choose a reason for hiding this comment

trybka Aug 20, 2020

Choose a reason for hiding this comment

michaeleisel Aug 20, 2020

Choose a reason for hiding this comment

michaeleisel commented Aug 20, 2020

michaeleisel commented Aug 20, 2020

trybka commented Aug 20, 2020

trybka commented Aug 20, 2020

trybka commented Aug 21, 2020

michaeleisel commented Aug 21, 2020

trybka commented Aug 24, 2020