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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement CORDOVA_COMPATIBILITY_VERSION_IOS/ANDROID and EXCLUDE #8581

Merged
merged 2 commits into from Apr 26, 2017

Conversation

@wojtkowiak
Copy link
Contributor

@wojtkowiak wojtkowiak commented Apr 10, 2017

Hey,

quoting commit's message:
CORDOVA_COMPATIBILITY_VERSION_IOS or CORDOVA_COMPATIBILITY_VERSION_ANDROID allows to override compatibility version for a specified platform.

CORDOVA_COMPATIBILITY_VERSION_EXCLUDE provides a way of excluding a certain plugin from compatibility version calculation. You can pass several plugin names with ';'. For example: CORDOVA_COMPATIBILITY_VERSION_EXCLUDE='cordova-plugin-crosswalk-webview;cordova-plugin-device'

This addresses:
#7030
and somehow: #7183

Why CORDOVA_COMPATIBILITY_VERSION_EXCLUDE?
This can be used in many ways but the one crucial is crosswalk. Since it will no longer be developed https://crosswalk-project.org/blog/crosswalk-final-release.html this is crucial to be able to build two versions of the app while still being able to benefit from HCP on both. One can add the crosswalk plugin to exclude for instance and have the same compatibility version on both builds (the one with and without crosswalk).

@abernix, @martijnwalraven let me now what do you think about this 馃檪

鈥ITY_VERSION_IOS/ANDROID

CORDOVA_COMPATIBILITY_VERSION_IOS or CORDOVA_COMPATIBILITY_VERSION_ANDROID allows to override compatibility version for a specified platform.

CORDOVA_COMPATIBILITY_VERSION_EXCLUDE provides a way of excluding a certain plugin from compatibility version calculation. You can pass several plugin names with ';'. For example: `CORDOVA_COMPATIBILITY_VERSION_EXCLUDE='cordova-plugin-crosswalk-webview;cordova-plugin-device'`
@wojtkowiak
Copy link
Contributor Author

@wojtkowiak wojtkowiak commented Apr 10, 2017

Is there a way to run the CI with self-test --slow?

Copy link
Member

@abernix abernix left a comment

I don't have much input on the Cordova side of this (perhaps @martijnwalraven will?) but these changes all appear to be opt-in to me and should only affect users if they explicitly enable the changes with environment variables, so I'm generally okay with their existence.

My thoughts on the implementation are below.

As far as tests go, unfortunately tests which are tagged with "slow" have to be run elsewhere as the full-suite can take several hours and I believe CircleCI would time-out. (I'm not even sure if CircleCI can run the Cordova emulator anymore?) When this is ready to go, I'm happy to spawn the tests elsewhere for you, but if it's just for the sake of running this one test feel free to commit a test without the slow tag and we can add the slow tag back later before we merge. You can also try making whatever changes you'd like if you run your own CircleCI.

version,
this.cordovaDependencies);
let cordovaDependencies = Object.assign({}, this.cordovaDependencies);
let pluginsExcludedFromCompatibilityHash = process.env.CORDOVA_COMPATIBILITY_VERSION_EXCLUDE || '';

This comment has been minimized.

@abernix

abernix Apr 10, 2017
Member

I realize this variable is already long, but we should prefix (new)聽environment variables with METEOR_.

Also, I think "version" makes sense for the other environment variable, but maybe this should be "hash" (since it's being excluded from the calculateCordovaCompatibilityHash function)? (Also, it's shorter!)

Maybe METEOR_CORDOVA_COMPAT_HASH_EXCLUDE?

This comment has been minimized.

@wojtkowiak

wojtkowiak Apr 10, 2017
Author Contributor

Yeah, been thinking about this but I would rather stick to VERSION even though it's longer. It's named that way in the guide and also in the manifest.json you have cordovaCompatibilityVersion field. But it's your call so have a second thought 馃檪 and let me know.

this.cordovaDependencies);
let cordovaDependencies = Object.assign({}, this.cordovaDependencies);
let pluginsExcludedFromCompatibilityHash = process.env.CORDOVA_COMPATIBILITY_VERSION_EXCLUDE || '';
cordovaDependencies = _.omit(cordovaDependencies, pluginsExcludedFromCompatibilityHash.split(';'));

This comment has been minimized.

@abernix

abernix Apr 10, 2017
Member

Instead of re-assigning cordovaDependencies, is there a reason we can't switch the order the variables are created and then switch them to consts?

const pluginsExcludedFromCompatibilityHash =
  (process.env.CORDOVA_COMPATIBILITY_VERSION_EXCLUDE || '')
  .split(';');
const cordovaDependencies =
  Object.assign(Object.create(null),
    _.omit(this.cordovaDependencies, pluginsExcludedFromCompatibilityHash));

This comment has been minimized.

@wojtkowiak

wojtkowiak Apr 10, 2017
Author Contributor

Nope, there is no reason. Good point, will be switched.

let pluginsExcludedFromCompatibilityHash = process.env.CORDOVA_COMPATIBILITY_VERSION_EXCLUDE || '';
cordovaDependencies = _.omit(cordovaDependencies, pluginsExcludedFromCompatibilityHash.split(';'));

