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

Vendoring process fails on Windows #7617

Closed
pradyunsg opened this issue Jan 21, 2020 · 6 comments
Closed

Vendoring process fails on Windows #7617

pradyunsg opened this issue Jan 21, 2020 · 6 comments
Assignees
Labels
auto-locked Outdated issues that have been locked by automation project: vendored dependency Related to a vendored dependency type: maintenance Related to Development and Maintenance Processes

Comments

@pradyunsg
Copy link
Member

Environment

  • pip version: master
  • Python version: 3.8
  • OS: Windows 10

Description

Running tox -e vendoring fails on Windows.

The reason for the failure seems to be newline normalization -- see https://github.com/pypa/pip/commit/5563141d58bc17d5fab3bf4ca1649e136e25696c/checks/396942169/logs (from #7611). The patching process failed and vendoring (the tool) did not error out immediately. The git diff --exit-code catches the problem though. :)

Expected behavior

tox -e vendoring should work on all platforms.

How to Reproduce

  1. Checkout pip's current master on Windows.
  2. Run tox -e vendoring
  3. An error occurs.

Output

From the logs linked above, the relevant lines are:

2020-01-18T18:29:27.6652039Z   Running git apply --verbose 'tools\automation\vendoring\patches\requests.patch'
2020-01-18T18:29:27.6652131Z     error: corrupt patch at line 7
@pradyunsg pradyunsg added project: vendored dependency Related to a vendored dependency type: maintenance Related to Development and Maintenance Processes labels Jan 21, 2020
@pradyunsg pradyunsg self-assigned this Jan 21, 2020
@pfmoore
Copy link
Member

pfmoore commented Jan 21, 2020

I can take a look at this later this week, if no-one gets to it before me. @pradyunsg I see you self-assigned it - I'm fine if you prefer to deal with it yourself.

It may be as simple as marking the patch files as "binary" in .gitattributes to stop git using platform line endings in them.

@pradyunsg
Copy link
Member Author

pradyunsg commented Jan 21, 2020

There's 2 things to do here:

  1. Making vendoring fail when the patching fails (which is what I assigned myself for)
  2. Properly handling line endings for these patches (please feel free to self assign this bit to yourself @pfmoore).

I'm not 100% sure what the problem with the line endings is - if it's the line endings changing, or it might be a difference of line ending between the two files. I'm pretty sure that you'd be much better at figuring this out than me. :)

@uranusjr
Copy link
Member

This plus #7600 means we’ll need to figure out a universal setting for all these things. There are two settings affecting this: core.autocrlf and core.eol.

The “easiest” (in quotes) solution would be to punt and always use UNIX line endings (core.autocrlf=false, core.eol=lf). I can think of two downsides: we need to always write automation scripts with newline='\n', and some Windows folks using archaic editors may have problems editing the files. The solution at the other end of the spectrum is to always stick to the platform default, which would eliminate us from needing to stick newline='\n', but could have other problems (as the one we are seeing here).

@pfmoore
Copy link
Member

pfmoore commented Jan 23, 2020

I'd prefer to go with native line endings, as that's less likely to have odd side-effects (even though most editors work fine with \n line endings, they typically default to \r\n for new files).

Treating patch files as special makes more sense to me, because, well, they are - the patch utilities are what don't handle mixed line endings cleanly.

Actually, it looks like the problem is far simpler than all this - when we moved the patches from tasks to tools/automation, the .gitattributes file wasn't updated! I've just filed #7641 to fix this - it works on my PC, but if anyone else can test it as well, that would be great (I may not wait for confirmation from others before merging - it's the right change to make regardless).

@webknjaz
Copy link
Member

webknjaz commented Feb 3, 2020

It looks like this can be closed. Confirmed by rebasing #7611.

@pradyunsg
Copy link
Member Author

Yup!

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Mar 10, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Mar 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation project: vendored dependency Related to a vendored dependency type: maintenance Related to Development and Maintenance Processes
Projects
None yet
Development

No branches or pull requests

4 participants