conda-build should/could detect already applied patches and issue errors or warnings about them instead of reversing them

[beckermr]

Sometimes a patch will get applied in reverse. I am not 100% clear on when this happens, but it happens pretty silently and is usually not the desired behavior. This behavior can generate bad packages. See eg https://github.com/AnacondaRecipes/conda-build-feedstock/pull/7.

So my questions are

1. Is this the desired behavior?
2. Should we add a flag to turn this off if desired?
3. Do some recipes rely on this?

cc @isuruf

[mingwandroid]

Patching is complicated. Even modern tools like git implement unfortunate behavior WRT patching (git format-patch will not emit the correct line-endings for each block). I had to rewrite the patching code in conda-build quite a lot at 3.20 time.

The reasons were:

In unix-land, all line-endings are LF and everything is fine. On Windows, batch files and VS project files and more often have line-endings ending in CRLF. In general, the old patching code could handle a patch with uniform LF or CRLF just fine; conda-build tries various attempts to get a patch to apply changing line-endings from one type to another and trying again. This generally worked fine. However if it encountered a patch where some bits changed a file with LF and other bits changed a file with CRLF then trouble would ensue. The patch would apply partially, and other bits would not apply. The process would fail and the source tree would be left in an indeterminate state.

That's ok, provided we reverse the patch, so I think what you are seeing is the logs reporting that has happened. The failed patch gets reversed (which correctly deletes .rej files and undoes the bits that managed to get applied). The patch gets reversed and various other attempts are made to make it work.

One thing I took particular care over is to ensure that if a patch is entirely 'native', i.e. it correctly contains patch-blocks that contain LF when appropriate and CRLF when appropriate (i.e. potentially mixed), then it will apply first time without problem.

For a concrete example of a patch that is mixed in LF/CRLF patching-ness (and was therefore conda-build before 3.20 could not apply) see the python 3.8 (master) recipe on AnacondaRecipes.

Maybe I should hide the messages but I'd rather point all this stuff out, there's no need for us to have messy patches.

I have a really poor python script that wraps git format-patch to post-correct the line-endings. It does this by finding the first one in the given source file and doesn't allow for patching blocks in source files that are also mixed in LF / CRLF, but it works for my purposes: https://github.com/AnacondaRecipes/aggregate/tree/master/git-format-patch-le.py

[mingwandroid]

I am not sure what happens in the situation where a patch has already been applied, that could trip up my new code.

I will keep any eye out.

[beckermr]

These subtle things are why I try to ask a lot of questions! Thank you for taking the time to explain this!

[mingwandroid]

Oh it is a pleasure, thank you for your continued excellent work!

[mingwandroid]

Oh, a further "fun" complication is that sometimes we download a .zip on Windows and a .tar.* on Unix and patch them. Sometimes upstream make sure that text files in the .zip releases contain CRLF line endings while those in the .tar.* are 'native'. Fun fun fun! Let's stick to one archive format per source archive!

[mingwandroid]

I'd like to close this unless you think there's a problem here?

[beckermr]

I am not sure what happens in the situation where a patch has already been applied, that could trip up my new code.

Did we want to address this? I think this is the case that caused the issue above w/ conda-build.

That could be a new issue too depending on the actual cause.

[mingwandroid]

I have no evidence that it causes a problem, to the best of my knowledge it does not. It's only a very slight fear given the bug report, but as I say, messages about reversion of patches are not necessarily bad (though they do indicate we should reformat the patch). Happy to keep it open as a placeholder though.

[beckermr]

fair enough - happy to close for now

if I find a more clear example I will let you know!

[mingwandroid]

I've changed the title and re-opened!

[mingwandroid]

I will try to find time to add a test for @isuruf's bug too, but contributions would be welcome as I have a lot of packaging work on my plate ATM.

[beckermr]

I owe you several PRs, including those with the changes to the RPM skeleton. Thank you for your patience with my delay!

[mingwandroid]

Well, I probably owe conda-forge a fair few PRs. I have a fix for Python 3.8 DLL loading on Windows that conda-forge has been hacking around for a while now.

[mingwandroid]

As @isuru has confirmed this is a bug, it's now much higher on my priorities.

[mingwandroid]

(last thing I want is a load of broken packages on my hands!)

[beckermr]

This bug just caused a broken build of conda 4.8.5

https://github.com/conda-forge/conda-feedstock/pull/125

