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

Set Cordova build number properties #4048

Closed
wants to merge 2 commits into from
Closed

Set Cordova build number properties #4048

wants to merge 2 commits into from

Conversation

@srounce
Copy link

@srounce srounce commented Mar 27, 2015

This PR allows the use of tools such as TestFlight to push multiple builds of the same app version by allowing the setting of a buildNumber property in mobile-settings.json.

Samuel Rounce
@apollo-cla
Copy link

@apollo-cla apollo-cla commented Mar 27, 2015

@srounce: Before we can merge your pull request, you'll need to sign the Meteor Contributor Agreement: https://contribute.meteor.com/

@srounce
Copy link
Author

@srounce srounce commented Mar 27, 2015

Signed.

@srounce
Copy link
Author

@srounce srounce commented Mar 28, 2015

Also, out of interest, where do I add tests for this? Thx

@Slava
Copy link
Member

@Slava Slava commented May 19, 2015

Can you explain how is Date.now() a good value for these properties? I have no idea what it is and how it should work.

@srounce
Copy link
Author

@srounce srounce commented May 19, 2015

It allows developers to specify a build number in their Cordova properties (as explained in the cordova docs), making CI with Cordova (and automated device provisioning) a possibility.

This PR allows one to do as follows:

App.info({
  id: 'company.myapp',
  name: 'MyApp',
  version: '0.2.1',
  buildNumber: process.env.BUILD_NUMBER,
  ...
})

Currently the android-versionCode/ios-CFBundleVersion values are never given a default, so normally user intervention is required in order to submit builds which will pass iTunesConnect validation.
I chose Date.now() as a default, it satisfies the criteria of:

  • Identifying when the build happened
  • Guaranteed to be larger since the last build
  • Not dependent on any environmental factors (bleh - there's no doubt a word for this, but it's late and I'm kinda slow)

Does this answer your questions?

@Slava
Copy link
Member

@Slava Slava commented May 20, 2015

I think this is a great PR, going to merge it with a minor tweak. Thanks.

@Slava Slava closed this May 20, 2015
@Slava
Copy link
Member

@Slava Slava commented May 20, 2015

@srounce
Copy link
Author

@srounce srounce commented May 20, 2015

Yes, good shout on my silly type inference, reads a lot more clearly now.

@Slava
Copy link
Member

@Slava Slava commented May 20, 2015

I am testing right now, this change breaks the builds:

~/w/a/.m/l/cordova-build> cordova build
Running command: /Users/imslavko/work/asdf/.meteor/local/cordova-build/platforms/android/cordova/build
ANDROID_HOME=/usr/local/Cellar/android-sdk/24.1.2
JAVA_HOME=/Library/Java/JavaVirtualMachines/jdk1.8.0_45.jdk/Contents/Home
Running: /Users/imslavko/work/asdf/.meteor/local/cordova-build/platforms/android/gradlew cdvBuildDebug -b /Users/imslavko/work/asdf/.meteor/local/cordova-build/platforms/android/build.gradle -Dorg.gradle.daemon=true

FAILURE: Build failed with an exception.

* Where:
Script '/Users/imslavko/work/asdf/.meteor/local/cordova-build/platforms/android/CordovaLib/cordova.gradle' line: 128

* What went wrong:
A problem occurred evaluating root project 'android'.
> For input string: "1432153134359"

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.

BUILD FAILED

Total time: 1.034 secs

/Users/imslavko/work/asdf/.meteor/local/cordova-build/platforms/android/cordova/node_modules/q/q.js:126
                    throw e;
                          ^
@Slava
Copy link
Member

@Slava Slava commented May 20, 2015

Ugh, after some trial and error, here is my "fix" (or rather a hack) to make it work consistently:

  var defaultBuildNumber = (Date.now() % 1000000).toString();
@srounce
Copy link
Author

@srounce srounce commented May 22, 2015

@Slava I'm a little confused, is the modulo expression due to android not accepting build numbers over a certain size?
If so, perhaps we should use an arbitrary date such as 'Meteor Epoch' ("2012-04-09T23:00:00.554Z"), and subtract that from the current timestamp returned by Date.now(). This would once again allow the default build number to act as an identifier for when the build took place, and guarantee that every build will have a higher build number than those prior (modulo will prevent this by wrapping).
However this approach is just as flawed as we will no doubt run into the android issue again at some point in the future. I'll do some head scratching in a bit and try to figure out a more elegant solution.

@Slava
Copy link
Member

@Slava Slava commented May 22, 2015

@srounce my understanding of the problem is pretty limited, but my believe is - there is some limitation on the string length and it is smaller than a 32bit integer.

@martijnwalraven
Copy link
Contributor

@martijnwalraven martijnwalraven commented Aug 13, 2015

Just came across this issue while trying to fix apps seemingly randomly refusing to install on an Android device.

It turns out this is due to Android not allowing installs with version codes that are lower than the existing one (it fails with INSTALL_FAILED_VERSION_DOWNGRADE). And as @srounce mentions, our current build number calculation doesn't guarantee increasing build numbers because of wrapping.

The problem didn't manifest itself before because cordova-android used the -d switch to force an install anyway, but this was changed in a commit a few months ago (because it isn't supported on Android 4.1.1 and lower): apache/cordova-android@c9e7201

Unless someone has a better idea, I think I'll remove the default build number (while still allowing specifying a buildNumber in App.info).

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

5 participants
You can’t perform that action at this time.