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

Add some linting jobs for CI #1807

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .editorconfig
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ insert_final_newline = true
[*.py]
indent_size = 4

[meson.build]
[*.json]
indent_size = 2

[{meson.build,meson.options,meson_options.txt}]
indent_size = 2

1 change: 0 additions & 1 deletion subprojects/packagefiles/lame/libmp3lame/fix_def.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
#!/usr/bin/env python3

import os
import sys

with open(sys.argv[1], 'r') as f:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import os
import sys
import re
import subprocess

# Put output files in the builddir
Expand Down
3 changes: 2 additions & 1 deletion subprojects/packagefiles/libuv/link_file_in_build_dir.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#!/usr/bin/env python3

import os, sys
import os
import sys
Comment on lines -3 to +4
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, please can we not!

Copy link
Member Author

@dcbaker dcbaker Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we follow standard python coding guidelines? yes please

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing standard about this change, other than that black encourages it.

Copy link
Contributor

@trim21 trim21 Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually comes from pep8.

https://peps.python.org/pep-0008/#imports

(Even same modules, lol)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The authors and maintainers of the cpython PEPs take every possible opportunity they possibly can to tell people that PEP 8 is not a standard for anything other than "the style for internal cpython project source code" and that people should stop putting them in a position to be the arbiters of python style, funnily enough.

And this particular rule is inconsistent and ugly so I can kind of see the point. :)


filename = sys.argv[1]
linkname = sys.argv[2]
Expand Down
38 changes: 18 additions & 20 deletions subprojects/packagefiles/m4/checks/m4_test_runner.py
Original file line number Diff line number Diff line change
@@ -1,16 +1,15 @@
import sys
import subprocess
import os
import tempfile

# We want to print line endings normally, but each line should be a b'' string
def clean(x):
lines = []
for l in x.splitlines():
if b'examples' in l:
lines.append(str(l.replace(b'\\',b'/')))
for line in x.splitlines():
if b'examples' in line:
lines.append(str(line.replace(b'\\',b'/')))
Comment on lines -9 to +10
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This specific ruff "issue" is extremely opinionated and also wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that it's opinionated, and in this case I'd agree waht was there before was fine, I think the second loop is more readaable with a line vs l though, since it's a long loop and things like line[len('MY_REALLY_LONG_STRING):] are generally more readable than l[len('MY_REALLY_LONG_STRING):]

else:
lines.append(str(l))
lines.append(str(line))
return '\n'.join(lines)

def check_error(run_result,
Expand Down Expand Up @@ -45,29 +44,28 @@ def check_error(run_result,
def main() -> int:
m4_path = sys.argv[1]
input_path = sys.argv[2]
tmproot = sys.argv[3]
workdir = sys.argv[4]
workdir = sys.argv[3]
examples_path = workdir + '/examples'
if ':' in examples_path:
examples_path = examples_path.partition(':')[2]
with open(input_path, 'rb') as input_file, tempfile.TemporaryDirectory(dir=tmproot) as tmpdir:
with open(input_path, 'rb') as input_file:
expected_out = bytes()
expected_err = bytes()
ignore_err = False
m4_input = bytes()
for l in input_file.read().splitlines():
if l.startswith(b'dnl @ expected status: '):
expected_code = int(l[len('dnl @ expected status: '):].rstrip())
if l.startswith(b'dnl @ extra options: '):
args = l[len('dnl @ extra options: '):].rstrip().decode()
if l.startswith(b'dnl @result{}'):
expected_out += l[len('dnl @result{}'):] + os.linesep.encode()
if l.startswith(b'dnl @error{}'):
expected_err += l[len('dnl @error{}'):] + os.linesep.encode()
if l.startswith(b'dnl @ expected error: ignore'):
for line in input_file.read().splitlines():
if line.startswith(b'dnl @ expected status: '):
expected_code = int(line[len('dnl @ expected status: '):].rstrip())
if line.startswith(b'dnl @ extra options: '):
args = line[len('dnl @ extra options: '):].rstrip().decode()
if line.startswith(b'dnl @result{}'):
expected_out += line[len('dnl @result{}'):] + os.linesep.encode()
if line.startswith(b'dnl @error{}'):
expected_err += line[len('dnl @error{}'):] + os.linesep.encode()
if line.startswith(b'dnl @ expected error: ignore'):
ignore_err = True
if not l.startswith(b'dnl @'):
m4_input += l + b'\n'
if not line.startswith(b'dnl @'):
m4_input += line + b'\n'
runargs = []
runargs.append(m4_path)
runargs.append('-d')
Expand Down
6 changes: 3 additions & 3 deletions subprojects/packagefiles/m4/checks/meson.build
Original file line number Diff line number Diff line change
Expand Up @@ -260,23 +260,23 @@ m4_path = m4.full_path()

foreach file : checks
test(file, py,
args: [m4_test, m4_path, files(file), meson.current_build_dir(), meson.project_source_root()],
args: [m4_test, m4_path, files(file), meson.project_source_root()],
env: m4_env,
depends: m4)
endforeach

if host_machine.system() != 'windows'
foreach file : noparallel_checks
test(file, py,
args: [m4_test, m4_path, files(file), meson.current_build_dir(), meson.project_source_root()],
args: [m4_test, m4_path, files(file), meson.project_source_root()],
depends: m4,
env: m4_env,
is_parallel: false)
endforeach

foreach file : nowindows_checks
test(file, py,
args: [m4_test, m4_path, files(file), meson.current_build_dir(), meson.project_source_root()],
args: [m4_test, m4_path, files(file), meson.project_source_root()],
env: m4_env,
depends: m4)
endforeach
Expand Down
4 changes: 3 additions & 1 deletion subprojects/packagefiles/openblas/gen_install_headers.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#!/usr/bin/env python3
import argparse
import re
from pathlib import Path

def write_openblas_config_header(dest_dir, version, config_last_path, template_path):
Expand All @@ -20,7 +21,7 @@ def write_openblas_config_header(dest_dir, version, config_last_path, template_p
rest_of_line = " ".join(parts[1:]) if len(parts) > 1 else ""
line_to_write = f"#define OPENBLAS_{macro_name} {rest_of_line}"
f.write(f"{line_to_write.strip()}\n")


f.write(f'#define OPENBLAS_VERSION " OpenBLAS {version} "\n')

Expand Down Expand Up @@ -101,3 +102,4 @@ def main():

if __name__ == "__main__":
main()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you know, a file with a missing new line. The very thing that started this :)

Copy link
Member

@eli-schwartz eli-schwartz Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally github's view of this would have indicated via some kind of red circle when the previous version didn't have a trailing end of line character, I thought. Meanwhile, the actual display here is this adds a blank line 105 to go with the code line 104. Files ending with main() are a problem and files ending with main()\n are correct... but if I checkout your PR, the file ends with main()\n\n, which is surely wrong?

In fact, if I checkout the file in git master locally, it doesn't appear to be missing the trailing end of line character.

1 change: 1 addition & 0 deletions subprojects/packagefiles/sdl2/find-dylib-name.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import re
import sys
import pathlib
import glob


def verbose(*args):
Expand Down
2 changes: 1 addition & 1 deletion subprojects/packagefiles/vo-aacenc/meson/makedef.py
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ def output(platform, symbols):
# Substitute prefix out
dump = [re.sub(f'\s+{prefix}', ' ', x) for x in dump]
# Substitute big chonky spaces out
dump = [re.sub(f'\s+', ' ', x) for x in dump]
dump = [re.sub('\s+', ' ', x) for x in dump]
# Exclude blank lines
dump = [x for x in dump if len(x) > 0]
# Take only the *second* field (split by spaces)
Expand Down
4 changes: 3 additions & 1 deletion tools/fake_tty.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
#!/usr/bin/env python3

import os, pty, sys
import os
import pty
import sys

os.environ['TERM'] = 'xterm-256color'
exit(os.waitstatus_to_exitcode(pty.spawn(sys.argv[1:])))
2 changes: 1 addition & 1 deletion tools/import-wraps.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def get_wrap_info(wrap: str) -> T.List[T.Tuple[str, str]]:
version, revision = line.split()
versions.append((version, revision))
except subprocess.CalledProcessError:
pass
pass
return versions

def rewrite_wrap(wrap: str):
Expand Down
4 changes: 2 additions & 2 deletions tools/sanity_checks.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@
'glbinding-aux_export.h'
],
'godot-cpp' : [
'meson-bindings-generator.py',
],
'meson-bindings-generator.py',
],
'gumbo-parser': [
'tokenizer.cc',
],
Expand Down
2 changes: 1 addition & 1 deletion tools/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ def is_linux() -> bool:
return platform.system().lower() == 'linux'

def is_windows() -> bool:
return platform.system().lower() == 'windows' and not "MSYSTEM" in os.environ
return platform.system().lower() == 'windows' and "MSYSTEM" not in os.environ
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't tell is this is a bugfix.


def is_msys() -> bool:
return platform.system().lower() == 'windows' and "MSYSTEM" in os.environ
Expand Down