search

nodejs-mobile / nodejs-mobile

Full-fledged Node.js on Android and iOS
https://nodejs-mobile.github.io
Other
466 stars 46 forks source link

Rebase on Node v16.13.2 #3

Closed gmaclennan closed 1 year ago

gmaclennan commented 2 years ago

Here is a possible strategy for approaching this. I tried merging the upstream v16.13.2 branch into mobile-master but it's very hard to apply cleanly because of the changes both upstream and here, so I think even with resolving all the conflicts it would miss things.

As an alternative strategy I suggest we manually rebase onto upstream v16.13.2. We could approach this by:

  1. Cherry-pick each commit since node-upstream-v12.16.0 (this is when this repo diverged from Node upstream)
  2. Apply the diff upstream-node-v12.19.0...mobile-master in one go and try to resolve changes (this should be the diff between upstream node v12.19.0 and what is in mobile-master).
  3. Apply the above diff file-by-file resolving changes as we go.

I think (3) might be the best approach, because it allows us to go through this incrementally and push changes as we go. It might make sense to afterwards squash some commits so that we have a clean commit history. This is what I have started in this PR. In order to apply a single file from the above patch this is what I did:

git apply --include=common.gypi --whitespace=fix --reject upstream-node-v12.19.0...mobile-master.diff

Files that are modified or deleted should be relatively quick and easy, so that leaves 64 modified files that probably need to be reviewed manually. I have generated a task list from the files in the diff below so that we can work through it step-by-step. After we have cleanly applied this diff, then we can start working on fixes required to get everything working.

Modified

Added

Deleted

staltz commented 2 years ago

@gmaclennan Interesting approach! Let's say this works and we're ready to ship, what would we do with the branch mobile-master (which is currently the default branch)? Also I understand how we can merge a PR into a branch, but it seems that this PR is targeting a tag, how does that work?

Should we make a new branch called master-v16 which coincides with the tag upstream-node-v16.13.2 and then target this PR on that branch? Then, we can probably swap (in GitHub settings) the default branch from mobile-master to master-v16. (And as a general strategy in the future, the default branch would be master-vX for the nodejs-mobile that's compatible with Node.js version X). This way we can keep the history of commits on older versions of nodejs-mobile, but they would be tucked away in parallel branches. (I'm trying to find a way where we don't need to rewrite history or break HTTP links to commits on this repo)

staltz commented 2 years ago

Oh, I also recommend that we start versioning nodejs-mobile to indicate the respective version of Node.js. So Node.js 16.13.2 => nodejs-mobile 16.x.y, or ... nodejs-mobile 16.13.2-x

gmaclennan commented 2 years ago

Good questions!

I actually created upstream-node-v16.13.2 as a branch, based on the upstream tag v16.13.2, then created a new branch (nodejs-mobile-v0.4.0) where I'm applying my commits.

In terms of steps if this works, one approach I can think of:

Maybe worth renaming the branch I'm doing the PR against to just upstream-node, and then we can track upstream with that (latest LTS), so then when a merge isn't working then we just rebase this PR onto the latest upstream-node.

Open to other thoughts and ideas about how to do this though!

Side-note: I wonder if any of these changes might be accepted upstream as commits to Node? They suggest they are open to improvements to the Android build at least. That would reduce the work we need to do here. Also maybe we can reset this repo not be a fork, so that PRs don't default to opening against janeasystems/nodejs-mobile.

staltz commented 2 years ago

Sounds good!

  • For minor Node releases, it's probably easiest to just merge in changes from upstream

Could we just update to the next LTS? I think they release an LTS fairly often, and they release minor versions too often which would make it more work for us to keep up with them. On the other hand if they have critical bug fixes or security patches, I agree we should pick those.

Also maybe we can reset this repo not be a fork, so that PRs don't default to opening against janeasystems/nodejs-mobile.

I actually don't know how to do this! If it is even possible. Couldn't find anything in the repo settings.

gmaclennan commented 2 years ago

Could we just update to the next LTS? I think they release an LTS fairly often, and they release minor versions too often which would make it more work for us to keep up with them. On the other hand if they have critical bug fixes or security patches, I agree we should pick those.

Yeah, agreed.

I actually don't know how to do this! If it is even possible. Couldn't find anything in the repo settings.

I've done it before years back and it required writing to Github support, but it was quick and easy.

gmaclennan commented 1 year ago

I was looking into how Electron maintains its patched version of Node, and it's an interesting option: https://github.com/electron/electron/blob/main/docs/development/patches.md

Rather than having a fork of nodejs in this repo, this becomes instead just a folder of patches that are applied. This could perhaps make maintenance and upgrades easier?

staltz commented 1 year ago

@gmaclennan Very good point! I support that idea, and since we anyway were going to restructure the git branches in this repo, might be worth doing this patch system.

Do we included nodejs as git submodule, and then a folder for the patches?

nodejs-mobile
├── nodejs/node (git submodule)
└── patches
gmaclennan commented 1 year ago

Do we included nodejs as git submodule, and then a folder for the patches?

I haven't used git submodules before (so don't know much about them), but this seems like a reasonable option. I looked into how Electron does this, and it uses a complicated setup of Chromium's Depot Tools and GN for downloading Node source code and applying patches. I don't think we want something as complicated as that!

