Skip to content

Fix AppCheck Catalyst build on Xcode 12.4 #8184

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 3, 2021
Merged

Fix AppCheck Catalyst build on Xcode 12.4 #8184

merged 3 commits into from
Jun 3, 2021

Conversation

paulb777
Copy link
Member

@paulb777 paulb777 commented Jun 3, 2021 •
edited
Loading

Fix #8175

Fix AppCheck Catalyst build and add CI. This fixes a regression building the zip.

Interestingly, this failure does not repro on Xcode 12.5

@google-cla google-cla bot added the cla: yes label Jun 3, 2021
@google-oss-bot
Copy link

1 Warning
⚠️ Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger

@maksymmalyhin
Copy link
Contributor

Looks like the availability of DCAppAttestService changed in Xcode 12.5 from

API_AVAILABLE(ios(14.0)) API_UNAVAILABLE(macos, tvos, watchos)

to

API_AVAILABLE(macos(11.0), ios(14.0)) API_UNAVAILABLE(tvos, watchos)

It also reflected in the docs.

@maksymmalyhin
Copy link
Contributor

Keychain dependent logic require additional configuration on Catalyst (enabling Keychain sharing). Probably it will be easier just to disable the Keychain dependent tests for Catalyst.

@paulb777 paulb777 requested a review from maksymmalyhin June 3, 2021 15:26
maksymmalyhin
maksymmalyhin approved these changes Jun 3, 2021
View reviewed changes
Copy link
Contributor

@maksymmalyhin maksymmalyhin left a comment

Choose a reason for hiding this comment

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

Thank you for fixing it!

FirebaseAppCheck/Tests/Unit/AppAttestProvider/FIRAppAttestProviderTests.m Outdated
@@ -92,6 +92,7 @@ - (void)tearDown {

#pragma mark - Init tests

#if !TARGET_OS_MACCATALYST
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Could you please add a comment explaining why the tests are disabled.

FirebaseAppCheck/Sources/AppAttestProvider/DCAppAttestService+FIRAppAttestService.h Outdated
@@ -15,7 +15,7 @@
*/

// Currently DCAppAttestService is available on iOS only.
#if TARGET_OS_IOS
#if TARGET_OS_IOS && !TARGET_OS_MACCATALYST
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe not in this PR, but, I think we should allow using App Attest on macOS and Catalyst with Xcode 12.5+. We probably will need to introduce a separate flag like FIR_APP_ATTEST_SUPPORTED where we will take into account the Xcode version, platform and OS version (if needed). Then this flag can be used instead of this condition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding support in the future sounds good to me. Agreed that it is beyond the scope of this PR that is focused on fixing CI to unblock other work.

@paulb777 paulb777 merged commit 1d9bb05 into master Jun 3, 2021
@paulb777 paulb777 deleted the pb-ac-catalyst branch June 3, 2021 16:35
paulb777 added a commit that referenced this pull request Jun 3, 2021
@paulb777
Restore zip.yml after fix from #8184
f712fd6
@firebase firebase locked and limited conversation to collaborators Jul 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@maksymmalyhin maksymmalyhin maksymmalyhin approved these changes

Assignees
No one assigned
Labels
api: appcheck cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Nightly Testing Report
3 participants
@paulb777 @google-oss-bot @maksymmalyhin