scripts/build: add a python script for checking EoF markers across source tree #14107
Conversation
|
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I wouldn't mind, I usually wait for CI when merging anything.
There was a problem hiding this comment.
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.
No description provided.