:(

[mingwandroid]

Gah! OK, will see how quickly I can jump onto this.

[mbargull]

The changes also broke a patch with correct line endings I just wrote, see https://github.com/conda-forge/nsis-feedstock/pull/19#issuecomment-696567496. [edit: nvm that, sorry. I made a tab/space error in another file in that patch]
@mingwandroid, in https://github.com/conda/conda-build/issues/4054#issuecomment-692261994 are you describing behavior on Windows? On Linux git format-patch works correctly for me with mixed line ending and I've also added patches with mixed line endings numerous times (which have definitely been applied correctly during a conda-build) before in Bioconda (i.e., only Unix builds). If most of those problems are Windows related, I'd assume conda-build to have an additional Windows code path which does not affect behavior on Unix.
What I expect to happen would just be that patches get applied without changing line endings and never be reverse-applied, i.e., with patch --forward.

[mbargull]

Pew, thankfully it was only me that made an error in the nsis-feedstock PR linked above. And actually, the new behavior even uncovered my user error that would've been unrecognized before!

---

However, my patch --forward argument still stands. I took a glance at the source code and it seems that patch -R has been explicitly added as a fallback in 3.20. Why is that? I'd rather like the build to fail in that case and let the user fix their patch and thus avoid https://github.com/conda-forge/conda-feedstock/pull/125.

[mingwandroid]

On Linux git format-patch works correctly for me with mixed line ending and I've also added patches with mixed line endings numerous times (which have definitely been applied correctly during a conda-build) before in Bioconda (i.e., only Unix builds).

As I said, Git is completely incapable of creating a patch that touches blocks with CRLF along with other blocks (possibly in other files, possibly in the same one) with LF.

Your generated patch will contain LFs (or CRLFs if you've configured things that way).

I explained the logic for all of this and won't repeat it! Please read above carefully.

[mingwandroid]

I cannot simply break what were (previously) somewhat applicable patches (with enough CRLF conversion).

The reversion is necessary so that other attempts at patching can proceed.

That the reversion happens in some other breaking case is a bug that needs to be fixed.

[mingwandroid]

It's not a fallback, it's an attempt to reverse damage that patch does to working trees (which works, in the face of in-complete application of a patch but not in the face of complete non-application of the patch)/

Now perhaps the damage could be undone in a better way, but it needs to be fast. For sure the bug needs fixed and I need to find an hour to look at it soon.

[mingwandroid]

And none of this is Windows specific in any way.

[mbargull]

As I said, Git is completely incapable of creating a patch that touches blocks with CRLF along with other blocks (possibly in other files, possibly in the same one) with LF.

On Linux git format-patch works correctly for me with mixed line ending

$ git init asdf
Initialized empty Git repository in /home/conda/asdf/.git/
$ cd asdf
$ cat >file <<EOF
> qwer
> asdf^M
> zxcv
> ^M
> EOF
$ git add file
$ git commit -m c1
[master (root-commit) 4fc383a] c1
 1 file changed, 4 insertions(+)
 create mode 100644 file
$ cat >file <<EOF
> qwer
> asdf
> zxcv^M
> 
> qwer^M
> EOF
$ git add file
$ git commit -m c2
[master f27fa23] c2
 1 file changed, 4 insertions(+), 3 deletions(-)
$ git format-patch HEAD~1
0001-c2.patch
$ cat -v 0001-c2.patch 
From f27fa23f553ffa271c6de32480c94089965b8e3e Mon Sep 17 00:00:00 2001
From: i <[email protected]>
Date: Tue, 22 Sep 2020 09:55:01 +0000
Subject: [PATCH] c2

---
 file | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/file b/file
index 34d10c0..f1e440d 100644
--- a/file
+++ b/file
@@ -1,4 +1,5 @@
 qwer
-asdf^M
-zxcv
-^M
+asdf
+zxcv^M
+
+qwer^M
-- 
2.28.0

^-- what I'm saying is that it works for me, unlike what you wrote before.

---

And none of this is Windows specific in any way.

That's good to know, thanks!

---

Regarding -R, I can't really follow on when it's needed. Probably some examples, i.e., test cases that require this behavior would make it clearer for me to understand.

[mbargull]

Oh, I didn't correctly read

The reversion is necessary so that other attempts at patching can proceed.

That the reversion happens in some other breaking case is a bug that needs to be fixed.

before. So, you mean that the -R one is just to revert the failed attempt. Okay, that makes sense. And yeah, it's tricky to see when no or partial things have been applied. Now I get it.

[mingwandroid]

what I'm saying is that it works for me, unlike what you wrote before.

It may be some gitconfig I have, I will try to figure that out! My fix was not pleasant to write. I had to inspect git blobs!

I can't remember which patch I was using for this, clearly it needs some testcases too. After (a long!) lunch I will try to tackle this. Sorry for the trouble.

[mbargull]

Wouldn't it suffice to run patch --dry-run beforehand instead of running patch -R afterwards? Or do you know of cases when this wouldn't work?
Do the patch binaries on macOS/Windows that we package have --dry-run?

[mbargull]

My fix was not pleasant to write. I had to inspect git blobs!

Ouch, sorry to hear that, doesn't sound pleasant at all! :/

[mingwandroid]

Dry run does sound like a good idea! Thanks .. now to eat (lots).

[mbargull]

After (a long!) lunch I will try to tackle this.

Thanks!

Thanks .. now to eat (lots).

Me too, enjoy your meal ^^.