[BUG] build_scripts.py incorrectly handles shebang lines with DOS line endings

[amalachow]

Duplicating pypa/setuptools#3129 here due to some permissions issues during the transfer.

Python version

Python 3.8.10

OS

NT-10.0-19042

Description

shebang using DOS line endings causes the shebang regular expression to fail resulting in redirection to the python interpreter used for the installed script to not behave as expected.

Expected behavior

The shebang is rewritten consistently for UNIX and DOS line endings.

How to Reproduce

#!/bin/bash -eu
printf "[[System Info]]\n"
uname -srvmo
python3 --version
python3 -m virtualenv --version
rm -rf .venv
python3 -m virtualenv --quiet -p python3 .venv
.venv/Scripts/python -m pip install --quiet --upgrade pip &> /dev/null
.venv/Scripts/python -m pip install --quiet --upgrade setuptools &> /dev/null
.venv/Scripts/python -m pip list

rm -rf example
mkdir example
cat > example/example.py << END
#!/usr/bin/env python3

def main():
    print("Example script")

if __name__=="__main__":
    main()
END

cat > example/setup.py << END
from setuptools import setup
setup(name="example", scripts=["example.py"])
END

printf "\n[[Expected behavior]]\n"
file example/example.py
.venv/Scripts/pip install --quiet ./example
head -1 .venv/Scripts/example.py

printf "\n[[Issue]]\n"
sed -ie 's/\r*$/\r/g' example/example.py
file example/example.py
.venv/Scripts/pip install --quiet -I ./example
head -1 .venv/Scripts/example.py

fix="--- a/build_scripts.py
+++ b/build_scripts.py
@@ -15 +15 @@
-first_line_re = re.compile(b'^#!.*python[0-9.]*([ \t].*)?$')
+first_line_re = re.compile(b'^#!.*python[0-9.]*([ \t].*)?\r?$')"

printf "\n[[Proposed Fix]]\n"
echo "${fix}"
patch -u -p0 .venv/Lib/site-packages/setuptools/_distutils/command/build_scripts.py <<< ${fix}
file example/example.py
.venv/Scripts/pip install --quiet -I ./example
head -1 .venv/Scripts/example.py```

Output

[[System Info]]
MINGW64_NT-10.0-19042 3.1.7-340.x86_64 2021-03-26 22:17 UTC x86_64 Msys
Python 3.8.10
virtualenv 20.10.0 from C:\Python38\lib\site-packages\virtualenv\__init__.py
Package    Version
---------- -------
pip        22.0.3 
setuptools 60.9.3 
wheel      0.37.1 

[[Expected Behavior]]
example/example.py: Python script, ASCII text executable
#!C:\setuptools-issue\.venv\Scripts\python.exe

[[Issue]]
example/example.py: Python script, ASCII text executable, with CRLF line terminators
#!/usr/bin/env python3

[[Proposed Fix]]
--- a/build_scripts.py
+++ b/build_scripts.py
@@ -15 +15 @@
-first_line_re = re.compile(b'^#!.*python[0-9.]*([ \t].*)?$')   
+first_line_re = re.compile(b'^#!.*python[0-9.]*([ \t].*)?\r?$')
patching file .venv/Lib/site-packages/setuptools/_distutils/command/build_scripts.py
example/example.py: Python script, ASCII text executable, with CRLF line terminators
#!C:\setuptools-issue\.venv\Scripts\python.exe

[jaraco]

I'm not certain the right thing to do here is support shell scripts containing carriage returns in the shebang. See this reference, which I found from Wikipedia on shebang. That answer suggests that it's incorrect or unsupported to encounter a carriage return in the shebang.

Before distutils enacts changes to support this additional, non-preferred form (versus the preferably one obvious way to do it), I'd like to see support from the Unix community that this format should be supported.

I'd also like to see a more thorough description of why it's important to support this format. Your repro above does a pretty good job of telling the story. In particular, it seems that if one uses simple unix tools (i.e. cat) to construct a script on a Windows machine, it will create the file containing a carriage return.

It's curious to me that Python's regex module detects "\n" as a new line, but does not detect "\r\n" as a new line. Perhaps that implies that distutils should have read the file using universal newlines in order to translate any \r\n combinations to \n. Regardless, I don't believe embedding \r in the regex is the right approach here. Instead, the input to first_line_re.match should have any trailing newline removed.

Also, I notice that when the shebang is replaced, it gets replaced without a carriage return. That seems bad. That will end up creating a script with mixed newlines (LF for the shebang, and CRLF for the rest), another reason to think carefully about whether it's worth supporting CRLF at all.

Finally, if we can determine that the best thing for Python/distutils to do here is support CRLF, I'd like to see some tests that capture this new requirement (i.e. test that copy_scripts works on a file created with CRLF in the shebang).

[amalachow]

I'd also like to see a more thorough description of why it's important to support this format.

There are some behavioral implications of the shebang being rewritten inconsistently when using copy_scripts within a virutalenv. So if example.py uses a dependency installed in the virtualenv the shebang not being rewritten can result in running the script directly to fail as the shebang still points to the system interpreter.

I noted this failure condition as a result of inconsistency in the line endings ultimately stemming from git core.autocrlf auto vs input. Obviously this can be mitigated by being explicit about line endings in .gitattributes, but it seemed sensible that Unix and Windows could/should be handled consistently.

Outside the Unix expectations it looks like CPython's launcher supports "\r\n" which is worth considering given the nature of the reported issue as its predominantly Windows centric behavior that is being scrutinized.

I'm generally in agreement with your overall assessment on the mixed line endings as a side effect of the shebang rewrite and that simply including "\r\n" as consideration in the regex may not be the appropriate solution. I can work on getting some additional test cases added should this progress as something you consider worth rectifying.

[jaraco]

Given the now near ubiquity of Linux on Windows, I think this is worth fixing. Here's what I'd like to see:

• When reading a scripts file for copy, read it in using universal newlines. That implies also using text mode. Since the text could potentially contain multi-byte characters, use encoding='latin-1', which supports all bytes.
• When writing a script file for copy, use the newline as detected by the read operation (i.e. retain newlines, i.e. outf = open(name, newlines=inf.newlines)).

This way, the style of newlines in the file is unchanged, but any processing of the file, such as replacing the shebang, can be done assuming \n for newlines and regex works naturally.

I'd definitely consider a patch with this approach. Let me know how it goes and if there are any assumptions I'm making that are broken.

[jaraco]

• Since the text could potentially contain multi-byte characters, use encoding='latin-1', which supports all bytes.

On second thought, use of utf-8 and the 'surrogateescape' handler would be better:

>>> raw = b''.join(val.to_bytes(1, 'big') for val in range(256))
>>> raw.decode('utf-8', errors='surrogateescape').encode('utf-8', errors='surrogateescape') == raw
True