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

[TIMOB-26121] iOS: Prioritize selected xcode when determining default simulator #10100

Merged
merged 9 commits into from Jun 25, 2018

Conversation

garymathews
Copy link
Contributor

@garymathews garymathews commented Jun 11, 2018

  • Always prioritize the selected xcode when determining the default simulator iOS version
TEST CASE
  • Install xcode-beta 10.0 along with xcode 9.x
  • Use xcode-select to select 9.x
  • Build a project without specifying a simulator: appc run -p ios -l trace
  • A compatible iOS 11 simulator will be selected

JIRA Ticket

@garymathews garymathews added this to the 7.3.0 milestone Jun 11, 2018
@garymathews garymathews modified the milestones: 7.3.0, 7.4.0 Jun 11, 2018
@build
Copy link
Contributor

build commented Jun 12, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

return xcodeInfo[a].selected || appc.version.gt(xcodeInfo[a].version, xcodeInfo[b].version) ? -1 : appc.version.lt(xcodeInfo[a].version, xcodeInfo[b].version) ? 1 : 0;
// prioritize selected xcode
if (xcodeInfo[b].selected) {
return 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to be mindful of the original sort order. This change makes the selected Xcode the last element where the previous code had it be the first. This sort function is used in 2 places. The first instance reverses the results and the other doesn't. This code would change that behavior.

iosSdkVersion is set if an iOS SDK version was passed in via the command line --ios-version <ver> option. When this is set, we want to pick the most recent Xcode or selected Xcode.

If there's no iosSdkVersion and it's a device/dist build, then we pick the oldest Xcode. Why? I have no idea. Compatibility? Who knows?

So, I'm thinking you either need to remove the .reverse() or change the -1 to 1 and 1 to -1 in the sort function. The former is probably the better, cleaner route. We should also figure out why the code chose the oldest Xcode to select an iOS Simulator from.

@garymathews
Copy link
Contributor Author

@cb1kenobi Updated PR

@@ -1954,9 +1958,8 @@ iOSBuilder.prototype.validate = function validate(logger, config, cli) {
logger.error(__('Unable to find any Xcode installations that support iOS SDK %s.', this.iosSdkVersion) + '\n');
process.exit(1);
}
} else if (cli.argv.target === 'simulator' && !cli.argv['build-only']) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can't do this. This line must remain.

If you're doing a sim build, running the app in a sim, and you didn't specify an iOS SDK version, then do nothing. Why? Because on https://github.com/garymathews/titanium_mobile/blob/74c48667224892db965fcca6b70cbf1fffcabffd/iphone/cli/commands/_build.js#L2012 when it calls ioslib.simulator.findSimulators(), it will determine the best iOS sim and Xcode version based on the inputs and return the results. If there isn't a selected iOS SDK version, then we use the latest associated with the Xcode version for the simulator it chose (https://github.com/garymathews/titanium_mobile/blob/74c48667224892db965fcca6b70cbf1fffcabffd/iphone/cli/commands/_build.js#L2043-L2046).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there isn't a selected iOS SDK version, then we use the latest associated with the Xcode version for the simulator it chose.

That is what this change fixes. It should not use the latest Xcode as it may not be supported and it's ignoring my xcode-select selection. With this change, it will use a supported Xcode version and select the first listed simulator that is greater than our minIosVersion.

The Xcode used should not change between:

appc run -p ios
appc run -p ios --build-only

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I'm not concerned with what version of Xcode is used. I'm very concerned if the iOS SDK changes depending on whether or not --build-only is specified.

I think part of the complexity is the iOS SDK version is passed in as a command line argument when I believe it should be a setting in the tiapp.xml.

I'm fine with this PR, but then I believe lines 2043-2046 become dead code.

I think we also need to fix this in ioslib. The Titanium build shouldn't need to be burdened with all this selecting/sorting logic. Care will need to be taken as to not repeat what got us in this situation.

Copy link
Contributor

@cb1kenobi cb1kenobi left a comment

Choose a reason for hiding this comment

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

You still have a problem in _build.js with the removal of line 1957-1959. Those lines are necessary.

Copy link
Contributor

@cb1kenobi cb1kenobi left a comment

Choose a reason for hiding this comment

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

CR'd and FR'd. APPROVED!

@build
Copy link
Contributor

build commented Jun 25, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

@hansemannn hansemannn merged commit d361b57 into tidev:master Jun 25, 2018
@hansemannn hansemannn modified the milestones: 7.4.0, 7.5.0 Aug 24, 2018
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

5 participants