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-24000] Module build should error if the apiversion in manifest… #9563

Merged
merged 3 commits into from Nov 13, 2017

Conversation

feons
Copy link
Contributor

@feons feons commented Oct 27, 2017

… does not match the SDKs apiversion

JIRA: https://jira.appcelerator.org/browse/TIMOB-24000

@feons feons requested a review from sgtcoolguy October 27, 2017 16:20
@cb1kenobi
Copy link
Contributor

I thought we discussed that the apiversion should be dictated by the module build for the selected SDK version. No?

@feons
Copy link
Contributor Author

feons commented Oct 27, 2017

then minsdk should also be updated, but do we have a record of the exact min sdk version for a given apiversion?

@cb1kenobi
Copy link
Contributor

I'm not sure. If we don't, then it might be a good idea to start a wiki page (or section of existing page) documenting the min sdk info.

@sgtcoolguy
Copy link
Contributor

We do not have an exact record of the sdk versions that bumped the module api versions, but we should.

The apiversion mismatch is a good time to alert the user to update both that and the minSDK. We could just ignore the user-supplied apiversion and stick the current one into the built manifest, but it means the minSDK has to be updated as well.

We could stick a JSON file into the SDK to map from apiversion to sdk version and insert both (or tell the user what they should be)...

if (this.manifest.apiversion && sdkModuleAPIVersion && this.manifest.apiversion !== sdkModuleAPIVersion) {
logger.error(__('The module manifest apiversion is currently set to %s', this.manifest.apiversion));
logger.error(__('Titanium SDK %s Android module apiversion is at %s', this.titaniumSdkVersion, sdkModuleAPIVersion));
logger.error(__('Please update module manifest apiversion to match Titanium SDK module apiversion.'));
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be good to also alert them that they likely need to update the minSDK value too.

if (this.manifest.apiversion && sdkModuleAPIVersion && this.manifest.apiversion !== sdkModuleAPIVersion) {
logger.error(__('The module manifest apiversion is currently set to %s', this.manifest.apiversion));
logger.error(__('Titanium SDK %s iOS module apiversion is at %s', this.titaniumSdkVersion, sdkModuleAPIVersion));
logger.error(__('Please update module manifest apiversion to match Titanium SDK module apiversion.'));
Copy link
Contributor

Choose a reason for hiding this comment

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

alert about minSDK too.

Copy link
Contributor

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

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

So at a bare minimum we should alert about minSDK too. Ideally we'd be able to tell them the minSDK version to use by adding some JSON file into our SDK (or maybe add to the package.json files) the mapping of apiversion -> minSDK.

If we had that data, we could potentially just inject the apiversion and minSDK for them - though I would assume we should at least alert them that we overrode the values or something.

@mukherjee2 mukherjee2 self-requested a review November 13, 2017 18:54
Copy link
Contributor

@mukherjee2 mukherjee2 left a comment

Choose a reason for hiding this comment

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

Passed FR with this environment:
Node Version: 6.11.5
NPM Version: 3.10.10
Mac OS: 10.13
Windows OS 10.0.14393
Appc CLI: 7.0.0-master.6
Appc CLI NPM: 4.2.11-2
Ti SDK version 7.0.0

@mukherjee2 mukherjee2 merged commit e08975b into tidev:master Nov 13, 2017
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.

None yet

4 participants