Support for C++ source compatibility #8055

k-mack · 2018-12-17T01:53:52Z

Context

sourceCompatibility is added to the new C++ plugins for the GCC and Clang toolchains, enabling users to declare a C++ binary's source compatibility. The introduced source compatibilities are Cpp98, Cpp03, Cpp11, Cpp17, and Cpp2a. The CppSourceCompatibility enum will have to be updated with any future compatibilities. In addition, GccVersionCppSourceCompatibility and ClangVersionCppSourceCompatibility encapsulate the logic that translates a CppSourceCompatibility into the correct command-line argument for the compiler. This requires knowledge of the compiler's version.

Contributor Checklist

Review Contribution Guidelines
Make sure that all commits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team
Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective
Provide unit tests (under <subproject>/src/test) to verify logic
Update User Guide, DSL Reference, and Javadoc for public-facing changes
Ensure that tests pass locally: ./gradlew <changed-subproject>:check

Gradle Core Team Checklist

Verify design and implementation
Verify test coverage and CI build status
Verify documentation
Recognize contributor in release notes

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

At this point, the selected toolchain should support the selected source compatibility. Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

If source compatibility is not selected by the time a toolchain is selected, then default the source compatibility to the toolchain's default compatibility. This is currently only implemented for GCC and Clang compilers. Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Also makes source compatibility an optional input to the task. Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Unit test caught a bug when testing Cpp17 for clang 3.5. Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

lacasseio

Thanks @k-mack for looking into this. I believe it's a big step in the right direction. The change I request are mostly polishing and to avoid waterfall changes everywhere. Great work so far 👍

...age-native/src/integTest/groovy/org/gradle/language/cpp/CppApplicationIntegrationTest.groovy

lacasseio · 2018-12-27T15:31:23Z

...c/main/java/org/gradle/nativeplatform/toolchain/internal/gcc/GccCompilerArgsTransformer.java

+                }
+            }
+        }
+    }


❌ Same as above, moving this method to CppCompilerArgsTransformer simplify it greatly. For starter, the spec is already typed as CppCompileSpec. To work around the need for compiler type, I would simply create 2 types just as we did for Visual C++. I would keep the two distinct type for C++ compiler only. This will allow you to avoid passing the compiler type at a constructor argument.

lacasseio · 2018-12-27T15:41:51Z

subprojects/language-native/src/test/groovy/org/gradle/language/cpp/tasks/CppCompileTest.groovy

