Add newline conversion for postBuild scripts #301
Conversation
|
What do people in general think about this? If it has support I would add some tests with explicit |
|
I'm +1, I think it's a fair concern that it slightly change the thing that is run on the repo2docker side vs. whatever is on the computer. However, since this is limited to line endings I think it's OK. Otherwise any user that makes things on windows will have to 1. learn about the concept of line endings, and 2. remember to change this, every time the wanna use postBuild. tl;dr, I can't really think of a situation where we'd think "damn, I wish we had not forced *nix line endings", so I'm +1 |
repo2docker/buildpacks/base.py
Outdated
| @@ -110,6 +110,8 @@ | |||
| {% if post_build_scripts -%} | |||
| {% for s in post_build_scripts -%} | |||
| RUN chmod +x {{ s }} | |||
| # Fix up line endings for users who checked out postBuild scripts on Windows | |||
| RUN python -c "open('{{ s }}', 'w').write(open('{{ s }}').read())" | |||
There was a problem hiding this comment.
This technically eliminates the possibility of binary postBuild, but I'm not sure that's an issue.
Should this also be specifically python3?
There was a problem hiding this comment.
Is there a good way to tell if it is a binary? It seems rare but we could try to detect it and refuse to do anything if we think it is a binary. I'll experiment a bit with file postbuild to see how many different things it outputs.
👍 on python3
There was a problem hiding this comment.
We could add a test checking if file postBuild thinks this is a binary but I'd vote to do that once we have someone who actually uses binaries for postBuild.
|
Does anyone have an idea why suddenly the memlimit tests fail? Doesn't seem like something that would be flaky but also not really related to changes here I'd say. |
|
Can we add something that asserts that the original |
| @@ -0,0 +1,4 @@ | |||
| #!/bin/bash | |||
|
|
|||
| # I contain CRLF line endings on purpose | |||
There was a problem hiding this comment.
I had hoped to see some extra characters/symbol here on GH to verify that this file contains CRLF. Could someone on a not-windows system checkout this branch to confirm my .gitattribute worked?
There was a problem hiding this comment.
I opened it on my windows machine and it said that it was in CRLF mode, which is what we want. Just FYI, you should be able to open this with Atom or something like this, and it should tell you the line endings present in the file (and if not by default, then there are plugins for this kinda thing for most text editors)
There was a problem hiding this comment.
Locally I see the extra characters but because git can/will mangle the line endings when you checkin/-out I wanted to see if someone somewhere else can also see them after it has gone through git :)
|
in terms of an assertion, I think all we'd need to is use with open('./postBuild.txt', 'r', newline='') as ff:
lines = ff.read()and lines should now reflect whatever the file contains (e.g. |
|
Where/how would we get that snippet executed as part of the test? |
|
Does someone have an idea why the |
betatim
force-pushed
the
sanitise-newline
branch
from
64f4063 to
cb77c47
Compare
This adds a test and the git settings to prevent git from changing the line endings in our carefully crafted postBuild file.
betatim
force-pushed
the
sanitise-newline
branch
from
cb77c47 to
6a2b232
Compare
|
What shall we do with this one, @betatim? Do you think there have been enough complaints about this in the last two years for us to push this through? |
|
I'm triaging some PRs - @betatim do you have a recommendation on what to do with this? |
|
I think it would be useful and doesn't add too much complexity/magic. However it seems like there isn't a lot of interest/need. If someone is interested to pick up this stalled PR that would be great. |
consideRatio
added
enhancement
configuration file
and removed
workflow:needs-update
labels
|
I'll go for a close at this point since we are not actively working this PR and since it would introduce a possible issue for binary postBuild executables. |
Fixes #263
Python normalises line endings to always be "platform appropriate". So by reading in a file that potentially uses
\r\n(because it was checked out from git on Windows) and writing it out again on a linux platform (inside a docker container) we make sure files only use\n.