Conda-build: Cannot patch file with CRLF line endings

[xflr6]

Unfortunately, the only way to patch a file with CRLF line endings seems to be using the --binary option with a CRLF patch file.

If this option cannot be added to meta.yaml quickly, how about adding the --binary switch by convention: if a patch filename ends with e.g. .patch-binary here (also making sure to never apply _ensure_unix_line_endings in this mode)?

[jakirkham]

What do you think about this, @mingwandroid?

[msarahan]

Should be fine. I'm happy to merge such a PR - seems like @rolando did some of that work at https://github.com/conda/conda-build/pull/2035

Sorry I haven't had more time to work on this.

[msarahan]

fixed by #2035

[xflr6]

I don't think adding --binary unconditionally under Windows solves this: This still gives us no platform-independent way to apply a CRLF patch file to a CRLF file. Please reconsider my proposal for doing it on all platforms if (and only if) the patch file ends with .patch-binary (and don't change line endings on that branch).

As I learned, patching a CRLF file with a LF patch actually did work under Windows before: The patch binary (the one included with git-for-windows, and apparently the one used by AppVeyor) is more forgiving and apparently does some CR conversion out of the box (see this build). So the current change might break patching under Windows: Please consider putting together a toy feedstock to verfiy this all.

[xflr6]

Correction: I think the linked Windows build applies a CRLF instead of a LF patch file (the git checkout of the patch file in auto text mode probably gets CRLF line endings under Windows).

Note that you might need text eol=crlf in .gitattributes to consistently checkout a CRLF patch file on all platforms.

[blankclemens]

Why is this closed? I might be wrong, but #2035 only fixes this on Windows, right? But the key issue is: How to fix a CRLF file with a patch on a Unix system?

[mingwandroid]

Can you not just fix your broken patch?

[blankclemens]

How? It doesn't matter if the patch is LF or CRLF.

[mingwandroid]

Yes it does, on Unix CRLF is nonsensical from a newline perspective and patches are really 'binary' files (here I mean they need to work on a vast range of code-points for things like UTF-8 so they are not 'ASCII' files) so having a CR in your patch file should absolutely not match an input which does not have the CR in the same place. CRLF is an artefact of incorrect patch generation on Windows.

To fix them you should run dos2unix or d2u on the patch file.

[blankclemens]

Thanks for your help! Sadly, it does not seem to solve the issue, instead there are two options:

• convert all of the files to use LF endings, or
• convert all of the files to use CRLF endings and add the --binary option

The first one would require to change the source code prior to patching, e.g. in the build.sh, plus a tool like dos2unix to exist in the build environment - quite overkill and this tool does not seem to be in the main Conda repositories.

The second one would require the option to set the --binary flag for patches in Conda - and I think that's exactly what's been asked for (e.g. through a .patch-binary file ending), would solve the issue and wouldn't require any additional work and tools in the build.sh.

[mingwandroid]

• convert all of the files to use LF endings, or

The first one would require to change the source code prior to patching, e.g. in the build.sh, plus a tool like dos2unix to exist in the build environment - quite overkill and this tool does not seem to be in the main Conda repositories.

I am not suggesting to make this fix dynamically, I am suggesting that you fix your source repository once and for all, and then if anyone ever tries to commit broken line endings in future, you reject that change.

[blankclemens]

Ah, there might be our misunderstanding, I cannot change the source code. I mean, if I could change the source repository, why would I need a patch at all? ;-)

[mingwandroid]

The problem with any special casing of CRLF is that you run the risk to break any instances of UTF-8 code-points that contain those characters consecutively as the process will misinterpret the UTF-8 code-point asCRLF.

I cannot change the source code

I disagree with this assertion. If you can access the source code (which is a requirement for being able to patch or build the package) then you can modify it and upload it elsewhere.

[blankclemens]

You're right, I could do just that. On the other hand that's the same league as adding some dos2unix or likewise magic to the build.sh, just because Conda doesn't allow me to use the --binary flag when patching.

So basically the issue remains: "Cannot patch file with CRLF line endings" (on Unix).

[mingwandroid]

I already explained that I am opposed to this hack as it is incorrect and pays no attention to UTF-8. Fixing the source (pun intended) of the problem is the correct fix.

On this note, for the same reasons I'd prefer that we removed _ensure_unix_line_endings.

[mingwandroid]

At the end of the day this is @msarahan's call but it is the wrong fix and muddies the water in terms of how line endings should be done.

The end result is that we end up with .patch_binary files as well as .patch files and that's really horrible.

Tooling infrastructure will need to be changed to accommodate these two extensions, just because people are generating broken patches from broken source repositories. Source files should always be stored in Unix form and patches should always be created against and applied to files of that form.

[blankclemens]

That's certainly understandable, although I don't agree totally - it wouldn't change anything for all existing packages, instead it would just make an option that is already there (in the patch tool) accessible for people who know that they want to use this option. I don't like a filename approach too much though, I would prefer some tag in the yaml file, e.g. as with the skip behavior.

