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

Mark 8.0.0-dev & update cordova.js #558

Merged
merged 4 commits into from
Nov 14, 2018
Merged

Conversation

brodybits
Copy link
Contributor

@brodybits brodybits commented Nov 14, 2018

Changes

  • Update cordova.js from cordova-js@4.2.3, which includes User agent check in Cordova.js raises exception in Android 9 #534: Remove obsolete check for JellyBean to work properly on Android Pie
  • Update cordova.js from cordova-js@4.2.4, which includes CB-9366 cordova.js log error.stack
  • Update cordova.js, version, and VERSION to 8.0.0-dev

cordova-coho fork used

I use https://github.com/brodybits/cordova-coho/tree/improve-patch-and-dev-version-support in my fork of cordova-coho, with an enhancement to generate cordova.js from a specific version of cordova-js. This lets me keep the generated updates to cordova.js in separate commits instead of all changes combined together in the same commit.

I proposed this update in apache/cordova-coho#188 but did not have a chance to process the review comments. I hope to process the review comments for apache/cordova-coho#188 to be approved for merge in the near future.

cordova-coho commands for each step

./cordova-coho/coho copy-js -r android --js 4.2.3
  • Update cordova.js from cordova-js@4.2.4, which includes CB-9366 cordova.js log error.stack:
./cordova-coho/coho copy-js -r android --js 4.2.4
  • Update cordova.js, version, and VERSION to 8.0.0-dev:
./cordova-coho/coho prepare-platform-release-branch --version 8.0.0-dev -r android --js 4.2.4

Now merged by push to master

Christopher J. Brody added 4 commits November 14, 2018 15:24
using coho copy-js

with the following change from cordova-js-src:
* Remove obsolete check for JellyBean to work properly on Android Pie
  (apacheGH-534)
(Update cordova.js from cordova-js@4.2.4, using coho copy-js)
@brodybits brodybits merged commit 53e1c1b into apache:master Nov 14, 2018
@brodybits brodybits deleted the mark-8.0.0-dev branch November 14, 2018 20:47
@janpio
Copy link
Member

janpio commented Nov 14, 2018

Why is a PR with failing tests being merged?
Why does this PR contain a lot more than it says in the PR title?
Why does this PR refer to a WIP and unmerged coho PR?

@brodybits brodybits restored the mark-8.0.0-dev branch November 14, 2018 22:15
@brodybits brodybits changed the title Mark 8.0.0-dev Mark 8.0.0-dev & update cordova.js Nov 14, 2018
@brodybits
Copy link
Contributor Author

Why is a PR with failing tests being merged?

It is a build error, no actually failing tests.

I deleted my branch right after merge, that is what caused the failure. I didn't see the failure.

I tried restoring my branch and restarting the build a couple times. Unfortunately it didn't seem to work. It should go green in the next merge.

Why does this PR contain a lot more than it says in the PR title?

I just updated the title.

Purpose was only to mark 8.0.0-dev. But the procedure involves regenerating cordova.js, and I wanted to do that in a couple smaller steps, to avoid excessive changes combined in a single commit.

Why does this PR refer to a WIP and unmerged coho PR?

I have some enhancements that help me update cordova.js from a specific cordova-js version. There is quite a bit of review feedback on the PR and I did not a chance to process it. I just updated the description with some more info.

Alternative would be to close the WIP PR on cordova-coho and use my own fork, without giving record of what commands I used to do the update.

I will admit that this update did not go quite right, not much we can do now.

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

2 participants