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

apply patches with patch files #19451

Merged
merged 1 commit into from
Apr 8, 2024
Merged

apply patches with patch files #19451

merged 1 commit into from
Apr 8, 2024

Conversation

siddarthkay
Copy link
Contributor

@siddarthkay siddarthkay commented Mar 29, 2024

fixes #19449

Summary

In this PR we change the way patches are applied.
We no longer have to write patches in a patch phase like we used to, we can now place individual changes in a patch file inside the patches directory and they will be automatically applied.

Because of this change we can get rid of forks and instead have those changes in patch files.

To generate a patch file this make command can be used make patch-file
This will open an interactive shell which will allow you to specify which file you want to patch and then wait till you make those changes and generate a patch for it.

make patch-file
Configuring Nix shell for target 'default'...
Enter the path of the file to patch: ./node_modules/is-glob/index.js
File to patch: ./node_modules/is-glob/index.js
Temporary directory created: /tmp/tmp-status-mobile-40bc588fa/tmp.xrXarXoTPZ
Original file copied to temporary directory.
Please make the necessary changes to the file: ./node_modules/is-glob/index.js
Press any key when you are done with the changes...
Generating patch file...
Patch file created at /Users/siddarthkumar/code/status-im/PR/status-mobile/patches/index.js.patch
Info: Please execute 'make run-clojure' to test if the patch file works as expected.

Platforms

  • Android
  • iOS

status: ready

@status-im-auto
Copy link
Member

status-im-auto commented Mar 29, 2024

Jenkins Builds

Click to see older builds (54)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 82b7b89 #2 2024-03-29 13:37:59 ~6 min android 🤖apk 📲
✔️ 82b7b89 #2 2024-03-29 13:39:04 ~7 min android-e2e 🤖apk 📲
✔️ 82b7b89 #2 2024-03-29 13:41:42 ~9 min ios 📱ipa 📲
✔️ 99b2d15 #3 2024-03-30 02:54:49 ~4 min tests 📄log
✔️ 99b2d15 #3 2024-03-30 02:58:43 ~8 min android-e2e 🤖apk 📲
✔️ 99b2d15 #3 2024-03-30 02:58:51 ~8 min android 🤖apk 📲
✔️ 99b2d15 #3 2024-03-30 03:00:53 ~10 min ios 📱ipa 📲
✔️ 5d4ecfb #4 2024-04-01 06:19:56 ~4 min tests 📄log
✔️ 5d4ecfb #4 2024-04-01 06:22:32 ~7 min android-e2e 🤖apk 📲
✔️ 5d4ecfb #4 2024-04-01 06:22:49 ~7 min android 🤖apk 📲
✔️ 5d4ecfb #4 2024-04-01 06:25:04 ~9 min ios 📱ipa 📲
✔️ 88273cd #5 2024-04-01 11:51:17 ~4 min tests 📄log
✔️ 88273cd #5 2024-04-01 11:53:06 ~6 min android-e2e 🤖apk 📲
✔️ 88273cd #5 2024-04-01 11:54:05 ~7 min android 🤖apk 📲
✔️ 88273cd #5 2024-04-01 11:56:39 ~9 min ios 📱ipa 📲
✔️ 356722d #6 2024-04-01 16:18:15 ~4 min tests 📄log
✔️ 356722d #6 2024-04-01 16:21:09 ~7 min android-e2e 🤖apk 📲
✔️ 356722d #6 2024-04-01 16:23:05 ~9 min android 🤖apk 📲
✔️ 356722d #6 2024-04-01 16:25:00 ~11 min ios 📱ipa 📲
✔️ 57eab29 #7 2024-04-01 16:58:07 ~4 min tests 📄log
✔️ 57eab29 #7 2024-04-01 16:59:24 ~5 min android-e2e 🤖apk 📲
✔️ 57eab29 #7 2024-04-01 17:00:56 ~7 min android 🤖apk 📲
✔️ 57eab29 #7 2024-04-01 17:02:25 ~8 min ios 📱ipa 📲
✔️ 779e90f #8 2024-04-02 10:43:43 ~5 min tests 📄log
✔️ 779e90f #8 2024-04-02 10:44:10 ~6 min android 🤖apk 📲
✔️ 779e90f #8 2024-04-02 10:46:29 ~8 min android-e2e 🤖apk 📲
✔️ 779e90f #8 2024-04-02 10:47:51 ~10 min ios 📱ipa 📲
✔️ ad15504 #9 2024-04-03 02:52:13 ~4 min tests 📄log
✔️ ad15504 #9 2024-04-03 02:56:11 ~8 min ios 📱ipa 📲
✔️ ad15504 #9 2024-04-03 02:56:33 ~9 min android-e2e 🤖apk 📲
✔️ ad15504 #9 2024-04-03 02:56:35 ~9 min android 🤖apk 📲
✔️ c5ac62d #10 2024-04-03 13:11:24 ~7 min android-e2e 🤖apk 📲
✔️ c5ac62d #10 2024-04-03 13:11:34 ~7 min android 🤖apk 📲
✔️ c5ac62d #10 2024-04-03 13:21:51 ~17 min ios 📱ipa 📲
✔️ 40bc588 #12 2024-04-04 01:07:12 ~4 min tests 📄log
✔️ 40bc588 #12 2024-04-04 01:10:06 ~6 min android-e2e 🤖apk 📲
✔️ 40bc588 #12 2024-04-04 01:10:13 ~6 min android 🤖apk 📲
✔️ 40bc588 #12 2024-04-04 01:14:11 ~10 min ios 📱ipa 📲
✔️ 8c0765c #13 2024-04-04 01:20:38 ~4 min tests 📄log
✔️ 8c0765c #13 2024-04-04 01:24:24 ~8 min android 🤖apk 📲
✔️ 8c0765c #13 2024-04-04 01:24:28 ~8 min android-e2e 🤖apk 📲
✔️ 8c0765c #13 2024-04-04 01:29:32 ~13 min ios 📱ipa 📲
✔️ 70b5564 #14 2024-04-05 06:01:58 ~3 min tests 📄log
✔️ 70b5564 #14 2024-04-05 06:05:30 ~7 min android-e2e 🤖apk 📲
✔️ 70b5564 #14 2024-04-05 06:05:36 ~7 min android 🤖apk 📲
✔️ 70b5564 #14 2024-04-05 06:06:54 ~8 min ios 📱ipa 📲
✔️ 923652e #16 2024-04-08 11:00:14 ~4 min tests 📄log
✔️ 923652e #16 2024-04-08 11:04:19 ~8 min android-e2e 🤖apk 📲
✔️ 923652e #16 2024-04-08 11:04:26 ~8 min android 🤖apk 📲
✔️ 923652e #16 2024-04-08 11:14:36 ~19 min ios 📱ipa 📲
✔️ af66bd6 #17 2024-04-08 11:43:52 ~4 min tests 📄log
✔️ af66bd6 #17 2024-04-08 11:47:35 ~8 min android-e2e 🤖apk 📲
✔️ af66bd6 #17 2024-04-08 11:47:45 ~8 min android 🤖apk 📲
✔️ af66bd6 #17 2024-04-08 11:51:44 ~12 min ios 📱ipa 📲
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 4b4e499 #18 2024-04-08 14:25:38 ~4 min tests 📄log
✔️ 4b4e499 #18 2024-04-08 14:28:18 ~7 min android-e2e 🤖apk 📲
✔️ 4b4e499 #18 2024-04-08 14:28:25 ~7 min android 🤖apk 📲
✔️ 4b4e499 #18 2024-04-08 14:41:42 ~20 min ios 📱ipa 📲
✔️ 1a56dc2 #19 2024-04-08 15:30:05 ~4 min tests 📄log
✔️ 1a56dc2 #19 2024-04-08 15:32:40 ~6 min android 🤖apk 📲
✔️ 1a56dc2 #19 2024-04-08 15:32:55 ~7 min android-e2e 🤖apk 📲
✔️ 1a56dc2 #19 2024-04-08 15:36:24 ~10 min ios 📱ipa 📲