const hash = process.env[`CORDOVA_COMPATIBILITY_VERSION_${platform.toUpperCase()}`] ||

This comment has been minimized.

@abernix

abernix Apr 10, 2017
Member

METEOR_CORDOVA_COMPAT_VERSION_<platform>?

This comment has been minimized.

@wojtkowiak

wojtkowiak Apr 10, 2017
Author Contributor

Changed

this.cordovaDependencies);
let cordovaDependencies = Object.assign({}, this.cordovaDependencies);
let pluginsExcludedFromCompatibilityHash = process.env.CORDOVA_COMPATIBILITY_VERSION_EXCLUDE || '';
cordovaDependencies = _.omit(cordovaDependencies, pluginsExcludedFromCompatibilityHash.split(';'));

This comment has been minimized.

@abernix

abernix Apr 10, 2017
Member

Also, was there a particular reason for using a semi-colon (;) instead of a comma (,) as the delimiter when multiple packages are being excluded?

(Using a semi-colon makes it so the environment variable might have to be quoted on Unix.)

This comment has been minimized.

@wojtkowiak

wojtkowiak Apr 10, 2017
Author Contributor

Also a good point, will be changed to a comma.

var iosCompatibilityVersion = result.cordovaCompatibilityVersions.ios;

// Now exclude one of the plugins from compatibility version calculation.
s.env.CORDOVA_COMPATIBILITY_VERSION_EXCLUDE = 'cordova-plugin-meteor-webapp';

This comment has been minimized.

@abernix

abernix Apr 10, 2017
Member

Would it be difficult to test excluding two packages so the delimiter (;, etc.) is tested?

This comment has been minimized.

@wojtkowiak

wojtkowiak Apr 10, 2017
Author Contributor

No, added. But since there is no easy way of actually checking if those were excluded the only thing you actually test is whether the syntax does not brake the functionality.

run.match("Started your app");

var result = JSON.parse(httpHelpers.getUrl(
"http://localhost:3000/__cordova/manifest.json"));

This comment has been minimized.

@abernix

abernix Apr 10, 2017
Member

This will fail if the tests are run on a different port (e.g. meteor test-packages --port 3500). Can we test this through WebAppInternals.getBoilerplate (similar to this?) or otherwise without an HTTP request?

This comment has been minimized.

@wojtkowiak

wojtkowiak Apr 10, 2017
Author Contributor

I've just used the same approach that was already used https://github.com/meteor/meteor/blob/devel/tools/tests/cordova-hcp.js#L34
meteor test-packages --port 3500 this is not the case because this is self-test and you can not pass a port to it, right?

run.match("Started your app");

result = JSON.parse(httpHelpers.getUrl(
"http://localhost:3000/__cordova/manifest.json"));

This comment has been minimized.

@abernix

abernix Apr 10, 2017
Member

Same as above.

@wojtkowiak
Copy link
Contributor Author

@wojtkowiak wojtkowiak commented Apr 10, 2017

I will squash the commits once we're done with this.

this.cordovaDependencies);

const pluginsExcludedFromCompatibilityHash = (process.env.METEOR_CORDOVA_COMPAT_VERSION_EXCLUDE || '')
.split(',');

This comment has been minimized.

@benjamn

benjamn Apr 19, 2017
Member

This will give [""] rather than [] in the default case; is that what you want?

This comment has been minimized.

@wojtkowiak

wojtkowiak Apr 19, 2017
Author Contributor

Yeah, I know. It's not producing any side effects in _.omit so I'm okay with this. Otherwise, it would look like process.env.METEOR_CORDOVA_COMPAT_VERSION_EXCLUDE ? process.env.METEOR_CORDOVA_COMPAT_VERSION_EXCLUDE.split(',') : []. Or with an extra line to assign this env var to sth shorter 馃檪

@benjamn benjamn added this to the Release 1.4.4.2 milestone Apr 26, 2017
@abernix abernix merged commit fcdf351 into meteor:devel Apr 26, 2017
3 checks passed
3 checks passed
CLA Author has signed the Meteor CLA.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
abernix added a commit that referenced this pull request Apr 26, 2017
* Implement CORDOVA_COMPATIBILITY_VERSION_EXCLUDE and CORDOVA_COMPATIBILITY_VERSION_IOS/ANDROID

CORDOVA_COMPATIBILITY_VERSION_IOS or CORDOVA_COMPATIBILITY_VERSION_ANDROID allows to override compatibility version for a specified platform.

CORDOVA_COMPATIBILITY_VERSION_EXCLUDE provides a way of excluding a certain plugin from compatibility version calculation. You can pass several plugin names with ';'. For example: `CORDOVA_COMPATIBILITY_VERSION_EXCLUDE='cordova-plugin-crosswalk-webview;cordova-plugin-device'`

* Changes after review
@abernix
Copy link
Member

@abernix abernix commented Apr 27, 2017

This should be available to test in the release candidate for Meteor 1.4.4.2, currently 1.4.4.2-rc.1:

meteor update --release 1.4.4.2-rc.1

Please report any problems you encounter!

@wojtkowiak
Copy link
Contributor Author

@wojtkowiak wojtkowiak commented May 4, 2017

Forgot to respond. Works alright! Thanks for accepting this feature!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can鈥檛 perform that action at this time.