Rebase on Node v16.13.2#3
Conversation
Rejected chunk that could not be applied because without_snapshot is
no longer an option (for mobile we want to build with snapshots I think)
@@ -1057,7 +1059,7 @@ def configure_node(o):
else target_arch != host_arch)
want_snapshots = not options.without_snapshot
o['variables']['want_separate_host_toolset'] = int(
- cross_compiling and want_snapshots)
+ cross_compiling)
Still need to resolve merge issues in: - deps/uv/src/unix/fs.c - deps/uvwasi/src/uvwasi.c - deps/v8/src/base/platform/platform-posix.cc Suggest applying each file from diff and manually resolving rejected chunks
|
@gmaclennan Interesting approach! Let's say this works and we're ready to ship, what would we do with the branch Should we make a new branch called |
|
Oh, I also recommend that we start versioning nodejs-mobile to indicate the respective version of Node.js. So Node.js |
|
Good questions! I actually created 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 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 |
|
Sounds good!
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.
I actually don't know how to do this! If it is even possible. Couldn't find anything in the repo settings. |
Yeah, agreed.
I've done it before years back and it required writing to Github support, but it was quick and easy. |
|
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? |
|
@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? |
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! |
|
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:
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 |
|
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.
|
|
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. |
|
@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 |
|
Closing this PR because #9 has been merged |
3 participants
Here is a possible strategy for approaching this. I tried merging the upstream v16.13.2 branch into
mobile-masterbut 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:
node-upstream-v12.16.0(this is when this repo diverged from Node upstream)upstream-node-v12.19.0...mobile-masterin one go and try to resolve changes (this should be the diff between upstream node v12.19.0 and what is inmobile-master).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:
--rejectcreates a file.rejwith chunks that could not be applied--whitespace=fixfixes whitespace differences instead of just failing because of them--include=<filepath>is the file to apply from the diffFiles 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