Improve license checker line ending validation #2391
Conversation
tools/check-license.py
Outdated
| @@ -78,7 +79,7 @@ def main(): | |||
| for fname in files: | |||
| if any(fname.endswith(ext) for ext in EXTENSIONS): | |||
| fpath = os.path.join(root, fname) | |||
| with open(fpath) as curr_file: | |||
| with io.open(fpath, 'r', errors='ignore', newline='') as curr_file: | |||
There was a problem hiding this comment.
Why not simply use universal newlines (newline=None, or just don't mention newline at all)? Then python will see all newlines as \n and the LICENSE regex can be left unchanged.
(BTW, wouldn't that be simply equivalent to open(fpath, 'rU'), without need for io?)
There was a problem hiding this comment.
The problem was that Travis (seemingly) did not use the universal newlines that's why there was an license error reported in #2371 , on my system the conversion is performed and there is no error reported. For the rU I got a deprecation warning with Python3.
There was a problem hiding this comment.
Could you please double check? Here are my experiments (on top of this PR's commit):
akosthekiss
added
enhancement
The license checker previously assumed that the lines of the license will always end with \n characters. However when checking a file it could happen that other line endings are returned (should only happen for test files) thus the checker can incorrectly report invalid license as the line endings are incorrect. Additional note jerryscript-project#1: in Python when reading a file in text mode it can happen that the line endings are converted to the host system's line ending. However on Travis the conversion did not happen when using the open built-in method. By switching to the io.open call the conversion is enforced and all line endings are converted to '\n' regardless of the host system's line ending. Additional note jerryscript-project#2: it is possible that there are input test files which are not utf-8 conformant (eg.: to test the parser). These files can't be read as utf-8 strings and an exception would occur. By ignoring these errors the tool can check the file's license. In the license text there is no invalid utf-8 character so the check will work correctly. JerryScript-DCO-1.0-Signed-off-by: Peter Gal [email protected]
galpeter
force-pushed
the
licence-checker-lineendings
branch
from
4dad260 to
60a3c33
Compare
|
@akosthekiss did a triple-check and it worked as you suggested (removed the newline parameter). Guess that at a previous check there was some incorrect modifications on my side. Thanks! I've updated the PR. |
The license checker previously assumed that the
lines of the license will always end with \n
characters. However when checking a file
it could happen that other line endings are
returned (should only happen for test files) thus
the checker can incorrectly report invalid license
as the line endings are incorrect.
Additional note #1: in Python when reading a file
in text mode it can happen that the line endings are
converted to the host system's line ending.
However on Travis the conversion did not happen when
using the open built-in method. By switching to the
io.open call the conversion is enforced and
all line endings are converted to '\n' regardless of
the host system's line ending.
Additional note #2: it is possible that there
are input test files which are not utf-8 conformant
(eg.: to test the parser). These files can't be read
as utf-8 strings and an exception would occur.
By ignoring these errors the tool can check
the file's license. In the license text there is no
invalid utf-8 character so the check will work
correctly.