nokeedev / gradle-native

The home of anything about Gradle support for natively compiled languages
https://nokee.dev
Apache License 2.0
47 stars 8 forks source link

Core Gradle header dependencies doesn't account for headers that can be included multiple time #861

Open lacasseio opened 2 months ago

lacasseio commented 2 months ago

In Gradle's IncrementalCompileFilesFactory, cycles (aka already visited headers) are ignored. However, in preprocessor libraries, cycles may be intended. The header guards dictate if a cycle should be followed or not. Without a header guard, a header could be included multiple times with a different set of macros to resolve the header against. This may cause some headers to be omitted from the resulting graph. Detecting the header guard may be too hard given 1) the large possibility to guard headers (generally, we only think about #ifdef/#define and #pragma once, but it can be much more complicated) and 2) the fact that Gradle's header dependency is not meant to be a 1-to-1 representation of the actual header graph. We also need to avoid any possible cycle that would cause a stack overflow. However, we should account for the potential of new headers that may be resolved thanks to an updated visibleMacros. A sensible approach could be to 1) check if the cycle header provides any "new" state to resolve, e.g. if the header has an #include macro of a newly discovered macro, or we could 2) use a global approach where we aggregate resolvable expression and successfully resolve them against each discovered macros. Note the latter approach differs widely from the normal header resolution process, but let's keep in mind that Gradle's goal is not to make 1-to-1 the compiler's behaviour but to provide a good enough (performance and accuracy-wise) representation of the up-to-dateness and cache key for a compile task.

lacasseio commented 2 months ago

Actually, the cycle detection is only meant when the file is currently being visited (the result being calculated). If the result is available, the visibleMacros will be merged. So, the behaviour should be close to option 2. More documentation should be written about this piece of code.