Don't bail out if got include unrelated error while preprocessing #285
Conversation
Preprocessing is a bit tricky, since it expects an error to resolve missing includes; to do this, it invokes gcc with -CC flag (https://gcc.gnu.org/onlinedocs/gcc-4.4.0/gcc/Preprocessor-Options.html#Preprocessor-Options) to expand macro AND comments. Since this flag is not used during normal compilation, the errors reported are mostly bogus. This patch avoids writing the actual (non-include) error on stdout so the compilation can go on and fail on the proper errors (if there's any)
|
@ArduinoBot build this please |
|
I'm not so sure if this is really the right fix. Consider the following:
One could argue that even with the comment errors, the missing "B.h" will also generate an error message which builder can pick up during include scanning, but it is entirely possible that these comment errors will somehow interfere with the "B.h" include (for example when the include is guarded by an Having said that: In the case of #7930, the real problem is that include detection gives errors which are not present during normal compilation. Ideally, the preprocessor run will just do the first preprocessing step of normal compilation. Looking at the gcc manpage, shouldn't we just be passing |
|
✅ Build completed. ⬇️ Build URL: ℹ️ To test this build:
|
|
Macro expansion need to be applied because there is plenty of code like that would pick the wrong include without it. So Apart from that, the biggest problem in the current behaviour is that it reports errors that are not going to appear during compilation. Diagnostic may be improved by adding a line like |
Isn't this what -E does? If you read the |
Correct, so ideally we would prevent errors from happening that do not also happen during actual compilation. In this case, I think remove More generally, as I mentioned, hiding an error during include scanning seems contro-productive. Hiding the actual error, but instead showing a generic error as you propose could be a compromise (though having the real error output available right away might help diagnosing issues like these in the future). One other compromise could be to always show a "failed include scanning" error message as you propose, and in verbose mode add the actual compiler error? |
|
The problem that I see is that the error that would be displayed is not an error in fact, but only a byproduct of the flags we apply. Removing
|
If we drop
Correct, that is indeed tricky. We might implement some way to automatically apply such changes to older cores. I've been brooding on such an approach for a while and already wrote a specific proposal, but just now realized I showed the proposal to @cmaglie for a first review, but then forgot to send it to the developers list. I just sent the proposal there. If we would implement that proposal (or possible even without), we could let |
|
Alternative approach https://github.com/facchinm/arduino-builder/tree/avoid_error_on_preprocessor_alt_approach (based on #285 (comment)) |
|
✅ Build completed. ⬇️ Build URL: ℹ️ To test this build:
|
There was a problem hiding this comment.
Looks ok to me, at first glance. Some observations:
- This does not actually fix the problem with the marlin firmware. If the preprocessor error it runs into occurs after all include errors, it might actually compile with this fix, though the new error message remains. To really fix this issue, we should be looking at the
-CCoption as well (but both changes are independently useful, this PR now improves handling errors when the include scanning process differs from the main compilation process, while fiddling with-CCwould serve to remove those differences). - When such an error in preprocessing occurs, a message is now shown and include scanning continues with the next file, rather than bailing out. This is probably good, since it might allow compilation tot succeed anyway, but when it fails due to a missing library, the error message does provide a pointer to where the problem lies.
- I think the current implementation will happily add the "no includes found" to the cache, which means that on a subsequent compilation, the error message is not shown. To fix this, I would suggest not adding the empty result to the cache, which causes the cache to be invalidated on a subsequent run. This does mean that effectively no include detection after the error can be cached (but fixing that requires complicating the cache file structure, which is probably not worth it). I've added an inline comment with an implementation suggestion.
container_find_includes.go
Outdated
| // No include found? Bail out. | ||
| os.Stderr.Write(preproc_stderr) | ||
| return i18n.WrapError(preproc_err) | ||
| ctx.GetLogger().Println(constants.LOG_LEVEL_DEBUG, constants.MSG_FIND_INCLUDES_FAILED, sourcePath) |
There was a problem hiding this comment.
I would add here:
// When no include is found, but gcc exited with an error, something
// else failed that we cannot fix, so stop processing this file.
// This intentionally does *not* update the cache, to make sure that
// on the next run, the error is shown again.
return nil
just tried this case, and the preprocessor actually finds the missing |
|
This is the test I've tried: #include <avr/io.h>
#include <Arduino.h>
#define DIO54_PIN PINF0
#define DIO54_RPORT PINF
#define DIO54_WPORT PORTF
#define DIO54_DDR DDRF
#define DIO54_PWM NULL
#define OUT_WRITE(IO, v) do{ SET_OUTPUT(IO); }while(0)
#define _SET_OUTPUT(IO) do {DIO ## IO ## _DDR |= _BV(DIO ## IO ## _PIN); } while (0)
#define SET_OUTPUT(IO) _SET_OUTPUT(IO)
#define SUICIDE_PIN 54 // Must be enabled at startup to keep power flowing
void argh() {
OUT_WRITE(SUICIDE_PIN, HIGH);
}
wrpwefèßðđ#òdsafs²ò43²4àò¹²4idsjmiofvsmniovsdckdsfnkjwnerr
>><;:>,.>;.;<)(/&%&/!($(£"()&$%&)(!"($£&!(&()$!$
#include <SPI.h>and if I manually run the discovery build: as you can see, even with the worst garbage in the middle, the preprocessor will always run until the end and find the include. |
|
✅ Build completed. ⬇️ Build URL: ℹ️ To test this build:
|
Preprocessing is a bit tricky, since it expects an error to resolve missing includes; to do this, it invokes gcc with -CC flag (https://gcc.gnu.org/onlinedocs/gcc-4.4.0/gcc/Preprocessor-Options.html#Preprocessor-Options) to expand macro AND comments. Since this flag is not used during normal compilation, the errors reported are mostly bogus.
This patch avoids writing the actual (non-include) error on stdout so the compilation can go on and fail on the proper errors (if there's any)
This patch fixes arduino/Arduino#7930
@matthijskooijman since it modifies a bit the behaviour introduced in fcc9c5d , could you give it a check before merging? Thanks