Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for iOS, watchOS, and tvOS #245

Merged
merged 3 commits into from
Jan 25, 2022
Merged

Add support for iOS, watchOS, and tvOS #245

merged 3 commits into from
Jan 25, 2022

Conversation

cpsauer
Copy link
Collaborator

@cpsauer cpsauer commented Jan 20, 2022

Thank you, @nelhage, for an amazingly helpful set of rules!

I took a solid crack at adding support for iOS, watchOS, and tvOS this evening. (We just switched to using Bazel's @platforms// via a platform_mappings file.) I'm hoping you'd consider taking it as a contribution!

This commit was meant to be the simplest, most readable implementation change that I thought would work, for ease of review. So it's just adding parallel cases--plus a few places where I couldn't resist massaging some selects->with_or that'd simplify the cases I was adding.

Independently and subsequently, we may well want to do a pass over the repo, where we move the deprecated @bazel_tools//platforms constraint_value()s over to the @platforms// constraint_value()s they're aliases of, but that's probably best separated to keep things small. We might want to just directly use the constraint_values in selects at the same time, to eliminate some boilerplate. [Edit: This doesn't fully work until Bazel 6.] But that can wait. Lmk, though, if there's a reason we shouldn't do that.

To head off a couple things that might look wrong but are intentional:
watchOS and tvOS don't have @bazel_tools//platforms equivalents, so I used their shiny new @platforms// paths off the bat. Inconsistency necessary, unfortunately.
The Apple toolchains do indeed (mis)use arm for armv7 and armv7k --those aren't typos.

Obviously, feel free to modify whatever. Looking forward to hearing what you think!

Thanks for your consideration!
Chris

P.S. Details on how I got the lzma configs, saved to the end for readability.
Grabbed version 5.2.3 from source forge, parallel to boost.bzl.
Ran this to generate configs the distinct, new configs you see added:

CC="clang -arch arm64 \
-isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS.sdk" \
./configure --host=x86_64-apple-darwin21.2.0
cp config.h ~/Downloads/rules_boost/config.lzma-ios-arm64.h

CC="clang -arch armv7 \
-isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS.sdk" \
./configure --host=x86_64-apple-darwin21.2.0
cp config.h ~/Downloads/rules_boost/config.lzma-ios-armv7.h

CC="clang -arch i386 \
-isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator.sdk" \
./configure --host=x86_64-apple-darwin21.2.0
cp config.h ~/Downloads/rules_boost/config.lzma-ios-i386.h

And I did some checks to eliminate ones that were duplicates. For example, iOS x86_64 is the same as the existing macOS x86_64.

CC="clang -arch x86_64 \
-isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator.sdk" \
./configure --host=x86_64-apple-darwin21.2.0
cp config.h ~/Downloads/rules_boost/config.lzma-ios-x86_64.h

And armv7k for watchOS was the same as armv7 for iOS.

CC="clang -arch armv7k \
-isysroot /Applications/Xcode.app/Contents/Developer/Platforms/WatchOS.platform/Developer/SDKs/WatchOS.sdk" \
./configure --host=x86_64-apple-darwin21.2.0
cp config.h ~/Downloads/rules_boost/config.lzma-watchos-armv7k.h

@cpsauer
Copy link
Collaborator Author

cpsauer commented Jan 20, 2022

Notes on test failures:
(1) Buildkite is windows tests failing--looks to be the same failure as at HEAD.
(2) The second is @platforms//os:tvos missing, I'm guessing because a quite an old version of Bazel is being used? I see v1.1 in the logs.

Lmk if either of those are not okay/unexpected.

[Going to correct the capitalization of, e.g. tvOS in a comment. So please don't be alarmed by the force-push.]

@cpsauer
Copy link
Collaborator Author

cpsauer commented Jan 20, 2022

Okay, so one other thing:
iOS, watchOS, and tvOS all bundle libz (I'm seeing the same 1.2.11 that you're fetching in, for example /Applications/Xcode.app/Contents/Developer/Platforms/WatchOS.platform/Developer/SDKs/WatchOS.sdk/usr/lib/ I'm pretty sure it's bundled on Android, too...

Thoughts on modifying BUILD.zlib to just add -lz to the linkopts on those platforms, instead of re-bundling? (Update: Looks like TF has already considered this case, so I'll toss up a commit.)

@nelhage
Copy link
Owner

nelhage commented Jan 24, 2022

This looks great!

Can you rebase on top of master? I fixed Windows CI and also bumped some version on Circle in #246, so that should let this go green.

@cpsauer
Copy link
Collaborator Author

cpsauer commented Jan 25, 2022

Thank you! Absolutely. Done--and we're green! Glad to hear you like it :)

(For clarity: Same commit contents; rebase entailed force push.)

@cpsauer
Copy link
Collaborator Author

cpsauer commented Jan 25, 2022

Since you seemed positive on prior changes, I figured I'd go ahead and add a commit doing that cleanup I'd mentioned earlier, moving the deprecated @bazel_tools//platforms constraint_value()s over to using @platforms// constraint_value()s they're aliases of. For clarity, this isn't the part that requires Bazel 6--hence things staying green.

It's just an equivalent cleanup; no functionality changes. But the advantages are moving off a deprecated API and consistency with, e.g., watchOS, which can't be expressed with the old API (per original message).

Obviously, feel free to revert if you don't like. But I thought I should go ahead, since it seemed likely you'd approve, and just batching the change into this PR seemed like a better use of your time.

@nelhage nelhage merged commit 5be5ea4 into nelhage:master Jan 25, 2022
@nelhage
Copy link
Owner

nelhage commented Jan 25, 2022

Thanks for the fixups! Appreciate you helping to keep us on top of current standards :)

@cpsauer
Copy link
Collaborator Author

cpsauer commented Jan 25, 2022

My pleasure! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants