review: chore: Migrate Javadoc regression script to python

[I-Al-Istannen]

Motivation

After #4025 we now have a python script that tries to find the new errors and a bash script that actually runs the checkstyle. This PR will try to consolidate this into a single python script that should also work on Windows.

Content

This PR will expand the regression-finding Python script to also run the checkstyle and replace the existing bash script.

Question: Paths

subprocess on Windows is apparently pretty broken and won't scan through the full (user + system) PATH. This might cause the mvn invocation to fail, if the user has installed maven into their user PATH (e.g. via scoop). I couldn't find any proper workaround for this, but I could set shell=True and pass all commands as simple strings. We control them so we should be able to avoid security vulnerabilities along the way.

Question: Colors

Most terminal emulators and Github Actions (see here and here) do support ANSI escape codes (and we could disable it on windows by default). This allows us to color the output of the script to make it a bit easier to read. This is probably a bit controversial, but would you be opposed to that?

Current draft:

And if the user (likely) screwed up their maven setup:

[slarse]

Question: Paths [...] subprocess on Windows is apparently pretty broken and won't scan through the full (user + system) PATH

Ugh. Well, that's too much of a bother IMO. Unless someone says anything, let's leave it as explicitly *NIX compatible.

but I could set shell=True and pass all commands as simple strings

As this runs in CI, I would tend to say no. Maybe it'll be fine but it triggers my spidey sense pretty badly.

Question: Colors [...]

I don't think it's worth the bother. To my eyes it adds very little here since the output is so simple anyway. It makes the code just a little bit harder to read, and a little bit harder to edit (have to consider what color something should have).

That's just my opinion and I don't feel strongly enough about it to say nay, though. If you feel like the colors add enough value to be worth it, then go ahead.

[I-Al-Istannen]

Ugh. Well, that's too much of a bother IMO. Unless someone says anything, let's leave it as explicitly *NIX compatible.

I adjusted a few things while trying it out and it should work on windows, barring the potential PATH issue. So that might be good enough for now :)

As this runs in CI, I would tend to say no. Maybe it'll be fine but it triggers my spidey sense pretty badly.

Yea, it will probably be alright but you don't exactly have to add an possible attack vector if you can help it...

I don't think it's worth the bother. To my eyes it adds very little here since the output is so simple anyway. It makes the code just a little bit harder to read, and a little bit harder to edit (have to consider what color something should have).
That's just my opinion and I don't feel strongly enough about it to say nay, though. If you feel like the colors add enough value to be worth it, then go ahead.

I probably wrote too many weird websites and programs but I think it's quite nice and helps. I initially added color support because I was having trouble reading the plain white output at a glance. I'll leave it in there and you can always rip it out if you feel it gets a bit cumbersome. I do hope nobody will be unfortunate enough to touch this area in the near future though :P

---

The CI in this PR here runs with my changes so you can find the output here. I probably didn't have to enable them in my fork, as I could have just looked at the output in here.

[slarse]

Overall it LGTM, functionality seems like what we need. Definite improvement from the bash script. I just have a few comments on the code that I think can be improved, and a few general comments that you don't really need to worry about (more as FYIs).

[slarse]

This looks good to me, will merge today! Thanks for taking the time, this is much better than a bash script.

[I-Al-Istannen]

No worries, it was annoying me ;)

Maybe it would be worth it to update the referenced issue #3923 a bit? Not sure if it's needed, but we still link to it and a few things have changed. It will e.g. fail to checkout if you have incompatible files:

$ git rev-parse --show-toplevel
$ git checkout master
Error ececuting native command: ['git', 'checkout', 'master']
Error output:
  error: Your local changes in the following files would be overwritten
  	chore/check-javadoc-regressions.py
  Please commit or stash your changes before switching branches.
  Abort

This won't happen in CI as it doesn't edit any files here and as a user it sounded a bit nicer than bold warnings in a document you might not read. But this and a few other things might benefit from some updating.

Now that I re-read the issue I think a --help flag that prints a high level summary would help. I'm sorry I missed that before but it would probably come in handy if you expect people to run that script on their own.

I'll also prepare an updated version of your issue and post it in a comment here, then you can decide whether you want to use some parts of it.

[I-Al-Istannen]

Feel free to suggest changes for the help and change what you feel is reasonable (nothing to more than listed here) in your issue comment.

I hope this is finally actually ready to be merged... Sorry about that.

---

Help output:

Outdated parts of the comment:

- To facilitate this, we have a script to find checkstyle errors in method Javadoc at chore/ci-checkstyle-javadoc.
+ To facilitate this, we have a script to find checkstyle errors in method Javadoc at chore/check-javadoc-regressions.

---

The script compatibility is a bit of a mixed bag. It should work everywhere, but if you add maven and git to your user path settings it might fail on Windows. We should probably still caution against using it on plain Windows.

---

- Bug: At the time of writing, the script doesn't work at all if you're on the master branch, so checkout to another branch to use it!

It works, but it doesn't really make much sense to compare master to master.

---

- chore/ci-checkstyle-javadoc.sh
+ chore/check-javadoc-regressions.py

---

- but do note that running in this mode destroys uncommitted changes. So don't run it unless you've committed all changes!
+ 

---

And we might want to mention the --help flag somewhere.

[slarse]

Thanks @I-Al-Istannen, very nice work! I'll update the issue.