Set ZERO_AR_DATE in bazel_xcode_wrapper

[ob]

This is for hermeticity since libtool writes the date in the headers unless this is set.

While using these rules with the remote cache, I found instances where Swift produced libraries were different in the date only.

[sergiocampama]

instead of adding this to the wrapper script, this should be set only in actions that invoke libtool, so here you should add the following to the run_toolchain_action call:

env = {"ZERO_AR_DATE": "1"},

edit: by "here" I meant here

(copied the wrong link)

[ob]

@sergiocampama and ar too then? Does swiftc invoke ar or libtool internally?

[allevato]

It only needs to be added to the archiving actions in archiving.bzl. We never invoke swiftc in a way that would cause the driver to spawn an archiving job.

[keith]

This was discussed a bit offline but should we be calling the libtool wrapper from bazel instead of libtool directly here? It does already solve this issue:

https://github.com/bazelbuild/bazel/blob/02b62daa60bb582c3c0eaa10b0e0f8bf93e29d56/tools/objc/libtool.sh

I don't see an equivalent wrapper for ar though

[steeve]

@keith do you think it should be included in this repo or referenced via @bazel_tools ?

[steeve]

I was thinking of adding it SwiftToolchainInfo via xcode_swift_toolchain but I guess it would require modifying SwiftToolchainInfo.

        "_libtool": attr.label(
            cfg = "host",
            default = Label(
                "@bazel_tools//tools/objc:libtool",
            ),
            executable = True,
        ),

[steeve]

Hum apparently it's not as simple as using it directly.

The tests fails with:

python: can't open file '/private/var/tmp/_bazel_steeve/3be5cc5199d6a2d5d595a2e3e8e90cde/sandbox/darwin-sandbox/1/execroot/build_bazel_rules_swift/bazel-out/host/bin/external/bazel_tools/tools/objc/make_hashed_objlist.py': [Errno 2] No such file or directory

Which is weird since make_hashed_objlist.py is properly declared as a data file to libtool:
https://github.com/bazelbuild/bazel/blob/master/tools/objc/BUILD#L42-L49

Here is the diff:

diff --git a/swift/internal/api.bzl b/swift/internal/api.bzl
index 1bb2ca1..334006a 100644
--- a/swift/internal/api.bzl
+++ b/swift/internal/api.bzl
@@ -818,12 +818,15 @@ def _compile_as_library(
 
     if toolchain.system_name == "darwin":
         ar_executable = None
+        libtool_executable = toolchain.libtool_executable
     else:
         ar_executable = toolchain.cc_toolchain_info.ar_executable
+        libtool_executable = None
 
     register_static_archive_action(
         actions = actions,
         ar_executable = ar_executable,
+        libtool_executable = libtool_executable,
         libraries = cc_lib_files,
         mnemonic = "SwiftArchive",
         objects = compile_results.output_objects,
diff --git a/swift/internal/archiving.bzl b/swift/internal/archiving.bzl
index 6380860..72a0b34 100644
--- a/swift/internal/archiving.bzl
+++ b/swift/internal/archiving.bzl
@@ -20,6 +20,7 @@ load(":derived_files.bzl", "derived_files")
 def register_static_archive_action(
         actions,
         ar_executable,
+        libtool_executable,
         output,
         toolchain,
         libraries = [],
@@ -57,6 +58,7 @@ def register_static_archive_action(
         _register_libtool_action(
             actions = actions,
             libraries = libraries,
+            libtool_executable = libtool_executable,
             mnemonic = mnemonic,
             objects = objects,
             output = output,
@@ -126,6 +128,7 @@ def _register_ar_action(
         actions = actions,
         arguments = [args],
         command = command,
+        env = {"ZERO_AR_DATE": "1"},
         inputs = [mri_script] + libraries + objects,
         mnemonic = mnemonic,
         outputs = [output],
@@ -135,6 +138,7 @@ def _register_ar_action(
 
 def _register_libtool_action(
         actions,
+        libtool_executable,
         libraries,
         mnemonic,
         objects,
@@ -175,7 +179,7 @@ def _register_libtool_action(
     run_toolchain_action(
         actions = actions,
         arguments = [args, filelist],
-        executable = "/usr/bin/libtool",
+        executable = libtool_executable,
         inputs = libraries + objects,
         mnemonic = mnemonic,
         outputs = [output],
diff --git a/swift/internal/providers.bzl b/swift/internal/providers.bzl
index a086f89..263672d 100644
--- a/swift/internal/providers.bzl
+++ b/swift/internal/providers.bzl
@@ -193,6 +193,9 @@ on Darwin is required.
         "implicit_deps": """
 `List` of `Target`s. Library targets that should be added as implicit dependencies of any
 `swift_library`, `swift_binary`, or `swift_test` target.
+""",
+        "libtool_executable": """
+`String`. The path to the `libtool` executable, which is used to create archives on macOS.
 """,
         "linker_opts_producer": """
 Skylib `partial`. A partial function that returns the flags that should be passed to Clang to link a
diff --git a/swift/internal/swift_toolchain.bzl b/swift/internal/swift_toolchain.bzl
index 8cd967d..30ae10f 100644
--- a/swift/internal/swift_toolchain.bzl
+++ b/swift/internal/swift_toolchain.bzl
@@ -197,6 +197,7 @@ def _swift_toolchain_impl(ctx):
             cpu = ctx.attr.arch,
             execution_requirements = {},
             implicit_deps = [],
+            libtool_executable = None,
             linker_opts_producer = linker_opts_producer,
             object_format = "elf",
             # TODO(#34): Add SWIFT_FEATURE_USE_RESPONSE_FILES based on Swift compiler version once
diff --git a/swift/internal/xcode_swift_toolchain.bzl b/swift/internal/xcode_swift_toolchain.bzl
index 76f700e..eb00d74 100644
--- a/swift/internal/xcode_swift_toolchain.bzl
+++ b/swift/internal/xcode_swift_toolchain.bzl
@@ -458,6 +458,7 @@ def _xcode_swift_toolchain_impl(ctx):
             cpu = cpu,
             execution_requirements = execution_requirements,
             implicit_deps = [],
+            libtool_executable = ctx.executable._libtool,
             linker_opts_producer = linker_opts_producer,
             object_format = "macho",
             requested_features = requested_features,
@@ -496,6 +497,13 @@ The C++ toolchain from which linking flags and other tools needed by the Swift t
 `clang`) will be retrieved.
 """,
         ),
+        "_libtool": attr.label(
+            cfg = "host",
+            default = Label(
+                "@bazel_tools//tools/objc:libtool",
+            ),
+            executable = True,
+        ),
         "_xcode_config": attr.label(
             default = configuration_field(
                 name = "xcode_config_label",

[keith]

I think that change makes sense. But I'm not sure how others feel about using that script. It is too bad that it doesn't handle ar actions as well.

[steeve]

I think we're going to need to use bazel's libtool wrapper anyhow since the following happens when trying to link multiple swift_proto_library in which the names collide:

/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool: warning same member name (sample.pb.swift.o) in output file used for input files: bazel-out/ios-x86_64-min10.0-applebin_ios-ios_x86_64-fastbuild/bin/external/mylib2.swift.a(sample.pb.swift.o) and: bazel-out/ios-x86_64-min10.0-applebin_ios-ios_x86_64-fastbuild/bin/external/mylib.swift.a(sample.pb.swift.o) due to use of basename, truncation and blank padding

[steeve]

Apparently this is due to bazelbuild/bazel#1147

[steeve]

I managed to work around bazelbuild/bazel#1147 and opened #136.

Works well so far.

[allevato]

I've given this some thought, and here's where I'm at: the Swift rules shouldn't be involved with the nitty-gritty details of static linking at all. The fact that swift_library has to make decisions about whether it should use ar or libtool is already going in the wrong direction.

Fortunately the work that's been going on with the C++/CROSSTOOL APIs (cc_common) will help us do the right thing. There are a couple gaps still in those APIs, but once they're ready, the Swift rules can just pass the .o files to that API and it'll do the right thing based on the CROSSTOOL being used, without having to worry about which tool is invoked or what platform it's on.

For that reason I'd prefer to wait for those APIs rather than cause more churn in the current code, especially when that churn would include pulling in platform-specific external dependencies (which we would also have to handle differently internally within Google). Implementing this in terms of the new API will be a nice clean-up opportunity.

[steeve]

@allevato sounds fair

That said, both PRs are working today, and #136 is pretty simple, so I think we might as well merge them so that reproductible builds are working again, and same name .o files also link properly.

[steeve]

Although indeed https://docs.bazel.build/versions/master/skylark/lib/cc_common.html#create_library_to_link looks nice

[keith]

Ah actually Bazel does have a solution for ar as well https://github.com/bazelbuild/bazel/blob/master/tools/osx/crosstool/wrapped_ar.tpl

[keith]

@allevato since we don't have much of an ETA on when those changes will be available do you think we could merge this PR anyways? Since it's just 2 lines of code I don't think the churn impact should be too bad.

[allevato]

Now that #195 is pushed, this is no longer necessary—static archiving actions use the underlying C++ CROSSTOOL. So, if there are still any hermeticity issues with archiving, they should be fixed there.