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

Patching doesn't work #11

Closed
migzai opened this issue Aug 23, 2017 · 32 comments

Comments

Projects
None yet
3 participants
@migzai
Copy link

commented Aug 23, 2017

When patch-package is run.. the target files remain intact. I did some debugging and it turns out that 'git apply -v file.patch' is skipping patches.

Skipped patch 'node_modules/react-native-ssdp/lib/client.js'.
Skipped patch 'node_modules/react-native-ssdp/lib/index.js'.
Skipped patch 'node_modules/react-native-ssdp/node_modules/react-native-udp/UdpSocket.js'.

Not sure why. This happens on Windows and MacOS.

@ds300

This comment has been minimized.

Copy link
Owner

commented Aug 23, 2017

Thanks for the report. Do you mind sharing your patch files so I can debug this?

EDIT

If you've landed here from google, try the v6 pre release branches patch-package@beta

@migzai

This comment has been minimized.

Copy link
Author

commented Aug 23, 2017

Sure. Attached the patch file.

react-native-ssdp+2.7.5.zip

@ds300

This comment has been minimized.

Copy link
Owner

commented Aug 23, 2017

Cool thanks. I'm quite busy until Saturday but will try to make some time to look at this before then, it's a high priority for me.

@ds300

This comment has been minimized.

Copy link
Owner

commented Aug 23, 2017

Looks like it might be related to patching nested dependencies.

@migzai can you run

patch -p1 -i patches/react-native-ssdp+2.7.5.patch --verbose --dry-run

on MacOS and paste the output here please?

@migzai

This comment has been minimized.

Copy link
Author

commented Aug 23, 2017

Here's the output of patch -p1 -i patches/react-native-ssdp+2.7.5.patch --verbose --dry-run

Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|diff --git a/node_modules/react-native-ssdp/lib/client.js b/node_modules/react-native-ssdp/lib/client.js
|index 942b3f0..74c429d 100644
|--- a/node_modules/react-native-ssdp/lib/client.js
|+++ b/node_modules/react-native-ssdp/lib/client.js
--------------------------
Patching file node_modules/react-native-ssdp/lib/client.js using Plan A...
Hunk #1 succeeded at 50.
Hmm...  The next patch looks like a unified diff to me...
The text leading up to this was:
--------------------------
|diff --git a/node_modules/react-native-ssdp/lib/index.js b/node_modules/react-native-ssdp/lib/index.js
|index b90feb7..39905be 100644
|--- a/node_modules/react-native-ssdp/lib/index.js
|+++ b/node_modules/react-native-ssdp/lib/index.js
--------------------------
Patching file node_modules/react-native-ssdp/lib/index.js using Plan A...
Hunk #1 succeeded at 11.
Hunk #2 succeeded at 135.
Hmm...  The next patch looks like a unified diff to me...
The text leading up to this was:
--------------------------
|diff --git a/node_modules/react-native-ssdp/node_modules/react-native-udp/UdpSocket.js b/node_modules/react-native-ssdp/node_modules/react-native-udp/UdpSocket.js
|index 73dca9f..d30bcd1 100644
|--- a/node_modules/react-native-ssdp/node_modules/react-native-udp/UdpSocket.js
|+++ b/node_modules/react-native-ssdp/node_modules/react-native-udp/UdpSocket.js
--------------------------
Patching file node_modules/react-native-ssdp/node_modules/react-native-udp/UdpSocket.js using Plan A...
Hunk #1 succeeded at 83.
done
@ds300

This comment has been minimized.

Copy link
Owner

commented Aug 23, 2017

Thanks. Looks fine. Must be git-specific then. Any chance you could share your repo with me? Would make it a lot easier to debug.

@migzai

This comment has been minimized.

Copy link
Author

commented Aug 23, 2017

Attached a test case..

test.zip

@migzai

This comment has been minimized.

Copy link
Author

commented Aug 23, 2017

Yes this happens if the project uses git and the .git folder isn't at the same level as the patches directory.

@ds300

This comment has been minimized.

Copy link
Owner

commented Aug 23, 2017

Can confirm. Thanks.

This project was using the unix patch command up until recently. We switched to git apply for the windows support, but it seems that introduces its own problems. On initial inspection of git-apply, there doesn't seem to be a cli option to work around this. Will have to look into some other option for windows.

As a temporary workaround, version 3.3.1 should work on windows with git bash.

@ds300 ds300 closed this in 375f16e Aug 23, 2017

@ds300 ds300 reopened this Aug 23, 2017

@ds300

This comment has been minimized.

Copy link
Owner

commented Aug 23, 2017

I think I've fixed this now in version 3.3.4, by using the --unsafe-paths option to git apply Could you check it works on your end, @migzai ? I don't have Windows testing facilities.

@migzai

This comment has been minimized.

Copy link
Author

commented Aug 24, 2017

I can confirm that this doesn't work on Windows still. Git is 'Skipping patches'.

@ds300

This comment has been minimized.

Copy link
Owner

