Skip to content

scripts/build: add a python script for checking EoF markers across source tree #14107

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

scripts/build: add a python script for checking EoF markers across source tree #14107

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 27 additions & 0 deletions scripts/build/check_eof.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#!/usr/bin/python3
##
## license:BSD-3-Clause
## copyright-holders:Angelo Salese
"""Checks that all files in source code ends with a newline at end of file
"""

import os
import sys

if __name__ == '__main__':
CHAR_DEPTH = 2 if sys.platform in ["win32", "cygwin"] else 1
for r, _, f in os.walk("src"):
FILEPATHS = [os.path.join(r, file) for file in f]
for item in FILEPATHS:
if not item.endswith((".cpp", ".h", ".mm", ".lua", ".py")):
continue
with open(item, mode="r", encoding="utf-8") as fh:
try:
fh.seek(0, os.SEEK_END)
fh.seek(fh.tell() - CHAR_DEPTH, os.SEEK_SET)
value = fh.read(CHAR_DEPTH)
if value != os.linesep:
raise ValueError(value, item)
except UnicodeDecodeError:
print(item, file=sys.stderr)
raise
Comment on lines +11 to +27
Copy link
Member

Choose a reason for hiding this comment

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

This misses the various .ipp, .hxx, .lst, .inc, etc. files around the source tree.

I wonder if it wouldn’t be better to also check for various other stuff, perhaps:

  • Parse the whole file as UTF-8 and check for invalid UTF-8, overlong sequences, surrogates, etc.
  • Check for lines with trailing whitespace.
  • Check for non-native line endings (if you run the script on Linux, this would catch checked-in DOS line endings).
  • Check for X bit set on C-language sources.
  • Maybe some other simple stuff?

I do wonder what performance would be like if it was written in Python, although considering how well makedep.py performs these days, it might not be too bad.

I doubt it would do much to deter the people who check this stuff in most frequently, but it would be nice to get a flag on PRs at least. But then #13937 was merged when it obviously couldn’t build due to the added GAME line having the same short name as an existing one. This gives the impression that people aren’t even checking the most basic things before merging stuff, so I don’t know how effective anything will be.

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 do wonder what performance would be like if it was written in Python, although considering how well makedep.py performs these days, it might not be too bad.

Roughly 0.17 seconds locally.

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 doubt it would do much to deter the people who check this stuff in most frequently, but it would be nice to get a flag on PRs at least. But then #13937 was merged when it obviously couldn’t build due to the added GAME line having the same short name as an existing one. This gives the impression that people aren’t even checking the most basic things before merging stuff, so I don’t know how effective anything will be.

Sounds like a different script type, one that cross checks .lst with .cpp files in src/mame (don't think there are any other file extensions that use a GAME macro?)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but made the regular “compile MAME” jobs fail anyway due to duplicate symbols, so the person creating the PR and the person merging it already weren’t looking at build results. Having another CI job fail won’t fix that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Clang has a compiler warning for this:

/home/user/a.cpp:3837:2: warning: no newline at end of file [-Wnewline-eof]
 3837 | }
      |  ^

Automatic EOL handling in git is done via .gitattributes (updating an existing repo is fidgety though).

Invalid UTF-8 sequences in comments are caught by Clang with -Winvalid-utf8. Detetcing such sequences in actual code is only in clang-tidy IIRC.

Copy link
Member

Choose a reason for hiding this comment

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

Clang has a compiler warning for this:

That won’t address things that aren’t C++ source files (e.g. Python, Lua, XML, and various inputs to source generator tools).

Automatic EOL handling in git is done via .gitattributes (updating an existing repo is fidgety though).

It’s already set, but that doesn’t stop people from not setting core.autocrlf=true or otherwise messing things up and checking in DOS line endings as happened in 81a9784.

Also, none of that changes the fact that automated checks are useless if people just ignore the results.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, none of that changes the fact that automated checks are useless if people just ignore the results.

Can you protect master branch to avoid merging PRs if they fails for whatever reason? Needs admin rights in mamedev settings, around Require status checks to pass before merging

Copy link
Member

Choose a reason for hiding this comment

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

I see @Osso13 likes this idea. I guess it won’t be so bad as it would have been in the past now that CI runs don’t take as long as they used to (it used to be about three hours for macOS and over four hours for Windows, now it’s down to a bit over two hours for Windows).

What do other people think. @galibert?

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't mind, I usually wait for CI when merging anything.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I tried enabling this rules that CI has to pass, but it won’t work at the moment. The trouble is, not all jobs apply to all code changes, so e.g. if I enable checking the build-linux (gcc) status, it will prevent merging a PR that only affects the docs (and vice versa for build-docs status).

To make this workable, we need dummy jobs that set status results for the inverse conditions.

Loading