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

chore(ios): drop xcode 8 support #11343

Merged
merged 3 commits into from
Dec 9, 2019
Merged

Conversation

vijaysingh-axway
Copy link
Contributor

@build build added this to the 9.0.0 milestone Nov 18, 2019
@build build requested a review from a team November 18, 2019 20:52
@build
Copy link
Contributor

build commented Nov 18, 2019

Messages
📖 👍 Hey!, You deleted more code than you added. That's awesome!
📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖

✅ All tests are passing
Nice one! All 4562 tests are passing.
(There are 464 skipped tests not included in that total)

Generated by 🚫 dangerJS against 0dc833a

@sgtcoolguy
Copy link
Contributor

Pretty important to note that this also bumps minimum iOS from 9 to 10.

@ewanharris
Copy link
Collaborator

@sgtcoolguy, I thought the minIosVersion dictated that and the ios sdk entry dictated the minimum compiling SDK? (I'm sure that one might be being used incorrectly though somewhere). I'll build this PR locally and build to an iOS 9 device to see what happens

@sgtcoolguy
Copy link
Contributor

Ok, so we have two separate "minimums" in the package.json. The one bumped here (in vendorDependencies) indicates "the minimum supported iOS SDK required when building" according to our CLI code. It's also used by the node-titanium-sdk transpile target as the minimum to transpile for.

The other minIosVersion is also used by the Cli, but honestly it's hard to tell what the difference here is/should be. Seems to be related to code selecting xcodes/sims/target sdk version? There's a whole section of the code checking this value: https://github.com/appcelerator/titanium_mobile/blob/master/iphone/cli/commands/_build.js#L2115-L2160

Maybe @cb1kenobi has a better handle on the difference between the two (or why we have 2 values that more or less seem to denote min iOS sdk versions)?

@ewanharris
Copy link
Collaborator

Just confirming that this PR works fine building to an iOS 9 device, I think the transpile target should be updated to use this.minIosVersion which correctly calculates the minimum iOS version for an app based off minIosVersion from package.json and the min-ios-ver tag from the tiapp, not the iOS SDK which has no bearing on the JS Engine that will be targeted

@vijaysingh-axway
Copy link
Contributor Author

Yes. This will not change minimum iOS target. cc @cb1kenobi

Copy link
Contributor

@janvennemann janvennemann left a comment

Choose a reason for hiding this comment

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

The iOS changes are looking good, approved!

BTW, i noticed it too that we use the wrong value for the preset-env option while working on the Webpack tooling. The appcd plugin already uses the correct value:

https://github.com/appcelerator/appcd-plugin-webpack/blob/597b2981ae05bfb1c0d95f259a4293a12fecb3f8/src/utils/webpack.js#L93

// EDIT: I created https://jira.appcelerator.org/browse/TIMOB-27630 for the update in the SDK

Copy link
Collaborator

@ewanharris ewanharris left a comment

Choose a reason for hiding this comment

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

Seeing as the babel setup fix was split out to TIMOB-27630, this PR looks good to me

@ssekhri
Copy link

ssekhri commented Dec 5, 2019

FR Passed.
Verified on:
Mac OS: 10.15.1, 10.14.6
SDK: 9.0.0.v20191118123859
Appc CLI: 7.1.2
JDK: 11.0.4
Node: 10.16.3

@sgtcoolguy sgtcoolguy merged commit d5ede39 into tidev:master Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants