patch: Switch to using Python code

[mingwandroid]

@msarahan, @jakirkham what do you think?

patch.exe from gnuwin32 has fallen over on several of my bigger
patches now and it's becoming a serious impediment.

I don't want to add MSYS2's patch.exe since that requires a lot
of infrastructure to work correctly (though I could build a native
Windows patch.exe if necessary).

However rather than relying on some arbitrary patch executable,
it's better to use Python to do the work and avoid the subprocess
overhead (though there will be a native-vs-python difference).

Also add automatic patch skip level detection logic, which allows
us to directly use patches generated using git format-patch and
also those authored by external entities without needing to add
metadata (although this may not be a bad idea). This part could be
moved into patch.py at a later point perhaps?

I've not added tests for this since upstream has a comprehensive
testsuite (and I did add tests to check my changes to my PR).

Upstream: https://github.com/techtonik/python-patch
Pull Req: techtonik/python-patch#41

[jakirkham]

Moving to Python sounds great to me. There were lots of annoyance to using patch on Unix platforms. We just need to make sure that this code is really well tested. I was looking earlier to see if there were any decent libraries for patching. Unfortunately there are only refs to difflib, which doesn't cut it. Other than that there is python-patch, which is actually Python and C++ so that's not great for adding to conda-build, but it could be useful for inspiration and tests. There's also whatthepatch, but it is still very young. Though it does have a decent number of features, quite a few test cases, and is pure Python. In any event, both of those are MIT so that should aid in reusability.

[mingwandroid]

@jakirkham https://github.com/techtonik/python-patch doesn't use any C++ and (as the PR states) is the upstream for this :-)

It also has quite a comprehensive (if somewhat fiddly to add to) testsuite. But I agree that it needs testing and more testing, so please, go to town.

[ccordoba12]

@mingwandroid, we were using python-patch before moving to patch.exe. If I recall correctly, @pelson (among others) asked for this change. So maybe he has something to say about it :-)

[jakirkham]

doesn't use any C++ and (as the PR states) is the upstream for this :-)

I see. I didn't look too closely. Maybe GitHub was just pointing out the sample code for patch tests.

[msarahan]

I don't remember anyone asking to use patch.exe. IIRC, I did so because the old one was failing miserably in my efforts to add python 3.5 and all its packages to Anaconda.

[msarahan]

I'm OK with this addition, but I would like some tests to accompany it. What I have in mind is a project with some dummy source code, and a patch that applies to that source code. We need to parse or examine the patched output to ensure that the patch applied cleanly.

I guess we need patches both with p0 and p1 (without and with prefix, respectively), just for completeness?

[mingwandroid]

For the first one, can I copy a test from upstream? They have extensive tests for this sort of thing.

The patch strip level detection code needs a new test for sure (actually it should be able to detect -p too).

.. though actually I might make a single test that covers a few features (creation, deletion and patch level determination).

.. but first, I need to figure out what's up with the CI on this.

[msarahan]

Copying is A-OK, but any testing here should cover our use of patch.py, not
just the functionality of patch.py. Integration tests, of sorts, more than
unit-tests of the patch functionality.

On Tue, Jun 7, 2016 at 9:39 AM Ray Donnelly notifications@github.com
wrote:

For the first one, can I copy a test from upstream? They have extensive
tests for this sort of thing.

The patch strip level detection code needs a new test for sure (actually
it should be able to detect -p too).

.. but first, I need to figure out what's up with the CI on this.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1006 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AACV-d34oiO8FXR07wfrRjocnZrr-XNWks5qJYKvgaJpZM4Iv6Lp
.

[ilanschnell]

Is the Python patch only for Windows? Also, I would argue that if we move to a different patch program, we should only change the dependency, and package patch separately. I think conda-build should not be tied to a specific patch implementation (in the same way that conda-build doesn't include git, patchelf, install_name_tools, etc. , although it uses it).

[mingwandroid]

@ilanschnell python-patch is for all systems, it is pure python and it works on 2.7.x and 3.x.

I've split this PR two bits, one that I hope will be merged soon (automatic detection of patch strip level): #1011, the other bit I don't plan to push for inclusion at present (see later).

IMHO the patch executables in the wild are too variable. For example, gnuwin32's patch.exe crashes on completely valid patches. There are pros and cons to using Python implementations of the tools we depend on: not needing to use subprocess, consistency of operation, python's speed versus some-other-language's speed, maintainability, amount of code, and other things I've missed.

Therefore, the decision of whether to import a python implementation of a given tool needs to be done on a case-by-case basis and I think a (working) python-patch would be a good thing (provided the code isn't too large, which in this case, I don't think it is). Using different patch implementations could generate different source code and that could result in horrible to track down bugs. The other major issue with gnu patch executables is that they are always interactive on patch failure which breaks CI systems. It's interesting you mention install_name_tool since broken or incompatible versions of that tool appear to be hurting us on OS X.

That being said, python-patch can't handle offsets or fuzzing, both of which are features we need, so for that reason I am closing this request, but I'd be interested in carrying on the conversation about whether continuing on this path (by implementing offsets and fuzzing) in future is worthwhile or not.

[ilanschnell]

Thanks for your response, Ray. I've never had any problems with patch on Unix. It sounds like all your concerns could be equally addressed by creating a python-patch package, and have conda depend on that. I wold be in favor of this approach.

[mingwandroid]

That approach would also work for me, thanks Ilan.

[msarahan]

@mingwandroid, ok to close this?

[jezdez]

Closing since it's been a while.