Regardless, I do agree that in a perfect world there would be no need for this. Thank you very much for all the explaining!

[xflr6]

I agree with @blankclemens. Please do not consider a source file as broken only because it has different line endings than on *nix. E.g. the IDE that comes with Python creates CRLF files on Windows and LF on Linux: Pyton will run source files with both kinds of line endings on both platforms though. When you upload a package to PyPI under Windows, it will likely have CRLF line endings.

IMHO allowing the user explicitly chose to either .patch or .patch-binary would be cleaner than any 'automagic' behaviour that gets hard to reason about and test (explicit is better than implicit).

I do not understand what you refer to with the UTF-8 point: Encoding is completely orthogonal to line endings as both CR and LF are within ASCII and UTF-8 is in fact a superset of ASCII (backwards compatible). CRLF is the byte sequence 0x13 0x10 in both ASCII and UTF-8 so maybe you could be more specific about hat process exactly is to misinterpret what byte sequence here.

Note that using --binary unconditionally on Windows in #2035 AFAIU actually breaks applying an LF patch file to a CRLF file on Windows (as the patch executable is more forgiving under that platform, see above).

[xflr6]

Small correction, in hex, CRLF of course is 0xd 0xa.

[jakirkham]

I have to agree with Ray. Leaving line endings up to chance will result in trouble. It is also easy to fix with things like .gitattributes, which will normalize the line endings.

Note that there was a follow-up to PR ( https://github.com/conda/conda-build/pull/2035 ), which is PR ( https://github.com/conda/conda-build/pull/2052 ). It's probably worth checking whether having two patch files are necessary in that context before discussing further.

[mingwandroid]

Please go to: https://r12a.github.io/apps/conversion/
Paste the ጐ character into the 2nd box down (the one called Characters) and press convert.
Observe that the UTF-8 version of that character is 13 10 E1 8C 90

[mingwandroid]

There are many more encodings than UTF-8 too, UTF-16 and UCS-32 etc (with and without BOMs). To expect that 0xd 0xa will not appear in any of them without it unambiguously representing CRLF does not seem wise to me.

[xflr6]

Just to make sure: There might be some confusion of code points with UTF-8 byte sequences (E1 8C 90):

'ጐ'.encode('utf-8')
Out[1]: b'\xe1\x8c\x90'

[xflr6]

Note that .gitattributes is not a solution for patching a source archive downloaded from PyPI.

AFAIU #2052 only tries the LF conversion on Windows (where it may not be needed because patch does convert itself there, see the AppVeyor build I have linked earlier).

I did not mean to propose to leave anything to chance: just to allow users who exactly know how the line endings of the files they want to patch (and their patch files) look like to take control via the --binary option instead of needing to check the source code of the build tool for the automatic conversion details under different platforms.

[mingwandroid]

OK fair enough, in valid UTF-8 that byte-string won't appear, but it will in other encodings:

for l in range(1, 0x10ffff):
    ucc=chr(l)
    if ucc.isprintable():
        by = bytes(ucc,encoding='utf-32')
        if b'\x0d\x0a' in by:
            print('{} is {}'.format(ucc,by))

Can you point to a source archive on PyPI that suffers from incorrect line endings?

[xflr6]

My own ones, e.g. https://pypi.python.org/pypi/graphviz :-)

I agree that in a better world there would be only one kind of line endings. I would prefer to be pragmatic about this rather than considering them as incorrect.

With autocrlf, git checks out text files as CRLF under Windows. I think this is the default with https://git-scm.com/download/win

[mingwandroid]

I'd like to see an effort to fix the processes that end up with these CRLF-containing source dists on PyPI and/or something on PyPI to prevent uploads of the same instead of bodging and uglifying conda-build (I make no apologies for that characterisation) to workaround the side effects of these source archives, but I suppose these are 'facts on the ground'. Perhaps if these fixes were made or proposed to PyPA then doing something temporary in conda-build wouldn't be terrible.

@msarahan, @jakirkham, what are your opinions?

[xflr6]

One could still unconditionally convert all patched files to LF on all platforms but that would be the bigger change (that's why I probably did not propose it).

[msarahan]

I don't think a temporary solution in conda-build is possible. There's too many packages out there, and new ones may pop up with this issue.

Conda-build's current strategy is this:

1. Try to apply patch without any changes to line endings.
2. Try to convert patch to unix line endings
3. Try to convert patch to CRLF, and use --binary argument to patch

I'm not opposed to a fix for this, but it does make conda-build more brittle to get into this stuff. I understand that packagers can't necessarily change upstream, but for anyone who actually has control over their repo, they really need to fix their line endings. I guess the issue is that even if your repo has sane line endings, if you package wheels or sdists on windows, the wheel or sdist (which is what conda uses to build) is then wrong. Is that how this arises?

Please submit a PR with your desired outcome, and I'll gladly review it.