@@ -53,6 +55,7 @@ class CppCompileTest extends AbstractProjectBuilderSpec {
        cppCompile.objectFileDir = temporaryFolder.file("outputFile")
        cppCompile.source sourceFile
        cppCompile.setPreCompiledHeader pch
+        cppCompile.sourceCompatibility = Cpp11


❌ There is a missing coverage when the source compatibility isn't set. It should result in the spec property to be null.

I added another test to demonstrate this behavior. Let me know what you think.

lacasseio · 2018-12-27T16:01:36Z

...tform-native/src/main/java/org/gradle/nativeplatform/toolchain/internal/gcc/CppCompiler.java

-    CppCompiler(BuildOperationExecutor buildOperationExecutor, CompilerOutputFileNamingSchemeFactory compilerOutputFileNamingSchemeFactory, CommandLineToolInvocationWorker commandLineToolInvocationWorker, CommandLineToolContext invocationContext, String objectFileExtension, boolean useCommandFile, WorkerLeaseService workerLeaseService) {
-        super(buildOperationExecutor, compilerOutputFileNamingSchemeFactory, commandLineToolInvocationWorker, invocationContext, new CppCompileArgsTransformer(), Transformers.<CppCompileSpec>noOpTransformer(), objectFileExtension, useCommandFile, workerLeaseService);
+    CppCompiler(BuildOperationExecutor buildOperationExecutor, CompilerOutputFileNamingSchemeFactory compilerOutputFileNamingSchemeFactory, CommandLineToolInvocationWorker commandLineToolInvocationWorker, CommandLineToolContext invocationContext, String objectFileExtension, boolean useCommandFile, WorkerLeaseService workerLeaseService, CompilerType compilerType, VersionNumber compilerVersion) {
+        super(buildOperationExecutor, compilerOutputFileNamingSchemeFactory, commandLineToolInvocationWorker, invocationContext, new CppCompileArgsTransformer(compilerType, compilerVersion), Transformers.<CppCompileSpec>noOpTransformer(), objectFileExtension, useCommandFile, workerLeaseService);


❌ Instead of passing the version inside the constructor and causing lots of waterfall changes, I think we should simply set the version on the spec here. You would be required to add CompilerVersion getter/setter on CompileSpec. You will need to add test coverage for version aware compiler wrapped vs normal compiler. There isn't any yet, so just add a unit test Specification as VersionAwareCompilerTest and mock the wrapped compiler to assert the version is properly set on the CompileSpec.

The CompilerVersion most likely contains the String that would identify Clang vs GCC, however, I would still create typed CppCompiler for both tool chain simply because we want to avoid comparing strings and such.

Should we really be adding this getter/setter on CompileSpec? It's pretty high-level interface, and it would require changing a number of classes unrelated to this PR (e.g., JavadocSpec).

lacasseio · 2018-12-27T16:04:25Z

...c/main/java/org/gradle/nativeplatform/toolchain/internal/gcc/GccCompilerArgsTransformer.java

+        this.compilerType = compilerType;
+        this.compilerVersion = compilerVersion;
+    }
+


❌ This would go away with the change explained above.

lacasseio · 2018-12-27T16:04:51Z

.../src/main/java/org/gradle/nativeplatform/toolchain/internal/gcc/GccPlatformToolProvider.java

        OutputCleaningCompiler<CppCompileSpec> outputCleaningCompiler = new OutputCleaningCompiler<CppCompileSpec>(cppCompiler, compilerOutputFileNamingSchemeFactory, getObjectFileExtension());
-        return versionAwareCompiler(outputCleaningCompiler, ToolType.CPP_COMPILER);
+        return versionAwareCompiler(outputCleaningCompiler, gccMetadata);


❌ This would also go away with the change explained above.

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Prior to clang 6, the default C++ dialect is GNU C++98. From clang 6 and after, the default dialect is GNU C++14. This change also recognizes that the default dialects use the GNU C++ extensions as state in the clang documentation. Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

After rereading the GCC documentation, the default C++ dialect, if std is not chosen, is GNU C++. Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

k-mack · 2019-02-03T19:04:27Z

@lacasseio should this be closed and restarted? Since #8222 was merged, I'm assuming source compatibility should be put in the CppPlatform to align with the goals of gradle/gradle-native#963. Either way, let me know how we should proceed.

stale · 2020-08-05T14:33:47Z

This pull request has been automatically marked as stale because it has not had recent activity. Given the limited bandwidth of the team, it will be closed if no further activity occurs. If you intend to work on this pull request, please reopen the PR. Thank you for your contributions.

stale · 2020-08-26T14:59:45Z

This pull request has been automatically closed due to inactivity. If you are still interested in contributing this, please ensure that it is rebased against the latest branch (usually master), all review comments have been addressed and the build is passing.

k-mack added 18 commits December 12, 2018 20:37

Add CppSourceCompatibility enum

73d01bf

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Add src compat property to CppComponent & CppCompile

2621cb7

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Rename enum

05e5ee5

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Initial logic to determine std arg for gcc & clang

0f6ab15

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Initial logic to wire DSL to CppCompile

af3defb

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Revert rename enum

d882753

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Toolchain must support src compat or throw IAE

941de7b

At this point, the selected toolchain should support the selected source compatibility. Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Add compiler type & version to GCC arg transformer

46cb79c

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Fix compiler errors

8634ff9

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Add config src compat in compile task

d2bf3ea

Also makes source compatibility an optional input to the task. Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Fix compiler errors

151ff04

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Add test for clang src compat

e91a9bc

Unit test caught a bug when testing Cpp17 for clang 3.5. Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Add integ tests that use sourceCompatibility

d037d98

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Add test for gcc source compatibility & fix bug

de5f935

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Add more clang source compatibility tests

1f808f9

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Add test for recompile when src compat changes

d353dbc

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Bump newly introduced public APIs to 5.2

870036c

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

k-mack force-pushed the km/cpp-src-compat branch from 0d59354 to 870036c Compare December 17, 2018 05:28

lacasseio suggested changes Dec 27, 2018

View reviewed changes

k-mack added 10 commits December 29, 2018 10:58

Split test

d7ccc95

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Move common cpp app & lib tests to parent test spec

1c526c2

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Rename vars of type CppSourceCompatibilities to sourceCompatibility

f35fe90

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Do not inject objectfactory into task constructor

eac78d8

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Add test coverage for source compatibility not set on CppCompile task

812ba28

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Fix gcc's default C++ dialect to GNU extensions

56aea47

After rereading the GCC documentation, the default C++ dialect, if std is not chosen, is GNU C++. Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Avoid abbreviating IllegalArgumentException

d1ddc5a

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Remove compiler specifics in CppSourceCompatibility javadocs

dabba6e

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Support applying default source compatibility for VS on a cpp binary

f29346b

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

Configure source compatibility at component level in tests

1955d51

Signed-off-by: Kevin Macksamie <kevin.macksamie@gmail.com>

big-guy added the from:contributor label Jan 14, 2019

big-guy added the @core Issue owned by GBT Core label Jun 19, 2019

lacasseio added the in:native-platform c, cpp, swift and other native languages support, etc label Sep 11, 2019

big-guy added this to the 6.2 RC1 milestone Nov 5, 2019

big-guy modified the milestones: 6.2 RC1, 6.3 RC1 Jan 27, 2020

big-guy modified the milestones: 6.3 RC1, 6.4 RC1 Mar 2, 2020

big-guy removed this from the 6.4 RC1 milestone Mar 10, 2020

stale bot added the stale label Aug 5, 2020

stale bot closed this Aug 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for C++ source compatibility #8055

Support for C++ source compatibility #8055

k-mack commented Dec 17, 2018

lacasseio left a comment

lacasseio Dec 27, 2018

lacasseio Dec 27, 2018

k-mack Dec 30, 2018

lacasseio Dec 27, 2018

k-mack Dec 30, 2018

lacasseio Dec 27, 2018

lacasseio Dec 27, 2018

k-mack commented Feb 3, 2019

stale bot commented Aug 5, 2020

stale bot commented Aug 26, 2020

Support for C++ source compatibility #8055

Support for C++ source compatibility #8055

Conversation

k-mack commented Dec 17, 2018

Context

Contributor Checklist

Gradle Core Team Checklist

lacasseio left a comment

Choose a reason for hiding this comment

lacasseio Dec 27, 2018

Choose a reason for hiding this comment

lacasseio Dec 27, 2018

Choose a reason for hiding this comment

k-mack Dec 30, 2018

Choose a reason for hiding this comment

lacasseio Dec 27, 2018

Choose a reason for hiding this comment

k-mack Dec 30, 2018

Choose a reason for hiding this comment

lacasseio Dec 27, 2018

Choose a reason for hiding this comment

lacasseio Dec 27, 2018

Choose a reason for hiding this comment

k-mack commented Feb 3, 2019

stale bot commented Aug 5, 2020

stale bot commented Aug 26, 2020