Fix ARDUINO_LIB_DISCOVERY_PHASE automatic flag incompatiblity

[cmaglie]

Previously we added -DARDUINO_LIB_DISCOVERY_PHASE to the gcc command line during the library discovery phase: unfortunately this has an incompatiblity with compilers using non-standard -d flag instead of -D as reported here: #633 (comment)

To fix this behaviour this PR adds a new build variable {build.library_discovery_phase} that is set to 1 during library discovery or to 0 during normal build.

The platforms that wants to use it may add in their platform.txt something like:

# to keep backward compatibility...
build.library_discovery_phase=0

# define -D flag for gcc
build.library_discovery_phase_flag=-DARDUINO_LIBRARY_DISCOVERY_PHASE={build.library_discovery_phase}

and use {build.library_discovery_phase_flag} inside the gcc command line.

This way the gcc command line is not altered and all the other platforms not interested in this change will remain unaffected.

/cc @fpistm @facchinm

[matthijskooijman]

all the other platforms not interested in this change will remain unaffected.

Won't this change cause ARDUINO_LIBRARY_DISCOVERY_PHASE to become unset for all platforms until they apply the changes suggested above? And when they do apply the changes above and are used with older arduino-cli, the define will be applied twice, I think? Probably the latter one wins, so that should be the auto-added one then?

I guess this is one situation where something like https://groups.google.com/a/arduino.cc/forum/#!topic/developers/H6vCRlgRTCo would be helpful...

[cmaglie]

Won't this change cause ARDUINO_LIBRARY_DISCOVERY_PHASE to become unset for all platforms until they apply the changes suggested above?

yes

And when they do apply the changes above and are used with older arduino-cli, the define will be applied twice, I think?

uhm it shouldn't, I've imagined something like this: arduino/ArduinoCore-mbed#21

[matthijskooijman]

Oh, I missed the LIB vs LIBRARY difference. Gets complicated quickly, but I can't really think of any better way without some better versioning support...

[cmaglie]

Oh, I missed the LIB vs LIBRARY difference. Gets complicated quickly, but I can't really think of any better way without some better versioning support...

Yes it's a bit ugly... but this should allow the best compatibility. AFAIK mbed is the only platform using this define. New platforms that want's to use it from now can use directly the new definition and ignore the workaround.

I guess this is one situation where something like https://groups.google.com/a/arduino.cc/forum/#!topic/developers/H6vCRlgRTCo would be helpful...

I reread the thread a "platform.txt" versioning is surely something nice to have.
I don't know how much it would help in this particular case though, since an inadvertent breaking change has been implemented.

We may surely have designed the new feature in a less intrusive way from the beginning (like it is implemented in this PR for example) to be 100% sure it won't break anithing... but details are hard, is not always easy to figure out everything.

[fpistm]

Thanks @cmaglie

I've a question about the define ARDUINO_LIB_DISCOVERY_PHASE
https://github.com/arduino/ArduinoCore-mbed/blob/50b35d1f7c767280d40d30d8a38b3022fe8255ea/cores/arduino/Arduino.h#L20

It is not clear for me if a core want's to use it ?
This would allows to skip all the include resolutions from the core when searching lib dependencies? or something like that ?

[matthijskooijman]

Yes it's a bit ugly... but this should allow the best compatibility. AFAIK mbed is the only platform using this define. New platforms that want's to use it from now can use directly the new definition and ignore the workaround.

Well, it might not just be platforms that need this, but sketches or libraries could also use it (but if not all platforms offer it, then it's not really a usable API I guess). I don't have any examples of this, just theoretical.

I don't know how much it would help in this particular case though, since an inadvertent breaking change has been implemented.

It could help to make things simpler, since arduino-cli can detect that a platform was adapted or not, and continue to add the -D version to older version of the platform, so the platform no longer has to care (though this does not fix the "defined twice" when a newer platform is used with an older arduino-cli. That could be fixed on the longer term if stuff in platform.txt can be annotated with a version number (e.g. something like build.library_discovery_phase_flag.v2+ to only apply to newer arduino-cli, or maybe v2- to only apply to older arduino-cli, of course only down to the first version to support this syntax). I don't think I included this in my proposal, but it might be useful.

[cmaglie]

It is not clear for me if a core want's to use it ?

Only if your platform has a core made by a ton of header files that gets included from Arduino.h (like the mbed) and you're experiencing (very) long build time due to the lib discovery phase. Otherwise you don't need it and I guess this is the case for the majority of cores out there...