gradle / gradle-native

The home of Gradle's support for natively compiled languages
https://blog.gradle.org/introducing-the-new-cpp-plugins
Apache License 2.0
92 stars 8 forks source link

Unresolved macro include should snapshot relatively included headers #911

Open lacasseio opened 5 years ago

lacasseio commented 5 years ago

Expected Behavior

Relative included headers are part of the compile input header set fall back when encountering macro include.

Current Behavior

Header included relatively aren't considered as part of the fall back mechanic when encountering macro include.

Context

Given Gradle prone correctness, we should consider relative headers event when encountering unresolved macro includes.

Open Questions?

Should we only consider relative include from the discovered headers in the partial graph that is built or consider relative include from all headers in all include paths?

Steps to Reproduce (for bugs)

The following CppIncrementalBuildIntegrationTest test case show the issue:

    def "considers all header files including relative headers as input to source file with complex macro include #include"() {
        def include = 'MISSING'
        def text = '''
            #ifdef MISSING
            #include MISSING
            #else
            #include "hello.h"
            #endif
        '''

        when:
        appSourceFile.text = """
            $text
            #include "../relative-header.h"
            #include <iostream>

            int main () {
              std::cout << GREETING << " " << NAME;
              return 0;
            }
        """

        def relativeHeaderFile = file ("app/src/main/relative-header.h") << """
            #define NAME "John"
        """

        file("app/src/main/headers/hello.h") << """
            #define GREETING "hello"
        """

        then:
        executer.withArgument("-i")
        succeeds installApp
        output.contains("Cannot locate header file for '#include $include' in source file 'main.cpp'. Assuming changed.")
        install.exec().out == "hello John"
        // Test assumes there are 2 source files: one with unresolvable macros and one without. Verify that assumption
        appObjects.hasFiles(appSourceFile, appOtherSourceFile)

        when:
        relativeHeaderFile.text << "// changed"
        appObjects.snapshot()
        libObjects.snapshot()

        then:
        executer.withArgument("-i")
        succeeds installApp

        and:
        appObjects.recompiledFiles(appSourceFile)
        libObjects.noneRecompiled()

        and:
        executedAndNotSkipped appDebug.compile
        skipped libraryDebug.compile
        unresolvedHeadersDetected(appDebug.compile)

        when:
        succeeds installApp

        then:
        nonSkippedTasks.empty

        when:
        disableTransitiveUnresolvedHeaderDetection()
        relativeHeaderFile.text = "// changed again"

        appObjects.snapshot()
        libObjects.snapshot()

        then:
        succeeds installApp

        and:
        appObjects.noneRecompiled()
        libObjects.noneRecompiled()

        and:
        skipped appDebug.compile
        skipped libraryDebug.compile
    }

It fails at relativeHeaderFile change with the following error:

appObjects.recompiledFiles(appSourceFile)
|          |               |
|          |               /Users/daniel/gradle/gradle/build/tmp/test files/unknown-test-class/ire5a/app/src/main/cpp/main.cpp
|          Assertion failed: 
|           
|          assert changedFileNames == expectedNames
|                 |                |  |
|                 []               |  ['main']
|                                  false

Your Environment

big-guy commented 5 years ago

Can you check-in this test and mark it @NotYetImplemented or @Ignore? We'll pick this up as part of our regular cycle

My suspicion is that we're not including the path to the source file as part of an implicit include path.

lacasseio commented 5 years ago

Maybe we talk about the same thing here. It seems to me that when we find an unresolved macro include, we give up on the analysis and potential relatively include headers. It would improve the fallback case. However, care should be taken to improve the analysis to avoid fallback scenario.