Add --host_macos_minimum_os flag

[keith]

This flag makes sure we set the minimum macOS version when compiling
host tools. Otherwise you can end up not sharing caches across
different OS versions.

Fixes #12988

[thii]

Related change previously: #9403.

[allevato]

This seems reasonable, but I'm surprised the problem described in the linked issue hasn't been more widespread. I think I'd like @trybka to take a look at it too just to make sure this is the right way to tackle it.

[trybka]

I think this is fine? I'm not sure I entirely understand the usecase, though I can understand usecases where it might help.

This also feels like maybe not ideal, but until we have widespread execution platforms, I don't see a better alternative.

[brentleyjones]

The cache sharing would be based on different Xcode versions, not OS versions.

I'm not entirely sure what targeting a newer-than-current OS version does, since it's clearly "working" as is, but it's less than ideal imo, and we should be able to target our actual minimum OS. Maybe it could affect deprecation warnings for example?

[keith]

Did a bit more digging here. So it looks like it will happily create a binary for the SDK version you specify, and run it on an older version, but the downside is you get none of the protections around API usage, so there's the big risk. You won't find out until runtime on an older OS that your tools are actually incompatible in some way depending on what API they use.

[allevato]

I spent some more time thinking about this because something about it still didn't feel quite right. Some random thoughts:

• Conceptually, minimum deployment version is a property of individual targets, not of an entire build.
• The existence of --{ios,macos,tvos,watchos}_minimum_os command line flags would seem to go against that, but their main purpose is to be set by a transition based on an attribute value on an Apple bundle rule so that they apply to the {cc,objc,swift}_library targets underneath. They can be set directly when building a *_library target from the command line, but we discourage that.
• But, the flags also end up applying to rules like cc_binary and swift_binary, which don't have a way to set the deployment version since they also support non-Apple platforms, so they don't use the same transitions. Or perhaps stated more accurately, they don't so much "apply to {cc,swift}_binary" as they serve as a fallback value for anything in the build that isn't under a transition that sets it more specifically.

The original issue mentioned the use case being a genrule with a swift_binary tool. What if you had two or more such tools in your build that were written with different minimum deployment targets in mind? I think it's almost always fine to just go with the higher value, but if you want to build them as they were intended, you'd want to use different values for each one. In order to do that with rules like {cc,swift}_binary, you'd have to write a small rule + transition combo that sets --macos_minimum_os and then passes through the underlying binary. That's a lot of ceremony, unfortunately. But I'd like to make sure there's not a cleaner/more correct way out of this before we add a new command line flag (especially at a time when we're trying to reduce, rather than increase, the Apple API surface in Bazel core).

[keith]

I think it's almost always fine to just go with the higher value

I don't think you could ever jump to just 1 value for this given that in either case of going higher or lower you could fail the build depending on the warnings / errors that were introduced with different deployment targets.

Definitely open the other suggestions here, having folks create their own transitions to solve this case is a super high barrier of entry to solve this bug so I think we should land some solution

[aiuto]

@gregestren @katre Could this be done without a flag? It seems very similar to the Android case.

[katre]

Given that this is adding a host version of an existing flag, I think it makes sense.

The new Starlark flag we've added for Android's minimum SDK version is only possible because that affects the NDK, which is really a cc_toolchain, and so we added lots of select code in there to read and use the new flag. I don't think the macOS rules are that far moved to Starlark yet, so trying to use a Starlark flag isn't really appropriate there yet.

[brentleyjones]

@aiuto: Given @katre's and @trybka's comments, can this be merged?

[aiuto]

@allevato @trybka Can one of you please do a review and actually approve.
And, does anyone know who @Highaboveus2 is? It looks like a random driveby.

[aiuto]

OK. I'll do the merge.

[aiuto]

Damm. It must have gone stale and tests don't pass any more.
https://buildkite.com/bazel/google-bazel-presubmit/builds/49852
https://storage.googleapis.com/bazel-untrusted-buildkite-artifacts/02d97ba9-2756-40f0-b56a-70a3d405fda2/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRulesTests/attempt_1.log

Keith: can you look into that?

[keith]

Should be green shortly @aiuto

[brentleyjones]

Can this be imported now? 🙏 Thanks!

[aiuto]

Sigh. I am trying to merge this, but it fails, and I don't know how to fix the newly added test.
https://storage.googleapis.com/bazel-untrusted-buildkite-artifacts/cdc80700-682c-4e3e-90a3-53e0cd6c121c/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRulesTests/shard_13_of_20/test_attempts/attempt_1.log

[keith]

Hrm, I thought that was the one I fixed last time, testing locally again

[keith]

@aiuto which build did that failure come from? I don't see that locally anymore, did that include my more recent force push? (rebased to have CI run again for good measure)

This is what im testing with locally:

bz test src/test/java/com/google/devtools/build/lib/rules/objc:all

[aiuto]

Oh,... silly me. I may have to re-sync. I'll check later tonight.

…

On Mon, Oct 4, 2021 at 5:29 PM Keith Smiley ***@***.***> wrote: @aiuto <https://github.com/aiuto> which build did that failure come from? I don't see that locally anymore, did that include my more recent force push? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13001 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXHHHAJX4CQ3IESXRMSEEDUFIMFPANCNFSM4XPJNMJQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.