Fix implicit declaration compiler warnings (which are errors on some boards) #66
Conversation
|
Memory usage change @ 59cfe4d
Click for full report table
Click for full report CSV |
|
@per1234 - Some libraries, this one being a prime example, have bugs that the author should have seen as compiler warnings. Most likely they unwittingly had all warnings disabled in File > Preferences. I guess I'm wondering, would it be possible for github actions to automatically show compiler warnings? Ideally a commit would make it clear which warnings already exists and if the proposed change solve already-existing warnings and/or adds new ones. |
|
This is something I have investigated, and even done some initial work on. A simple and standard approach to surfacing warning and error messages is the use of a "problem matcher". This will cause the workflow run logs to be decorated as well as the commit itself. You can see a demo of the log decoration that would have occurred if there was a problem matcher in the sketch compilation CI workflow here:
The problem, as you have pointed out, is that we aren't always starting from a warning-free code base, where any warning indicates a problem with the proposed change. The warnings surfaced by the problem matcher were not necessarily introduced by the commit/PR. They might even originate from the code of external dependencies not in the repository. So this problem matcher approach to warnings might cause confusion for contributors. I haven't made up my mind yet on whether it would have a net beneficial effect in the CI system of an Arduino library repository. As you say, what is relevant for evaluating a proposed change is whether it introduces new warnings. My idea is that this can be indicated in a manner similar to the "size deltas report" you see in the comment above. This is determined by compiling the sketch, recording the data gathered from that compilation, then checking out the base ref (in the case of a PR) or parent commit (in the case of a push) and the repeating the process. The "delta" is the difference in the data between the two compilations. I have already added the capability to the The problem I ran into while adding the warnings delta capability to the action is that warnings for previously cached compilations are not shown (arduino/arduino-cli#1008), causing inaccurate warning counts after the first compilation. The workaround for that is to clear the cache before compiling. This means the feature has significant overhead as far as CI run duration goes, so I added an input to the action to control the feature and made it default to being off. Because it isn't clear to me that the benefit of these lines hidden away in a workflow run log most people never look at outweigh the harm of an increased CI run duration, I have not enabled warning reports in Arduino's sketch compilation workflows, The missing piece is the prominently visible report made via a comment in the pull request. The groundwork has been laid for that by making the compilation action record the data, as well as by already having the My "warning deltas" approach is fairly naïve in that a PR which fixed an innocuous warning and introduced a significant one would be seen as having a delta of 0. However, I still think it can have a lot of value when compared to the current situation even if it doesn't report on every single warning introduction. |
|
Sorry, I lost track of this PR yesterday and I applied 814321c . Closing the issue as duplicate, of course all the discussion about making the CI very picky is still relevant and can be continued either here or in a brand new issue (maybe in API repo? ) |
This fix might be called "trivial" but it fixes a compiler warning, and with some boards that use inline code for delayMicroseconds, it is an error rather than just a warning.