search

arduino / compile-sketches

GitHub Actions action that checks whether Arduino sketches compile and produces a report of data from the compilations
GNU General Public License v3.0
69 stars 13 forks source link

Adding Problem Matchers to display compilation errors as annotation. #27

Open peternewman opened 3 years ago

peternewman commented 3 years ago

Please can we have problem matchers? I assume there isn't some reason you aren't using them?

Given it's a composite action, it looks like it's probably as simple as loading https://github.com/marketplace/actions/gcc-problem-matcher before compiling.

peternewman commented 3 years ago

30 seconds testing suggests it works: https://github.com/peternewman/DmxSerial2/actions/runs/919810906

But due to the alternative path it doesn't correctly match and find the files out of the box.

I think fromPath will fix that, but needs a bit more digging: https://github.com/actions/toolkit/blob/master/docs/problem-matchers.md

per1234 commented 3 years ago

it looks like it's probably as simple as loading https://github.com/marketplace/actions/gcc-problem-matcher before compiling.

Way cool! This is something I've been thinking about for a while, but never got around to investigating.

But due to the alternative path it doesn't correctly match and find the files out of the box.

You're right that the library file paths in the compiler output don't match with the path in the repository due to the action having installed the library. However, it seems to find it anyway, I don't know how: https://github.com/per1234/DmxSerial2/commit/8d2966078d05fc5e53c6b045bfb0cae9570ed2e2

In your demo, I think the problem is that the library code isn't in the commit diff.

peternewman commented 3 years ago

it looks like it's probably as simple as loading https://github.com/marketplace/actions/gcc-problem-matcher before compiling.

Way cool! This is something I've been thinking about for a while, but never got around to investigating.

Heh, in some ways they are the main benefit I see to actions over where I was using Travis before for stuff, and I guess some slightly tighter integration of which specific task failed without having to visit another webpage.

But due to the alternative path it doesn't correctly match and find the files out of the box.

You're right that the library file paths in the compiler output don't match with the path in the repository due to the action having installed the library. However, it seems to find it anyway, I don't know how: per1234/DmxSerial2@8d29660

Indeed it does. I wonder if it just matches the right hand side somewhere in the path?

In your demo, I think the problem is that the library code isn't in the commit diff.

Ah yes, possibly, they seem to keep changing their mind on how that all behaves, I've definitely seen flake8 runs annotate for errors in bits of code that weren't changed in the commit/PR.

I guess I better open a PR then...

Probably need to see if that's actually the best GCC problem matcher available first...

peternewman commented 3 years ago

So it looks like the options are:

So it's probably between

Does Arduino run linkers or anything clever? Do people use make much/at all? Can that even be triggered from this action?

per1234 commented 3 years ago

Does Arduino run linkers

Yes: https://arduino.github.io/arduino-cli/latest/platform-specification/#recipes-for-linking Each boards platform author can choose what happens in that recipe. There are hundreds of platforms covering a wide array of architectures, so it's difficult to make universal generalizations about what happens when compiling an Arduino sketch.

Can that even be triggered from this action?

Although I haven't seen it, a boards platform author could use make as part of the compilation process if they wanted. Where I have seen make used by the Arduino community is directly as a replacement for the standard Arduino build system. This action would not be applicable for that approach because it is centered around the use of the standard Arduino build system via Arduino CLI.

per1234 commented 3 years ago

I've been doing a bit more investigation and experimentation with problem matchers lately. I am now using liskin/gh-problem-matcher-wrap in some non-sketch compilation workflows (flake8, pytest, ShellCheck) and have been happy with it so far in that context.

Although I think they are really cool and useful, I also see some issues caused by the use of problem matchers:

Warnings might have already been in the project prior to the contribution, and in some cases not represent a real problem, but them being surfaced in the pull request context could cause contributors to think they indicate a problem with the pull request.

Another problem I've encountered is related to my standard practice is to add both a push and a pull_request event triggers to the workflows I write. The reason is that the push event trigger is valuable for checking a contribution for problems before submitting a pull request, and the pull_request event essential for checking the PRs. But this also means you can get redundant workflow runs, which results in double annotations. We also are almost always using matrix jobs to compile for multiple boards, which means that might be multiplied, as demonstrated here: https://github.com/per1234/ArduinoModbus/pull/2/files

These things mean there is a potential for problem matchers to make a project less contributor friendly. My feeling is that the maintainers of Arduino library and sketch repositories should put extra attention to making them approachable to those who are just getting started with contributing to open source projects. So I'm not yet decided on whether I think it's a good idea to add a warning level problem matcher to Arduino's own sketch compilation workflows. I think it would at least be worth considering use of an error-only problem matcher though.

I don't see a strong benefit in this action having a built-in problem matcher capability when it's reasonably easy to add and maintain this capability by adding an extra step or two to the workflow. So I'm leaning towards thinking that this is out of scope for this repository. Do you have any thoughts on that @peternewman and @aentinger?

peternewman commented 3 years ago

Does Arduino run linkers

Yes:

In which case you may want to match linker output too.

I've been doing a bit more investigation and experimentation with problem matchers lately. I am now using liskin/gh-problem-matcher-wrap in some non-sketch compilation workflows (flake8, pytest, ShellCheck) and have been happy with it so far in that context.

I'm a bit curious of the benefit of this over just using the native actions. I guess it gives a more raw option to choose your own CLI options.

Warnings might have already been in the project prior to the contribution, and in some cases not represent a real problem, but them being surfaced in the pull request context could cause contributors to think they indicate a problem with the pull request.

Yes, fair point, especially with warnings. I wonder if there's a clever tool already, or it could be added to https://github.com/liskin/gh-problem-matcher-wrap or similar, to grep out lines depending on at least which file they were in, or possibly if they were lines touched by the PR.

Another problem I've encountered is related to my standard practice is to add both a push and a pull_request event triggers to the workflows I write.

Agreed, it's certainly irritating. This seems to be a relevant issue, with no obvious movement from GitHub: https://github.com/actions/runner/issues/504

These things mean there is a potential for problem matchers to make a project less contributor friendly. My feeling is that the maintainers of Arduino library and sketch repositories should put extra attention to making them approachable to those who are just getting started with contributing to open source projects. So I'm not yet decided on whether I think it's a good idea to add a warning level problem matcher to Arduino's own sketch compilation workflows. I think it would at least be worth considering use of an error-only problem matcher though.

Yes, as you say, error ones seem a no-brainer. I guess my more general thought is you should probably have a matcher for everything that's going to stop a PR being merged. If you require lint, or sorting or some warnings to be clean, then flagging it as annotation seems far more friendly to me than expecting someone to read through CI output and understand what's at fault. Even something like codespell, there's a lot of noise in the action compared to just a list of typos.

I don't see a strong benefit in this action having a built-in problem matcher capability when it's reasonably easy to add and maintain this capability by adding an extra step or two to the workflow. So I'm leaning towards thinking that this is out of scope for this repository. Do you have any thoughts on that @peternewman and @aentinger?

My thinking is the actions should make things easy for the developers deploying them, if we're agreeing error matchers are beneficial, why not have them built into this by default, maybe with an option to match errors/errors+warnings/nothing. ~~Composite actions might be relevant: https://docs.github.com/en/actions/creating-actions/creating-a-composite-run-steps-action~~ You already are, so it's there already!

At the very least, it would be good to recommend a particular annotation and give an example of how to set it up along with this action.