Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reintroduce -nostdinc for all system except macOS #7684

Merged
merged 11 commits into from
Nov 9, 2018

Conversation

lacasseio
Copy link
Contributor

macOS won't have -nostdinc until we support properly framework search
path discovery.

Context

Fixes gradle/gradle-native#925

Gradle Core Team Checklist

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

blop and others added 3 commits November 7, 2018 17:31
Signed-off-by: Olivier Voortman <olivier.voortman@gmail.com>
Instead of using toString().
macOS won't have `-nostdinc` until we support properly framework search
path discovery.
@lacasseio lacasseio added in:native-platform c, cpp, swift and other native languages support, etc from:member a:chore Minor issue without significant impact labels Nov 9, 2018
@lacasseio lacasseio added this to the 5.1 RC1 milestone Nov 9, 2018
@lacasseio lacasseio self-assigned this Nov 9, 2018
@pzygielo
Copy link

pzygielo commented Nov 9, 2018

expect or except?

…sh-code

Display only BuildCacheKey.getHashCode()
@big-guy big-guy self-requested a review November 9, 2018 15:11
@big-guy big-guy changed the title Reintroduce -nostdinc for all system expect macOS Reintroduce -nostdinc for all system except macOS Nov 9, 2018
Copy link
Member

@big-guy big-guy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small items.

@@ -33,7 +34,7 @@
private static class CCompileArgsTransformer extends GccCompilerArgsTransformer<CCompileSpec> {
@Override
protected boolean isNoStandardIncludes() {
return false;
return !OperatingSystem.current().isMacOsX();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a minor thing, but I think this should be based on the target platform (not that you could cross compile if you wanted to). We pass -nostdinc for everything except macOS.

@@ -32,7 +33,7 @@ public CPCHCompiler(BuildOperationExecutor buildOperationExecutor, CompilerOutpu
private static class CPCHCompileArgsTransformer extends GccCompilerArgsTransformer<CPCHCompileSpec> {
@Override
protected boolean isNoStandardIncludes() {
return false;
return !OperatingSystem.current().isMacOsX();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

@@ -33,7 +34,7 @@
private static class CppCompileArgsTransformer extends GccCompilerArgsTransformer<CppCompileSpec> {
@Override
protected boolean isNoStandardIncludes() {
return false;
return !OperatingSystem.current().isMacOsX();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

@@ -32,7 +33,7 @@ public CppPCHCompiler(BuildOperationExecutor buildOperationExecutor, CompilerOut
private static class CppPCHCompileArgsTransformer extends GccCompilerArgsTransformer<CppPCHCompileSpec> {
@Override
protected boolean isNoStandardIncludes() {
return false;
return !OperatingSystem.current().isMacOsX();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

@@ -34,7 +36,9 @@ class CCompilerTest extends GccCompatibleNativeCompilerTest {
@Override
protected List<String> getCompilerSpecificArguments(File includeDir, File systemIncludeDir) {
def arguments = super.getCompilerSpecificArguments(includeDir, systemIncludeDir)
arguments.remove('-nostdinc')
if (OperatingSystem.current().macOsX) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this just get moved up to the parent's method so we don't add -nostdinc to begin with?

@@ -35,7 +36,9 @@ class CPCHCompilerTest extends GccCompatibleNativeCompilerTest {
@Override
protected List<String> getCompilerSpecificArguments(File includeDir, File systemIncludeDir) {
def arguments = super.getCompilerSpecificArguments(includeDir, systemIncludeDir)
arguments.remove('-nostdinc')
if (OperatingSystem.current().macOsX) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

@@ -35,7 +36,9 @@ class CppCompilerTest extends GccCompatibleNativeCompilerTest {
@Override
protected List<String> getCompilerSpecificArguments(File includeDir, File systemIncludeDir) {
def arguments = super.getCompilerSpecificArguments(includeDir, systemIncludeDir)
arguments.remove('-nostdinc')
if (OperatingSystem.current().macOsX) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

@@ -35,7 +36,9 @@ class CppPCHCompilerTest extends GccCompatibleNativeCompilerTest {
@Override
protected List<String> getCompilerSpecificArguments(File includeDir, File systemIncludeDir) {
def arguments = super.getCompilerSpecificArguments(includeDir, systemIncludeDir)
arguments.remove('-nostdinc')
if (OperatingSystem.current().macOsX) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

@big-guy
Copy link
Member

big-guy commented Nov 9, 2018

@pzygielo except. For all platforms except for macOS, we want to restrict the include path used by the compiler to the set that Gradle knows about. We need to do a little more work to get this to work on macOS (to support Frameworks).

@big-guy big-guy merged commit 411adfd into master Nov 9, 2018
@lacasseio lacasseio deleted the lacasseio/native/nostdinc-linux branch November 10, 2018 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:chore Minor issue without significant impact in:native-platform c, cpp, swift and other native languages support, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants