Pass macOS version min flag in osx CROSSTOOL

[keith]

This fixes an issue where cc_library targets that are dependent on from
swift_binaries weren't passed the version min flag which lead to linker
warnings about version mismatches.

More details: bazelbuild/rules_swift#114

[jin]

@jmmv assigning to you since @aragos is OOO, feel free to reassign.

[jmmv]

I think the Apple rules folks are better suited to review this. Assigned to them as reviewers.

[keith]

I'm investigating the failures, it looks like somehow the iOS SDK version is sneaking into the commands

[keith]

Hmm actually. I think the problem is related to #6366

Since we're not passing --apple_platform_type for any of the tests, it's defaulting to iOS. Now the question is why it would be using these new crosstool arguments if that's the case

[trybka]

Perhaps the platform override specifying 'macos' is causing it to use the darwin CROSSTOOL stanza (and thus, your new flags) but the lack of apple_platform_type is passing the wrong version-min into the build var?

[keith]

It must be. Without my patch the correct target is passed to one of these failing tests:

clang" "-cc1" "-triple" "x86_64-apple-macosx10.14.0" ...

But with this patch that triple and this new flag are broken:

clang" "-cc1" "-triple" "x86_64-apple-macosx12.1.0" ...

[keith]

It turns out even with my change the isysroot / __BAZEL_XCODE_SDKROOT__ is still set correctly too 🤔

[trybka]

Can you modify the test configurations for things which run on darwin/macos to pass the correct --apple_platform_type, does that help?

[keith]

Yes that does fix it. But I believe since Google doesn't do that this would be a breaking change

…

-- Keith Smiley

On Dec 14, 2018, at 08:33, trybka ***@***.***> wrote: Can you modify the test configurations for things which run on darwin/macos to pass the correct --apple_platform_type, does that help? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[trybka]

Does it work now with bf04c53 submitted?

[keith]

I've rebased on master so if someone could re-trigger the tests we can find out!

[keith]

Looks like it's having the same issue. Which makes sense locally since that change only applied to CI

[keith]

@sergiocampama any advice on the test side here?

[sergiocampama]

these tests continue to fail because the tests themselves invoke bazel, and those invocations do not have the --apple_platform_type set.

[keith]

Hmm so I actually think this is an existing bug that my change is surfacing. Without my commit if you build a tiny cc_binary:

cc_binary(
    name = "foo",
    srcs = ["foo.cc"],
)

with -s you see the command doesn't pass any platform specific flags:

external/local_config_cc/wrapped_clang '-D_FORTIFY_SOURCE=1' -fstack-protector -fcolor-diagnostics -Wall -Wthread-safety -Wself-assign -fno-omit-frame-pointer -O0 -DDEBUG '-std=c++11' -iquote . -iquote bazel-out/darwin-fastbuild/genfiles -iquote bazel-out/darwin-fastbuild/bin -iquote external/bazel_tools -iquote bazel-out/darwin-fastbuild/genfiles/external/bazel_tools -iquote bazel-out/darwin-fastbuild/bin/external/bazel_tools -MD -MF bazel-out/darwin-fastbuild/bin/_objs/foo/foo.d '-frandom-seed=bazel-out/darwin-fastbuild/bin/_objs/foo/foo.o' -isysroot __BAZEL_XCODE_SDKROOT__ -no-canonical-prefixes -Wno-builtin-macro-redefined '-D__DATE__="redacted"' '-D__TIMESTAMP__="redacted"' '-D__TIME__="redacted"' -c foo.cc -o bazel-out/darwin-fastbuild/bin/_objs/foo/foo.o

I think this is already wrong because since --apple_platform_type defaults to iOS, it should be passing an iOS target at least? But instead it's allowing clang to pick a default. The final foo.o binary here isn't actually an iOS binary since clang picks macOS as the default:

% otool -l bazel-out/darwin-fastbuild/bin/foo | grep -C 2 platform
       cmd LC_BUILD_VERSION
   cmdsize 32
  platform macos
       sdk 10.14
     minos 10.14

Then with my change we actually start passing the version min flag, with the version based on apple_platform_type, but somehow it's pulling from the macOS portion of the crosstool, even though apple_platform_type is iOS, which correctly leads to the clang error.

[sergiocampama]

The error stems from this line.

What happens is that the version is being set for the platform type, which by default is iOS. But the CROSSTOOL is setting up the flags for whatever CPU is set, which by default appears to be the host cpu, in this case being darwin_x86_64. This is the mismatch that makes the CROSSTOOL choose the macos flags, but the crosstool variables to choose the default platform type's version, which is iOS.

[keith]

Trying to fix this by merging #6366 first instead

[keith]

Now that the linked PR is merged I've rebased this one, which should mean our tests are green here. I have a local change to validate that apple_platform_type is the same as expected for cpu so we can catch the issue that blocked this earlier in the future, but that change currently depends on #7062

[keith]

@trybka mind reviewing this now that everything's green?

[keith]

@trybka friendly ping!

[trybka]

LGTM

[keith]

Thanks! @trybka can you help me land this?

[keith]

@trybka if you have a chance it would be great if we could land this for 0.23

[trybka]

@keith sorry for the delay, I'm not officially on the Bazel team, but they're helping me learn the process. I've sent it for review to land, and hope to get it imported on Monday.

[keith]

Oh sorry I didn't know. Sounds great though thanks!

[keith]

Thanks!