staltz commented 1 year ago

On a video call with @achou11, we were discussing how to make progress on the nodejs 16 update. There's the "uphill" (strenuous) part of this issue, and there's the "downhill" (straightforward) part. The uphill is figuring out what git approach to use in this repo to incorporate upstream node.js updates, and the downhill is fixing individual git conflicts AND fixing specific bugs until all nodejs-mobile tests pass on iOS / Xcode.

We outlined 4 approaches for the uphill:

  1. Cherry-pick all upstream commits into this repo
  2. Merge commit from the upstream into this repo
  3. Git submodule for node.js upstream inside this repo, plus a folder with patch files
  4. Squash all commits from node 12.19.0 to node 16.x.y as one huge commit, and then merge commit that into this repo

Approach 4 is something that just occurred to me while meeting Andrew, and I like it. The uphill challenge is handling the jungle of commits from upstream, and if we reduce the number of commits to 1, it'll make our repo easier to navigate from git.

The problem with 4 is that we have to resolve all git conflicts in one commit, on one developer's machine. After that merge commit, we may be able to distribute the downhill tasks among developers to make tests pass in Xcode.

cc @gmaclennan @jrmeurer

therealjmj commented 1 year ago

Great idea. You might also try squashing each major version instead of the entire 12-16 block as one. Like squash and merge 13 first. Just a thought.

gmaclennan commented 1 year ago

It's taken me a while to respond on this since I finally succumbed to the dreaded COVID, but better now! When I started trying to do the merge, I think it's really hard to do it by merging upstream into here, because its hard to figure out which side of each merge conflict to choose: is a change from upstream compared with this repo because of a fix/improvement upstream, or is it because of a patch that was in this repo to make the build work on mobile?

Since there are not many changes / patches here, I think it's much easier to do the merge the other way around, e.g. rebase or cherry pick the commits in this repo onto latest node. There aren't actually many patches: the handful of commits here by Jaime are directly on Node 12.16.0: https://github.com/nodejs-mobile/nodejs-mobile/commits/mobile-master?before=e64924271a509a2808ae8010b4c6ddc93688bdbe+1788&branch=mobile-master&qualified_name=refs%2Fheads%2Fmobile-master

As I said above I think the easiest way to do the rebase in steps is to go through file-by-file using the steps above. After that then we could look at merging the commits (since we will have a commit for each file / group of files), and then from there we can turn them into patches. Where I think it would be useful to get to is having a handful of commits that apply the patches necessary to build for Android and iOS, so that when a new version of Node comes out we can just rebase onto that, rather than trying to merge Node upstream onto here, which I think will be much more difficult.

I can spend a bit of time trying to do this again. I think it's just a case of reviewing merge conflicts file-by-file and resolving.

staltz commented 1 year ago

@gmaclennan Not sure if you know this already, but I think Jaime made more than a handful of commits: https://github.com/nodejs-mobile/nodejs-mobile/commits?author=jaimecbernardo

staltz commented 1 year ago

Closing this PR because #9 has been merged