-
Notifications
You must be signed in to change notification settings - Fork 138
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 some robustness to the CI and switch to Xcode 12 #709
Conversation
Merge upstream into fork
.ado/apple-pr.yml
Outdated
@@ -31,6 +32,8 @@ jobs: | |||
- template: templates/apple-job-javascript.yml | |||
parameters: | |||
apply_office_patches: $(apply_office_patches) | |||
slice_name: 'Xcode_12_2' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should skip to 12.4
@@ -24,7 +24,7 @@ Pod::Spec.new do |s| | |||
s.documentation_url = "https://reactnative.dev/docs/actionsheetios" | |||
s.license = package["license"] | |||
s.author = "Facebook, Inc. and its affiliates" | |||
s.platforms = { :ios => "10.0", :tvos => "10.0", :osx => "10.13" } # TODO(macOS GH#214) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ios=>"13.0" it will make our sizes small too...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we should try to do that ASAP, but I'm not sure we necessarily want to diverge from upstream here. it would be weird if someone consumed this fork of react-native for x-plat support but that resulted in a different minimum supported OS than the upstream repo.
Separate of this change it might be good to try to push upstream to be more aggressive with minimum supported OS given usage trends and practical impact of size as well as work internally to see if we can override this in our consumption of react-native-macos (and in turn provide guidance to other clients who want to upgrade their minimum supported OS before we are able to get a newer version pushed to upstream)
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
@@ -21,7 +21,7 @@ steps: | |||
signingOption: auto | |||
packageApp: false | |||
teamId: '$(XCodeSigningMicrosoftTeamID)' | |||
args: '-destination "${{ parameters.xcode_destination }}" ONLY_ACTIVE_ARCH=NO -verbose -UseModernBuildSystem=NO -derivedDataPath DerivedData ${{ parameters.xcode_extraArgs }}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HeyImChris was there any particular reason you know of we were forcing legacy build system? I probably should have done things one at a time to control variables but this was a delta between CI and building locally in Xcode by default so seemed helpful to get rid of that to make it easier to reproduce CI issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The legacy build system used to be required before we built using cocoapods. The old static xcodeproj's from upstream had intentional circular dependencies that were worked around by having build order in the xcschemes. The new pod based builds don't have this issue anymore.
Please select one of the following
Summary
We need to switch to Xcode 12 so let's leverage that fact we're in here to parameterize there versions so Xcode select is compatible with any future Xcode version changes we need to make going forward.
Changelog
[macOS/iOS] [Improvement] - Add robustness to CI
Test Plan
Will verify if the CI passes or not as this doesn't affect the product.
Microsoft Reviewers: Open in CodeFlow