commented Aug 24, 2017

💩 I'll look in to this some more on saturday

@migzai

This comment has been minimized.

Copy link
Author

commented Aug 28, 2017

Hi. Any progress on this so far? Thanks

@ds300

This comment has been minimized.

Copy link
Owner

commented Aug 28, 2017

Sorry I didn't make any useful progress on it on saturday. I'll have some time this evening to try again.

@ds300

This comment has been minimized.

Copy link
Owner

commented Aug 28, 2017

Progress update: this is definitely an inconsistency in the way git apply works on different platforms. Windows git ignores the facet of --unsafe-paths that says the files don't have to be in the index, and will silently skip them if it can't find them in the index. Apparently there is an undocumented --no-index flag, but that doesn't seem to help either, at least on the windows machine I'm using. I'm going to do two things:

  • check for silently skipped files
  • if files were silently skipped and we're running on windows and the git root folder is not the same as the package.json, make a copy of the patch file with the named file paths resolved relative to the root of the git repo, and apply that instead.
  • otherwise throw an error

This should be a forwards-compatible workaround, in case windows git ever aligns its behaviour across platforms.

@ds300

This comment has been minimized.

Copy link
Owner

commented Aug 28, 2017

😕 Now I'm seeing the same behaviour on MacOS so it must be that my testing env was not set up properly when I thought I'd fixed it last week. At least that means git is consistent :)

@ds300

This comment has been minimized.

Copy link
Owner

commented Aug 28, 2017

@migzai Pretty sure this is fixed now. Tested on windows and everything! Do you mind confirming 3.3.5 solves your issue?

@ds300 ds300 added the bug label Aug 28, 2017

@migzai

This comment has been minimized.

Copy link
Author

commented Aug 29, 2017

Hi. Just tested 3.3.5 on Windows. Still getting the same behavior as before..

@ds300

This comment has been minimized.

Copy link
Owner

commented Aug 29, 2017

😧 Even with the test repo you attached?

@migzai

This comment has been minimized.

Copy link
Author

commented Aug 29, 2017

Yes..

@ds300

This comment has been minimized.

Copy link
Owner

commented Aug 29, 2017

My apologies. I'll double check my tests next time I have access to the windows laptop, which will be on Sunday. Sorry again for the inconvenience. Could have sworn this was working 😕

@calvin337

This comment has been minimized.

Copy link

commented Aug 31, 2017

Same issue here, works on node modules in the same folder as where the .git folder is but doesn't work for any sub-directories.

@ds300

This comment has been minimized.

Copy link
Owner

commented Sep 1, 2017

Thanks @calvin337 — is that also for windows?

@calvin337

This comment has been minimized.

Copy link

commented Sep 1, 2017

Yeah also for Windows, it works fine on Mac.

@ds300

This comment has been minimized.

Copy link
Owner

commented Sep 3, 2017

@migzai @calvin337 just released 3.3.6 - again pretty sure this fixes the issue. Third time's a charm 🤞 :D

@migzai

This comment has been minimized.

Copy link
Author

commented Sep 4, 2017

I can confirm that this now works on Windows. Thanks! There's an issue though on Linux when I attempt to run patch-package:

Patch was made for version 0.10.8
Meanwhile node_modules/react-native-fetch-blob is version 0.10.8

Something not logical going on and the patch-package fails with exit code 1.
patch-package.zip

@ds300

This comment has been minimized.

Copy link
Owner

commented Sep 4, 2017

Thanks for the log. I'll take a look tonight.

@ds300

This comment has been minimized.

Copy link
Owner

commented Sep 4, 2017

Can't reproduce this issue on ubuntu with the test repo. @migzai any chance you could zip and attach the patch file for react-native-fetch-blob?

@migzai

This comment has been minimized.

Copy link
Author

commented Sep 5, 2017

@ds300

This comment has been minimized.

Copy link
Owner

commented Sep 5, 2017

Thanks. Still can't reproduce the issue with your test repo, unfortunately. I added the patch file into the patches directory, upgraded patch-package, and added react-native-fetch-blob. The patch was applied as expected on Ubuntu 17.04 with Git 2.11.0

Which linux/git are you on? Did you try deleting node_modules and trying again? Any other help you could give to reproduce the issue would be most appreciated 🙇

@ds300

This comment has been minimized.

Copy link
Owner

commented Sep 29, 2017

@migzai Not sure if you're still suffering from this problem, but thanks to #21 we figured out that git can enforce particular whitespace rules for patch files, which should make them work across platforms seamlessly. 🤞

All you need to do is add one line in .gitattributes, see the linked issue for details. Let me know if that solves your problem, or maybe you're not using patch-package anymore in which case feel free to ignore this and I'll close the issue due to inactivity in a few days.

Thanks for your help and collaboration!

@ds300

This comment has been minimized.

Copy link
Owner

commented Oct 2, 2017

Closing due to inactivity, as discussed. Please re-open if you find the problem has not been solved.

@ds300 ds300 closed this Oct 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.