@@ -4,16 +4,17 @@

{ stdenv, deps, nodejs, patchMavenSources }:

let
patchesDir = ./../../../patches;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wondering if should I move patches directory inside nodejs-patched or keep it in root 🤔
In most projects patches remain in root

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe $GITROOT could be used to make this neater?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, Nix paths have to be relative here. I don't mind them being at repo root, but they would also make sense here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by here do you mean instead of status-mobile/patches we could store patches in status-mobile/nix/deps/nodejs-patched/patches ?
I don't mind actually, I'd be more than happy to get rid of the "./../../../" from the patchesDir variable :D

@siddarthkay siddarthkay force-pushed the patch-libs-with-nix branch 7 times, most recently from ad15504 to c5ac62d Compare April 3, 2024 13:04
Copy link
Member

@jakubgs jakubgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good, but it would be better if we had a script or at least a README.md explaining how these patch files should be created.

@@ -4,16 +4,17 @@

{ stdenv, deps, nodejs, patchMavenSources }:

let
patchesDir = ./../../../patches;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, Nix paths have to be relative here. I don't mind them being at repo root, but they would also make sense here.

@siddarthkay
Copy link
Contributor Author

script or at least a README.md

Thanks!
Script sounds fun, I have the manual method in my notes but will try a swing at a bash script.
If it takes a while I'll settle for a README.md and do a script as a follow up PR.

@siddarthkay siddarthkay force-pushed the patch-libs-with-nix branch 3 times, most recently from 8c0765c to 70b5564 Compare April 5, 2024 05:57
@siddarthkay siddarthkay requested a review from a team April 5, 2024 06:11
@siddarthkay siddarthkay force-pushed the patch-libs-with-nix branch 4 times, most recently from af66bd6 to 4b4e499 Compare April 8, 2024 14:20
fixes #19449

In this commit we change the way patches are applied.
We no longer have to write patches in a patch phase like we used to, we can now place individual changes in a patch file inside the `patches` directory and they will be automatically applied.

Because of this change we can get rid of forks and instead have those changes in patch files.

To generate a patch file this make command can be used `make patch-file`
This will open an interactive shell which will allow you to specify which file you want to patch and then wait till you make those changes and generate a patch for it.

```
make patch-file
Configuring Nix shell for target 'default'...
Enter the path of the file to patch: ./node_modules/is-glob/index.js
File to patch: ./node_modules/is-glob/index.js
Temporary directory created: /tmp/tmp-status-mobile-40bc588fa/tmp.xrXarXoTPZ
Original file copied to temporary directory.
Please make the necessary changes to the file: ./node_modules/is-glob/index.js
Press any key when you are done with the changes...
Generating patch file...
Patch file created at /Users/siddarthkumar/code/status-im/PR/status-mobile/patches/index.js.patch
Info: Please execute 'make run-clojure' to test if the patch file works as expected.
```

- Android
- iOS
@siddarthkay siddarthkay merged commit ce69df1 into develop Apr 8, 2024
6 checks passed
Pipeline for QA automation moved this from REVIEW to DONE Apr 8, 2024
@siddarthkay siddarthkay deleted the patch-libs-with-nix branch April 8, 2024 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Implement a new mechanism of applying patches to existing libraries
3 participants