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

[apple] support watchos_arm64 in toolchain #14512

Closed
wants to merge 1 commit into from

Conversation

dflems
Copy link
Contributor

@dflems dflems commented Jan 4, 2022

The watch-only changes from #14439

"watchos_arm64": [
"@platforms//os:ios",
"@platforms//cpu:aarch64",
],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if this will conflict with the one below or if there's a different CPU that should be used for one or the other?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it may in the future. If the constraints are the same, toolchain resolution would pick this one first.
But, what is up with the name watchos_arm64_32?
That entry is strange with a name that implies 32 bits, while using aarch64.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arm64_32 is a variant of arm64 with 32-bit pointer sizes, used on Apple Watch Series 4 and later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure about these. But ios_sim_arm64 and ios_arm64 are identical here too, so if it's wrong what's in here already is wrong.

@dflems
Copy link
Contributor Author

dflems commented Jan 4, 2022

cc @Wyverald @kaylathar I broke this out of that other PR (#14439). The watchos_arm64 CPU already existed. This PR doesn't add any new ones, just integration with the default osx toolchain.

@kaylathar
Copy link
Member

This LGTM - no concerns

@brentleyjones
Copy link
Contributor

@oquenchil or @Wyverald can you import?

@aiuto aiuto requested a review from allevato January 7, 2022 13:41
Copy link
Contributor

@aiuto aiuto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really a review. I don't have enough knowledge of the Apple naming conventions.

"watchos_arm64": [
"@platforms//os:ios",
"@platforms//cpu:aarch64",
],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it may in the future. If the constraints are the same, toolchain resolution would pick this one first.
But, what is up with the name watchos_arm64_32?
That entry is strange with a name that implies 32 bits, while using aarch64.

@Wyverald
Copy link
Member

Wyverald commented Jan 7, 2022

I'll do the import.

@Wyverald
Copy link
Member

Wyverald commented Jan 7, 2022

@katre asks:

Not @platforms//os:watchos?

I don't know why they are all set like this, but this feels like a misconfiguration to me.

@brentleyjones
Copy link
Contributor

I don't think this change should change those, as that's a much larger and riskier change for this current addition.

@bazel-io bazel-io closed this in b341802 Jan 7, 2022
@katre
Copy link
Member

katre commented Jan 7, 2022

Someone should investigate these, but I don't know enough about the difference between the ios variants to know what's correct.

brentleyjones pushed a commit to brentleyjones/bazel that referenced this pull request Jan 7, 2022
The watch-only changes from bazelbuild#14439

Closes bazelbuild#14512.

PiperOrigin-RevId: 420296580
(cherry picked from commit b341802)
@dflems dflems deleted the dflems/20220104-watch branch January 7, 2022 23:13
Wyverald pushed a commit that referenced this pull request Jan 8, 2022
The watch-only changes from #14439

Closes #14512.

PiperOrigin-RevId: 420296580
(cherry picked from commit b341802)

Co-authored-by: Dan Fleming <dflems@spotify.com>
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.

7 participants