search

zephyrproject-rtos / ci-tools

CI Tools and Scripts (obsolete)
Apache License 2.0
9 stars 18 forks source link

check_compliance.py: Avoid running 'git' before _main() #124

Closed ulfalizer closed 4 years ago

ulfalizer commented 4 years ago

Running 'git rev-parse --show-toplevel' to set GIT_TOP before _main() prevents any errors from it from being reported to GitHub, because the GitHub connection hasn't been initialized yet.

If there's an error, the error reporting is currently broken as well, because git() calls err(), which is defined at the end of the file.

Initialize GIT_TOP in _main() instead of at the top level, and use a magic "\<repo>" string to refer to the top-level repository directory when setting the 'ComplianceTest.path_hint' class variables instead.

Discovered while working on some unrelated error reporting improvements.

Also move the ZEPHYR_BASE initialization to a better spot, next to other global variables.

zephyrbot commented 4 years ago

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

ulfalizer commented 4 years ago

I find <repo> loses the important "TOP" part of "GIT_TOP". Why not just @git_top@?

Renamed it to <git-top>.

Besides no .git/ subdirectory, I'm curious what other sort of errors rev-parse --show-top-level can encounter. What did you experience?

I just put an error into the git command to test https://github.com/zephyrproject-rtos/ci-tools/pull/125. Makes it fail with an unrelated Python error, because the err() function isn't defined yet.

Means that if there was an error, it wouldn't even be properly reported when running locally.

More a general robustness thing so that the script never swallows errors.