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

Unwanted carriage returns in output file #165

Closed
Diggsey opened this issue Nov 28, 2022 · 10 comments
Closed

Unwanted carriage returns in output file #165

Diggsey opened this issue Nov 28, 2022 · 10 comments

Comments

@Diggsey
Copy link

Diggsey commented Nov 28, 2022

On windows os.linesep includes a carriage return, so when the output file is opened in text mode, \n is translated to \r\n. This can be disabled by passing newline='' when opening the file.

Whether this should be the default is another question. Personally I would prefer it if poetry produced identical output files regardless of the system. 🤷‍♂️ If this cannot be the default, then a flag or configuration option would be sufficient.

@dimbleby
Copy link
Contributor

so far as I can see https://pip.pypa.io/en/stable/reference/requirements-file-format/ is silent on what the line separator should be, which I take to mean that all the major variations should be valid everywhere.

Certainly that doesn't say that carriage returns are unwanted.

You haven't said why this matters to you - what's the problem you're trying to solve?

eg if you have found some tool that does care about the line endings then perhaps it would be better to fix that tool.

@Diggsey
Copy link
Author

Diggsey commented Nov 28, 2022

You haven't said why this matters to you - what's the problem you're trying to solve?

To have a consistent result regardless of whether I run poetry export on windows or linux. In this specific case we have a CI step which checks that requirements.txt is up to date with the poetry lockfile, but there are any number of reasons why you might want the output of the tool to be the same on all platforms.

@neersighted
Copy link
Member

neersighted commented Nov 28, 2022

I think that a platform default makes more sense/mirrors the behavior of existing tools (e.g. pip freeze, pip-tools).

I see two options here: suggest using .gitattributes e.g.

requirements.txt text eol=lf

Or introduce a flag e.g. poetry export --eol=lf.

I would like to point out that it's less idiomatic to manually (where manually here is "with CI validation") maintain a requirements.txt in-tree instead of generating it as-needed, but I suspect you have constraints/well-founded reasons to do so.

If the only rationale for customizing the line-ending is to support a workflow like yours, where the output of poetry export is checked in, I strongly prefer the .gitattributes approach as it really solves the underlying problem (you want Git to consider the files the same regardless of what platform you export on).

@dimbleby
Copy link
Contributor

for the specific problem of detecting change in a pipeline you can simply make the comparison with the appropriate flag to ignore line endings eg git diff --ignore-space-at-eol

@Diggsey
Copy link
Author

Diggsey commented Nov 28, 2022

I think that a platform default makes more sense/mirrors the behavior of existing tools (e.g. pip freeze, pip-tools).

Perhaps... Although doesn't pip freeze just write to stdout? It doesn't do any line conversion.

requirements.txt text eol=lf

Yeah, this is one of the better options. The main downsides are that it's extra configuration that needs to be added in each case, and it means that other local tools that might look at requirements.txt have to handle both crlf and lf cases, so it's just more opportunity for things to break.

I would like to point out that it's less idiomatic to manually (where manually here is "with CI validation") maintain a requirements.txt in-tree instead of generating it as-needed, but I suspect you have constraints/well-founded reasons to do so.

I think it's so that you can checkout that git repository and install dependencies without having to have poetry installed if you are only intending to run the code rather than work on it.

you want Git to consider the files the same regardless of what platform you export on

Well ideally they actually would be the same 😛 Git happens to be the way these files are getting from system A -> B, but this idea of having files be represented differently on different systems only exists for legacy reasons, and in reality LF is well supported on all platforms (even notepad, which was the last hold-out...)

for the specific problem of detecting change in a pipeline you can simply make the comparison with the appropriate flag to ignore line endings eg git diff --ignore-space-at-eol

It still results in spurious diffs though with multiple people updating the files.

@dimbleby
Copy link
Contributor

dimbleby commented Nov 28, 2022

if your position is that the standard for requirements.txt ought to be different than it is, then you should take that to pypa and try to get folk to agree to it. Maybe start a conversation at https://discuss.python.org/c/packaging/14. If the rules change, then I expect this plugin would follow.

if your problem is developers running poetry export on different systems and committing those changes then (i) the .gitattributes solution is good and (ii) I doubt that a new command line option would help anyway, people will still forget to use it

I'd definitely be opposed to forcing this plugin always to output unix-style line endings, that will surely cause more complaints than it solves.

I'd be only mildly opposed to a command line option for the plugin, not enough to kick up a fuss. If you put together an MR then perhaps you'll find a maintainer who's willing to merge it.

@neersighted
Copy link
Member

Perhaps... Although doesn't pip freeze just write to stdout? It doesn't do any line conversion.

Line endings are output on stdout just like when writing to a file. pip freeze uses the platform-specific line ending -- try piping it into xxd or friends on a Unix and Windows system.

Yeah, this is one of the better options. The main downsides are that it's extra configuration that needs to be added in each case, and it means that other local tools that might look at requirements.txt have to handle both crlf and lf cases, so it's just more opportunity for things to break.

That's the state of the standard. Tools must be robust against differing line endings as the line ending is not specified anywhere. So either tools must change or the standard must change. I don't think Poetry suddenly growing strong opinions out of line with the rest of the ecosystem is productive.

Given the state of standardization, and the fact that the problem seems to be "how do I ensure that there are always LF line endings in my Git repository," I think .gitattributes is the solution here.

@gazpachoking
Copy link

gazpachoking commented Apr 11, 2023

The issue I'm having is that we have pre-commit hooks that do a poetry export. I have git set up to always commit line endings as LF, but when the pre-commit hooks run on windows, it modifies the exported files to be CRLF, and the commit fails, because it detects changes in the files. When I actually commit the files back to the repo the line endings will be normalized back to LF, but that doesn't stop the pre-commit from aborting the commit the first time because they changed.
This might not actually be the case. I think it only happens when there are actual changes made to one of the exported requirements, which is reasonable. I was getting a bit confused because it does regenerate all the exported files with CRLF, even ones that haven't changed, but the commit only fails if one of them has actual content changes.

@dimbleby
Copy link
Contributor

I expect this issue should be closed, it's become clear that there's no appetite among maintainers for changing the code and .gitattributes seems satisfactory

@radoering
Copy link
Member

I expect this issue should be closed

I agree since .gitattributes is the idiomatic solution and seems to be sufficient.

Since it has not been mentionend so far: I'd probably choose

requirements.txt text=auto

This way, you get CRLF on Windows. poetry export keeps CRLF, so you can diff, etc. and if you commit, line endings will be converted to LF in the repository. (Nothing changes on Linux, all LF.)

In contrast if you set eol=lf you will get LF on Windows and if you run poetry export you get CRLF and have a change just because of the line endings.

@radoering radoering closed this as not planned Won't fix, can't repro, duplicate, stale Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants