GitHub Actions Lint: flake8, cpplint, enforce_licence, generic NOLINTs

[DaAwesomeP]

Working toward #1815. This is still a draft. I am force-pushing to avoid massive commit spamming; I'll update the commit message to be very descriptive once I'm done.

I am using Debian stable as packaging on Debian seems to be very actively attended to by OLA contributors and not just Debian packagers. There also exist build instructions for Debian within the OLA repo.

So far:

• Build once, lint multiple times from that one build
• Don't require building first for lint tasks that don't need it
• Run flake8 the exact same way it would be run on a dev machine (from Makefile), but with annotations
• Move cpplint to latest pip version
• Add GH Actions annotations to enforce_licence.py
• Move the generic NOLINT check from the Travis-only script to a script that can be run anywhere
• Add GH Actions annotations to the generic NOLINT check

Still lots to do, but progress!

[DaAwesomeP]

There seems to be a weird bug or some other issue with the new cpplint. It shows header guard name errors on a seemingly arbitrary set of files (a subset of files only within the tools directory) like:

./tools/ola_trigger/VariableInterpolator.h:21:  #ifndef header guard has wrong style, please use: ___W_OLA_OLA_TOOLS_OLA_TRIGGER_VARIABLEINTERPOLATOR_H_  [build/header_guard] [5]

[peternewman]

There seems to be a weird bug or some other issue with the new cpplint. It shows header guard name errors on a seemingly arbitrary set of files (a subset of files only within the tools directory) like:

./tools/ola_trigger/VariableInterpolator.h:21:  #ifndef header guard has wrong style, please use: ___W_OLA_OLA_TOOLS_OLA_TRIGGER_VARIABLEINTERPOLATOR_H_  [build/header_guard] [5]

It seems to be more than just tools to me, it looked like everything. I think it's because your source archive is extracted into w/ola/ola. I don't know if that's technically a bug with our Makefile (it might need one of the relative or absolute variables added like some of the Python stuff, you should be able to experiment and prove it locally I think), or you need to cd before calling it, or both.

[peternewman]

Some comments and potential improvements

[peternewman]

I am force-pushing to avoid massive commit spamming

I think force pushes often confuse the review comments, but we can see how we go...

I am using Debian stable as packaging on Debian seems to be very actively attended to by OLA contributors and not just Debian packagers. There also exist build instructions for Debian within the OLA repo.

As mentioned I think Ubuntu should be fine and ought to just work too with probably less overhead...

So far:

* Build once, lint multiple times from that one build

❤️

* Don't require building first for lint tasks that don't need it

I think they probably all want make builtfiles to check we've got licences etc on them.

Still lots to do, but progress!

🎉

[peternewman]

Still lots to do, but progress!

Also I think we can definitely break this down a bit and iterate on it. Once we've got at least once working action, we can merge that as it's better than the none we've got thanks to Travis. E.g. we could merge something without cpplint say, because the other bits will benefit.

I also forgot to say we should probably target 0.10 with this so we can check that branch works as intended too.

[DaAwesomeP]

As mentioned I think Ubuntu should be fine and ought to just work too with probably less overhead...

See also note above, not much overhead. I am partial to Debian because of the docs that exist in this repo specific to Debian and the long release cycles for every release (as opposed to Ubuntu where not every release is LTS, and then a larger jump). If you would like to switch to Ubuntu, please let me know which version to run against.

I think they probably all want make builtfiles to check we've got licences etc on them.

I think I'm not clear on the myriad of tasks that make builtfiles does. Does it also prepare Python files, CPP source files, and the license files?

Also I think we can definitely break this down a bit and iterate on it. Once we've got at least once working action, we can merge that as it's better than the none we've got thanks to Travis. E.g. we could merge something without cpplint say, because the other bits will benefit.

Sounds good! I think I fixed the cpplint issue! I'm going to leave my current GH annotations implementation in for now (better than nothing, and I promise I will migrate!) and I will move them to the parser method later.

I also forgot to say we should probably target 0.10 with this so we can check that branch works as intended too.

Will do, may involve a fresh pull.

[DaAwesomeP]

OK, I think I managed to change the target branch and re-branch and cherry-pick my source branch without causing mass destruction.

[DaAwesomeP]

OK, by archiving and unarchiving the artifacts I just shaved 6+ minutes off of the total actions time. It now takes 4ish minutes total instead of over 10 minutes.

See actions/upload-artifact#199, GitHub is very slow at dealing with large volumes of files.

[DaAwesomeP]

OK, I have converted the license and NOLINT checks to use problem matchers!

[peternewman]

See also note above, not much overhead. I am partial to Debian because of the docs that exist in this repo specific to Debian and the long release cycles for every release (as opposed to Ubuntu where not every release is LTS, and then a larger jump). If you would like to switch to Ubuntu, please let me know which version to run against.

Although the docs are aimed at Debian they should work fine on any Debian based OS (Ubuntu/Raspbian etc). I used to just target and switch based on LTSes (which I think is all GitHub Actions offers Ubuntu wise) and tried to run on the bleeding edge unless there seemed to be other issues e.g. with Coverity.

I think I'm not clear on the myriad of tasks that make builtfiles does. Does it also prepare Python files, CPP source files, and the license files?

So it was initially targeted at Doxygen:

ola/Makefile.am

Lines 197 to 200 in 1151e8d

	# This gives us a rather hacky method to build the files normally built during
	# the build, so they are present for Doxygen to run against
	builtfiles : Makefile.am $(built_sources)
	.PHONY : builtfiles

More here:
https://www.gnu.org/software/automake/manual/html_node/Built-Sources-Example.html

But basically it's every source file that the build itself generates somehow, regardless of the programming language it might be in or how it might get generated (e.g. Makefile, protoc or shell script).

So while it might mean we've generated some C++ files unnecessarily when we're running a Python based flake8, it's probably easier than trying to work out which ones we need and keep track of it separately.

OK, I think I managed to change the target branch and re-branch and cherry-pick my source branch without causing mass destruction.

Well done, that's better than I ever manage! 😆

OK, by archiving and unarchiving the artifacts I just shaved 6+ minutes off of the total actions time. It now takes 4ish minutes total instead of over 10 minutes.

Nice work.

See actions/upload-artifact#199, GitHub is very slow at dealing with large volumes of files.

Did you see the comments about how cheating and using the caching functions may be faster still, maybe something to play with in future.

Note that one limitation with the problem matchers is that it will never show more than 10 annotations of one type, but it also doesn't mention anywhere that there may be more hidden annotations when more than 10 exist.

I didn't know that, hopefully they'll fix it in future, but the user will spot anyway when they fix the first batch of annotations!

I think barring the above unresolved issues and comments with possible additional necessary make and ./configure compilation tasks or prerequisites for other tasks, this should be good to merge!

So as mentioned, aside from a few minor bits like which Python shebang which I think we should fix before merge, I think the other outstanding bits can just be listed as a TODO tick list in the main issue and then we can merge this so we start to benefit, and then we can individually pick off the outstanding bits to e.g. make the flake8 build run configure less times etc.

[peternewman]

Some comments (before I've seen your very latest changes...)

[peternewman]

I don't think GitHub recognises notice as a severity, just error and warning.
https://github.com/actions/toolkit/blob/master/docs/problem-matchers.md#single-line-matchers

[DaAwesomeP]

It does! I found it in the source. It causes nice messages like these: https://github.com/DaAwesomeP/ola/actions/runs/4218995033

[peternewman]

❤️ 🤦
Perhaps you'd care to open a PR to improve their docs?

[DaAwesomeP]

Will do!

[peternewman]

Ah this might make my backwards compatible comment a bit less relevant.

[DaAwesomeP]

I think we could also completely take this and flake8 out of dependency checking, if you're OK with that. I don't think that someone building OLA should necessarily be required to have cpplint and flake8 (or maybe there is already a ./configure flag for that).

[peternewman]

I don't believe it should block the build if they're not present.

Indeed my config.log has:

configure:27245: checking for flake8
configure:27273: result: yes
configure:27291: checking for cpplint.py
configure:27319: result: no

It just means we can present a more user-friendly message if they try to run the tools.

[peternewman]

One other minor procedural comment, which I'm sure is down to personal preference, I've normally reviewed stuff where the person commenting makes a comment, the contributor then fixes it and replies to the comment with a "Done" or similar, or indeed suggests why my comment is daft and they shouldn't, then I can go back and resolve the discussion when I've reviewed the fix/agreed with my stupidity. It just makes it a bit easier to track what comments I've re-reviewed and resolved and what still needs looking at.

[peternewman]

LGTM thanks.

I think the only outstanding bits that could be done are:

• Dropping flake8-annotation-python3 again
• Minor workflow comments
• Possibly make some workflow debug logging optional, I know this will inevitably bite me either way, we'll leave it in and I'll spend my life scrolling through it, or we'll take it out and I'll immediately need it, so I'll let you pick what to do (then I can curse someone else 🤣 ).

[DaAwesomeP]

One other minor procedural comment, which I'm sure is down to personal preference, I've normally reviewed stuff where the person commenting makes a comment, the contributor then fixes it and replies to the comment with a "Done" or similar, or indeed suggests why my comment is daft and they shouldn't, then I can go back and resolve the discussion when I've reviewed the fix/agreed with my stupidity. It just makes it a bit easier to track what comments I've re-reviewed and resolved and what still needs looking at.

I'm very sorry! I will stop marking anything as resolved.

Anyway, I think I fixed the remaining bits!

[DaAwesomeP]

@peternewman I was very confused (I'm sure this was not intentional), but apparently GitHub lets you edit others' comments?

[peternewman]

@peternewman I was very confused (I'm sure this was not intentional)

Indeed it wasn't, that was supposed to be a quote reply. I did wonder at the time why it hadn't commented some of the lines! 🤦

Now reverted!

but apparently GitHub lets you edit others' comments?

I assume that's a privelege of being an organisation admin. I'm guessing you can't edit mine?

[peternewman]

What I intended to write:

I'm very sorry! I will stop marking anything as resolved.

No worries, I can completely see the logic in what you did too.

Anyway, I think I fixed the remaining bits!

Amazing thanks! Onto the next one... laughing

[peternewman]

I've now added branch protection for 0.10 of these new checks thanks!