Polish Xcode support for source dependencies #3660

lacasseio · 2017-11-30T01:38:51Z

Context

Gradle Core Team Checklist

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

adammurdoch · 2017-11-30T20:27:09Z

...tegTest/groovy/org/gradle/ide/xcode/XcodeCppExternalSourceDependenciesIntegrationTest.groovy

+        succeeds 'xcode'
+
+        then:
+        executedAndNotSkipped(":xcodeProject", ":xcodeProjectWorkspaceSettings", ":xcodeSchemeAppExecutable", ":xcode")


We should have built the binaries for the greeter library as well, otherwise this workspace isn't going to be useable.

We should add some coverage for the case where the headers are buildable, and make sure they get built.

I may be wrong, but I don't think we have any coverage for buildable headers for Xcode. I wouldn't pull this feature in this PR as it would mean adding it to all of Xcode and will most likely send the PR into a rabbit hole. I would try to stay focus on a single feature and do another PR for this use case instead.

With regards to building the greeter library, we don't really need to build it as Xcode delegate to Gradle on every build which will ensure that greeter library is built.

adammurdoch · 2017-11-30T20:28:48Z

...tegTest/groovy/org/gradle/ide/xcode/XcodeCppExternalSourceDependenciesIntegrationTest.groovy

+        then:
+        executedAndNotSkipped(":xcodeProject", ":xcodeProjectWorkspaceSettings", ":xcodeSchemeAppExecutable", ":xcode",
+            ":greeter:xcodeProject", ":greeter:xcodeProjectWorkspaceSettings", ":greeter:xcodeSchemeGreeterSharedLibrary")
+        rootXcodeWorkspace.contentFile.assertHasProjects("${rootProjectName}.xcodeproj", "${checkoutRelativeDir(repo.name, commit.id.name, repo.id)}/greeter.xcodeproj")


we don't want the greeter library to be included in the workspace as a project, but as binaries.

adammurdoch · 2017-11-30T20:29:54Z

...gTest/groovy/org/gradle/ide/xcode/XcodeSwiftExternalSourceDependenciesIntegrationTest.groovy

+        succeeds 'xcode'
+
+        then:
+        executedAndNotSkipped(":xcodeProject", ":xcodeProjectWorkspaceSettings", ":xcodeSchemeAppExecutable", ":xcode")


should build the binaries of greeter library

you should update the test to show that the binaries of the greeter library are built. I'm not seeing any changes here.

adammurdoch · 2017-11-30T20:31:02Z

...gTest/groovy/org/gradle/ide/xcode/XcodeSwiftExternalSourceDependenciesIntegrationTest.groovy

+
+        then:
+        executedAndNotSkipped(":xcodeProject", ":xcodeProjectWorkspaceSettings", ":xcodeSchemeAppExecutable", ":xcode",
+            ":greeter:xcodeProject", ":greeter:xcodeProjectWorkspaceSettings", ":greeter:xcodeSchemeGreeterSharedLibrary")


should build the binaries of the greeter library, not the project files.

adammurdoch · 2017-11-30T20:31:24Z

...gTest/groovy/org/gradle/ide/xcode/XcodeSwiftExternalSourceDependenciesIntegrationTest.groovy

+        then:
+        executedAndNotSkipped(":xcodeProject", ":xcodeProjectWorkspaceSettings", ":xcodeSchemeAppExecutable", ":xcode",
+            ":greeter:xcodeProject", ":greeter:xcodeProjectWorkspaceSettings", ":greeter:xcodeSchemeGreeterSharedLibrary")
+        rootXcodeWorkspace.contentFile.assertHasProjects("${rootProjectName}.xcodeproj", "${checkoutRelativeDir(repo.name, commit.id.name, repo.id)}/greeter.xcodeproj")


should not include the greeter library as a project in the workspace, but as binaries.

adammurdoch · 2017-11-30T20:40:46Z

...tegTest/groovy/org/gradle/ide/xcode/XcodeCppExternalSourceDependenciesIntegrationTest.groovy

+
+import static org.gradle.ide.xcode.internal.XcodeUtils.toSpaceSeparatedList
+
+class XcodeCppExternalSourceDependenciesIntegrationTest extends AbstractXcodeIntegrationSpec {


Should add some test coverage where the root project is only a container that does not define any components, and there are 2 subprojects with an application defined in one and library in the other.

Should also cover the case where there are sub projects that don't define any components.

adammurdoch · 2017-11-30T20:42:40Z

...ojects/ide-native/src/main/java/org/gradle/ide/xcode/tasks/GenerateXcodeProjectFileTask.java

-            debugSettings.put("SWIFT_INCLUDE_PATHS", toSpaceSeparatedList(xcodeTarget.getCompileModules()));
-            releaseSettings.put("SWIFT_INCLUDE_PATHS", toSpaceSeparatedList(xcodeTarget.getCompileModules()));
-            testRunnerSettings.put("SWIFT_INCLUDE_PATHS", toSpaceSeparatedList(xcodeTarget.getCompileModules()));
+            debugSettings.put("SWIFT_INCLUDE_PATHS", toSpaceSeparatedList(parentDirs(xcodeTarget.getCompileModules())));


I think I broke that.

It's not that it was broken, it's just that it didn't behave like everything else.

adammurdoch · 2017-11-30T20:45:10Z

...de/src/integTest/groovy/org/gradle/plugins/ide/idea/IdeaCompositeBuildIntegrationTest.groovy

+
+        def ipr = getFile([:], 'root.ipr').text
+        assert ipr.contains('''  <component name="ProjectModuleManager">
+    <modules>


isn't there a fixture method that does this? Perhaps add it if not.

adammurdoch · 2017-11-30T20:51:21Z

...tive/src/main/java/org/gradle/nativeplatform/test/xctest/plugins/XCTestConventionPlugin.java

-                            return regularFile.getAsFile().getParentFile();
-                        }
-                    }))));
+                    .add(new DefaultSelfResolvingDependency((FileCollectionInternal)project.files(((DefaultSwiftBinary)testedComponent.getDevelopmentBinary()).getModuleFile())));


I think we can replace the construction of a DefaultSelfResolvingDependency with a call to project.getDependencies().create(project.files(testedComponent.getDevelopmentBinary().getModuleFile())

I will look into that.

adammurdoch · 2017-11-30T20:53:32Z

subprojects/ide/src/main/java/org/gradle/plugins/ide/idea/model/IdeaProject.java

-            LocalComponentArtifactMetadata imlArtifact = localComponentRegistry.findAdditionalArtifact(otherProjectId, "iml");
-            if (imlArtifact != null) {
-                xmlProject.addModulePath(imlArtifact.getFile());
+        Iterator<IdePlugin> it = project.getPlugins().withType(IdePlugin.class).iterator();


This logic should live in the plugin. The plugin should create a lazy list of some kind and attach it to this model object, rather than having the model reach across and try to figure out which plugin created it. This removes some complexity.

I was trying to move the logic out of there, but I kept running into backward compatibility. If we diverge too much from what we have now, we may break a lot of people that may be using the model in some untested ways. We may be alright with such breakage. I will look at the code again to see if I can remove this.

The thing to do here would be to make Project.modulePaths as lazily configured property. That is, add a new property of type SetProperty<Path> and have the old methods delegate to this. Then, the IDEA plugin can add a Provider<Path> to this that provides the convention.

We'd do something similar on the Eclipse and Xcode (and Visual studio) models as well.

Oh, and we'd have to add SetProperty too.

I'd get the Xcode project generation happening correctly first in this PR, then get the Xcode workspace generation happening correctly in the next PR and then finally roll this out to the IDEA and Eclipse and Visual Studio plugins in a later PR (or multiple PRs).

lacasseio

Here are some preliminary comments on the code review. It could be useful to discuss some of them.

lacasseio · 2017-11-30T22:53:43Z

...tegTest/groovy/org/gradle/ide/xcode/XcodeCppExternalSourceDependenciesIntegrationTest.groovy

+        succeeds 'xcode'
+
+        then:
+        executedAndNotSkipped(":xcodeProject", ":xcodeProjectWorkspaceSettings", ":xcodeSchemeAppExecutable", ":xcode")


I may be wrong, but I don't think we have any coverage for buildable headers for Xcode. I wouldn't pull this feature in this PR as it would mean adding it to all of Xcode and will most likely send the PR into a rabbit hole. I would try to stay focus on a single feature and do another PR for this use case instead.

lacasseio · 2017-11-30T23:02:48Z

...tegTest/groovy/org/gradle/ide/xcode/XcodeCppExternalSourceDependenciesIntegrationTest.groovy

+
+        then:
+        executedAndNotSkipped(":xcodeProject", ":xcodeProjectWorkspaceSettings", ":xcodeSchemeAppExecutable", ":xcode",
+            ":greeter:xcodeProject", ":greeter:xcodeProjectWorkspaceSettings", ":greeter:xcodeSchemeGreeterSharedLibrary")


This is exactly the issue I was having and trying to figure out. The code that finds all xcodeproj will search any project/included build that defines xcodeproj artifact. If the source dependency applies the xcode plugin, the root build will find the xcodeproj files and will try to include them. I was asking how to find out is a particular included build was included because of source dependency so we can exclude them. Otherwise, they are going to be included and there isn't much we can do about it (with the current code).

If we only populate the import paths and search directories, all works fine in term of symbol discovery, but the Swift experience may not be the best as you won't be able to jump to the source code. Instead, you will jump to the Swift file representation of the .swiftmodule which only show the class/function prototype.

The middle ground is to include them as normal, but any change to the project source file will be overwritten on each build as Gradle will do a git reset --hard on the folder. This way you can jump to the implementation, but won't be able to change it.

lacasseio · 2017-11-30T23:04:23Z

...tegTest/groovy/org/gradle/ide/xcode/XcodeCppExternalSourceDependenciesIntegrationTest.groovy

+        succeeds 'xcode'
+
+        then:
+        executedAndNotSkipped(":xcodeProject", ":xcodeProjectWorkspaceSettings", ":xcodeSchemeAppExecutable", ":xcode")


With regards to building the greeter library, we don't really need to build it as Xcode delegate to Gradle on every build which will ensure that greeter library is built.

lacasseio · 2017-11-30T23:31:45Z

subprojects/ide-native/src/main/java/org/gradle/ide/xcode/plugins/XcodePlugin.java

+                project.getConfigurations().getByName("ide").getResolvedConfiguration();
+
                return CollectionUtils.collect(
-                    allXcodeprojArtifactsInComposite(serviceRegistry),


I'm not sure I completely understand. We aren't changing how we include project files in the workspace. The getIdeArtifactMetadata is the same implementation of what used to be allXcodeprojArtifactsInComposite. The reason why we are forcing the resolution is because workspace.dependsOn will not find the source dependency project as they weren't evaluated yet. This means they are not part of the ProjectPathRegistry. However when the task executes and resolve workspaceTask.setXcodeProjectLocations, the source dependency project have been evaluated and now we find the Xcode project path. This cause the workspace to include Xcode project that will not be built because we don't have any task dependency on them.

You are right that we could add a dependency from Xcode project generation task to binary compile configuration. This would, in fact, fix some of the problems, maybe all of them, that we have now.

lacasseio · 2017-11-30T23:32:53Z

...ojects/ide-native/src/main/java/org/gradle/ide/xcode/tasks/GenerateXcodeProjectFileTask.java

-            debugSettings.put("SWIFT_INCLUDE_PATHS", toSpaceSeparatedList(xcodeTarget.getCompileModules()));
-            releaseSettings.put("SWIFT_INCLUDE_PATHS", toSpaceSeparatedList(xcodeTarget.getCompileModules()));
-            testRunnerSettings.put("SWIFT_INCLUDE_PATHS", toSpaceSeparatedList(xcodeTarget.getCompileModules()));
+            debugSettings.put("SWIFT_INCLUDE_PATHS", toSpaceSeparatedList(parentDirs(xcodeTarget.getCompileModules())));


It's not that it was broken, it's just that it didn't behave like everything else.

lacasseio · 2017-11-30T23:35:22Z

subprojects/ide/src/main/java/org/gradle/plugins/ide/idea/model/IdeaProject.java

-            LocalComponentArtifactMetadata imlArtifact = localComponentRegistry.findAdditionalArtifact(otherProjectId, "iml");
-            if (imlArtifact != null) {
-                xmlProject.addModulePath(imlArtifact.getFile());
+        Iterator<IdePlugin> it = project.getPlugins().withType(IdePlugin.class).iterator();


I was trying to move the logic out of there, but I kept running into backward compatibility. If we diverge too much from what we have now, we may break a lot of people that may be using the model in some untested ways. We may be alright with such breakage. I will look at the code again to see if I can remove this.

lacasseio · 2017-11-30T23:35:41Z

...tive/src/main/java/org/gradle/nativeplatform/test/xctest/plugins/XCTestConventionPlugin.java

-                            return regularFile.getAsFile().getParentFile();
-                        }
-                    }))));
+                    .add(new DefaultSelfResolvingDependency((FileCollectionInternal)project.files(((DefaultSwiftBinary)testedComponent.getDevelopmentBinary()).getModuleFile())));


I will look into that.

lacasseio

It should be mostly fixed. I differ the final fix for IdeaPlugin in another PR. This PR only offers a partial fix (remove the code duplication but doesn't introduce SetProperty).

lacasseio · 2017-12-04T02:36:31Z

...tegTest/groovy/org/gradle/ide/xcode/XcodeCppExternalSourceDependenciesIntegrationTest.groovy

+        then:
+        executedAndNotSkipped(":xcodeProject", ":xcodeProjectWorkspaceSettings", ":xcodeSchemeAppExecutable", ":xcode",
+            ":greeter:xcodeProject", ":greeter:xcodeProjectWorkspaceSettings", ":greeter:xcodeSchemeGreeterSharedLibrary")
+        rootXcodeWorkspace.contentFile.assertHasProjects("${rootProjectName}.xcodeproj", "${checkoutRelativeDir(repo.name, commit.id.name, repo.id)}/greeter.xcodeproj")


lacasseio · 2017-12-04T02:36:37Z

...gTest/groovy/org/gradle/ide/xcode/XcodeSwiftExternalSourceDependenciesIntegrationTest.groovy

+        succeeds 'xcode'
+
+        then:
+        executedAndNotSkipped(":xcodeProject", ":xcodeProjectWorkspaceSettings", ":xcodeSchemeAppExecutable", ":xcode")


lacasseio · 2017-12-04T02:36:44Z

...gTest/groovy/org/gradle/ide/xcode/XcodeSwiftExternalSourceDependenciesIntegrationTest.groovy

+
+        then:
+        executedAndNotSkipped(":xcodeProject", ":xcodeProjectWorkspaceSettings", ":xcodeSchemeAppExecutable", ":xcode",
+            ":greeter:xcodeProject", ":greeter:xcodeProjectWorkspaceSettings", ":greeter:xcodeSchemeGreeterSharedLibrary")


lacasseio · 2017-12-04T02:37:01Z

...gTest/groovy/org/gradle/ide/xcode/XcodeSwiftExternalSourceDependenciesIntegrationTest.groovy

+        then:
+        executedAndNotSkipped(":xcodeProject", ":xcodeProjectWorkspaceSettings", ":xcodeSchemeAppExecutable", ":xcode",
+            ":greeter:xcodeProject", ":greeter:xcodeProjectWorkspaceSettings", ":greeter:xcodeSchemeGreeterSharedLibrary")
+        rootXcodeWorkspace.contentFile.assertHasProjects("${rootProjectName}.xcodeproj", "${checkoutRelativeDir(repo.name, commit.id.name, repo.id)}/greeter.xcodeproj")


lacasseio · 2017-12-04T02:37:11Z

...gTest/groovy/org/gradle/ide/xcode/XcodeSwiftExternalSourceDependenciesIntegrationTest.groovy

+        succeeds 'xcode'
+
+        then:
+        executedAndNotSkipped(":xcodeProject", ":xcodeProjectWorkspaceSettings", ":xcodeSchemeAppSharedLibrary", ":xcode")


lacasseio · 2017-12-04T02:37:25Z

...gTest/groovy/org/gradle/ide/xcode/XcodeSwiftExternalSourceDependenciesIntegrationTest.groovy

+
+        then:
+        executedAndNotSkipped(":xcodeProject", ":xcodeProjectWorkspaceSettings", ":xcodeSchemeAppSharedLibrary", ":xcode",
+            ":greeter:xcodeProject", ":greeter:xcodeProjectWorkspaceSettings", ":greeter:xcodeSchemeGreeterSharedLibrary")


lacasseio · 2017-12-04T02:38:51Z

subprojects/ide-native/src/main/java/org/gradle/ide/xcode/plugins/XcodePlugin.java

+                project.getConfigurations().getByName("ide").getResolvedConfiguration();
+
                return CollectionUtils.collect(
-                    allXcodeprojArtifactsInComposite(serviceRegistry),


The ide configuration is removed.

lacasseio · 2017-12-04T03:06:01Z

subprojects/ide-native/src/main/java/org/gradle/ide/xcode/plugins/XcodePlugin.java

+                project.getConfigurations().getByName("ide").getResolvedConfiguration();
+
                return CollectionUtils.collect(
-                    allXcodeprojArtifactsInComposite(serviceRegistry),


I think I understand your idea now. Have a look at the latest changes.

lacasseio · 2017-12-04T03:07:54Z

...tive/src/main/java/org/gradle/nativeplatform/test/xctest/plugins/XCTestConventionPlugin.java

-                            return regularFile.getAsFile().getParentFile();
-                        }
-                    }))));
+                    .add(new DefaultSelfResolvingDependency((FileCollectionInternal)project.files(((DefaultSwiftBinary)testedComponent.getDevelopmentBinary()).getModuleFile())));


lacasseio · 2017-12-04T03:08:26Z

...de/src/integTest/groovy/org/gradle/plugins/ide/idea/IdeaCompositeBuildIntegrationTest.groovy

+
+        def ipr = getFile([:], 'root.ipr').text
+        assert ipr.contains('''  <component name="ProjectModuleManager">
+    <modules>


adammurdoch · 2017-12-05T06:26:21Z

subprojects/core/src/main/java/org/gradle/initialization/ProjectPathRegistry.java


+    /**
+     * Returns an path for every implicit project in a build, including projects from included builds.
+     * Always returns the same set for the lifetime of the build.


I don't think this is true, and even if it is, it shouldn't be part of the contract. Implicit builds can be added at pretty much any time, and consumers of this interface should deal with this.

Done. Does this mean we should also remove this mention from the getAllProjectPaths?

adammurdoch · 2017-12-05T06:26:52Z

subprojects/core/src/main/java/org/gradle/initialization/ProjectPathRegistry.java

+
+    /**
+     * Returns an path for every explicit project in a build, including projects from included builds.
+     * Always returns the same set for the lifetime of the build.


I don't think it should.

adammurdoch · 2017-12-05T06:29:47Z

...tegTest/groovy/org/gradle/ide/xcode/XcodeCppExternalSourceDependenciesIntegrationTest.groovy

+        appProject.indexTarget.getBuildSettings().HEADER_SEARCH_PATHS == toSpaceSeparatedList(file('src/main/headers'), checkoutDir(repo.name, commit.id.name, repo.id).file('src/main/public'))
+    }
+
+    def "adds source dependencies Swift module of main component to Xcode indexer search path when no component in root project"() {


"Swift" should be "C++"?

Good catch, done.

adammurdoch · 2017-12-05T06:32:19Z

...gTest/groovy/org/gradle/ide/xcode/XcodeSwiftExternalSourceDependenciesIntegrationTest.groovy

+        succeeds 'xcode'
+
+        then:
+        executedAndNotSkipped(":xcodeProject", ":xcodeProjectWorkspaceSettings", ":xcodeSchemeAppExecutable", ":xcode")


you should update the test to show that the binaries of the greeter library are built. I'm not seeing any changes here.

adammurdoch · 2017-12-05T06:32:54Z

...gTest/groovy/org/gradle/ide/xcode/XcodeSwiftExternalSourceDependenciesIntegrationTest.groovy

+
+        then:
+        executedAndNotSkipped(":xcodeProject", ":xcodeProjectWorkspaceSettings", ":xcodeSchemeAppExecutable", ":xcode")
+        rootXcodeWorkspace.contentFile.assertHasProjects("${rootProjectName}.xcodeproj")//, "${checkoutRelativeDir(repo.name, commit.id.name, repo.id)}/greeter.xcodeproj")


should get rid of the dead code in the comment

adammurdoch · 2017-12-05T06:35:37Z

...gTest/groovy/org/gradle/ide/xcode/XcodeSwiftExternalSourceDependenciesIntegrationTest.groovy

+        succeeds 'xcode'
+
+        then:
+        executedAndNotSkipped(":app:xcodeProject", ":app:xcodeProjectWorkspaceSettings", ":app:xcodeSchemeAppExecutable",


should show that the module file of the greeter library is built.

adammurdoch · 2017-12-05T06:36:59Z

...ects/ide-native/src/main/java/org/gradle/ide/xcode/tasks/GenerateXcodeWorkspaceFileTask.java

    }

-    @Internal
+    @Optional


why optional?

Good point, removed.

adammurdoch · 2017-12-05T06:37:28Z

...de/src/integTest/groovy/org/gradle/plugins/ide/idea/IdeaCompositeBuildIntegrationTest.groovy

+"""
+
+        //when
+        executer.usingBuildScript(buildFile).usingSettingsFile(settingsFile).withTasks("idea").run()


can you make this a spock test?

adammurdoch · 2017-12-05T06:39:43Z

subprojects/ide/src/main/java/org/gradle/plugins/ide/internal/IdePlugin.java

+            @Override
+            public List<FileCollection> call() throws Exception {
+                return CollectionUtils.collect(
+                    getIdeArtifactMetadata("xcodeproj"),


I think this parameter should be type

Good catch, changed.

adammurdoch · 2017-12-05T06:40:43Z

subprojects/ide/src/main/java/org/gradle/plugins/ide/internal/IdePlugin.java

+
+        for (Path projectPath : projectPathRegistry.getAllExplicitProjectPaths()) {
+            ProjectComponentIdentifier projectId = projectPathRegistry.getProjectComponentIdentifier(projectPath);
+            LocalComponentArtifactMetadata xcodeprojArtifact = localComponentRegistry.findAdditionalArtifact(projectId, type);


variable should probably not have xcodeproj in its name

Good catch, changed.

adammurdoch · 2017-12-05T06:47:07Z

You're still trying to change how the workspace is generated. You should instead change how the project file is generated. Otherwise the result is wrong and is working by coincidence only.

Specifically:

Filter the cpp compile configuration to remove the project components that will be included as xcode projects, attach this as an input to the project generation task, to generate the header path in the xcode project file.
Filter the swift compile configuration in the same way, attach this as input to the project generation task, to generate the module search path in the xcode project file.

lacasseio · 2017-12-06T07:00:43Z

I think I understood what you wanted Adam. Basically, we want to ensure that Swift module and headers are present for source dependencies as they should build be able to compile without issue as opposed to the Swift module and headers from the current build where they could have failures in it which would be annoying when generating the Xcode project.

I had to add some additional modification to make the filtering work such as exposing the include path configuration for C++ and exposing the Swift module of the application so it can be consumed by XCTest as well as filtered out when adding the task dependencies.

CI is running and you can have another look at the code.

Signed-off-by: Daniel Lacasse <[email protected]>

adammurdoch · 2017-12-07T07:24:05Z

...gTest/groovy/org/gradle/ide/xcode/XcodeSwiftExternalSourceDependenciesIntegrationTest.groovy

+
+        def appProject = xcodeProject("${rootProjectName}.xcodeproj").projectFile
+        appProject.targets.find { it.isUnitTest() }.getBuildSettings().SWIFT_INCLUDE_PATHS == toSpaceSeparatedList(file('build/modules/main/debug'), checkoutDir(repo.name, commit.id.name, repo.id).file('build/modules/main/debug'))
+        appProject.indexTarget.getBuildSettings().SWIFT_INCLUDE_PATHS == toSpaceSeparatedList(file('build/modules/main/debug'), checkoutDir(repo.name, commit.id.name, repo.id).file('build/modules/main/debug'))


Why do we need to add the main module file here when we don't in the previous cases? I would guess that Xcode doesn't need that connection between the tests and production sources for a project to be made explicit like this. If we do add it to the include path, we should probably build it during generation, so that the result doesn't change when Gradle builds the modules later on.

This isn't related to this PR, but it's something we should tidy up as a separate issue.

You are so right. My bad for not questioning this enough when developing this code. I will open an issue for this.

Issue created: gradle/gradle-native#331.

adammurdoch · 2017-12-07T07:30:36Z

subprojects/ide-native/src/main/java/org/gradle/ide/xcode/internal/XcodeTarget.java

    public ConfigurableFileCollection getCompileModules() {
        return compileModules;
    }
+


The headerSearchPath, compileModules and sources file collections should all just carry the task dependency information. These task dependencies shouldn't be tracked separately, otherwise they get out of sync, for example when someone adds something to the header file set but we don't update the task dependencies. Or when someone queries the files but forgets to query the task dependencies.

Maybe just merge what we have here and we can simplify it later.

I agree with you on this. When I discuss this with Sterling, we figure that changing the model to correctly add @Input annotation or changing the task to take individual properties would have been too risky of going down a rabbit hole. I will open an issue for this.

Issue created: gradle/gradle-native#332.

adammurdoch · 2017-12-07T07:35:15Z

.../language-native/src/main/java/org/gradle/language/swift/plugins/SwiftApplicationPlugin.java

+
+        Configuration implementation = application.getImplementationDependencies();
+
+        Configuration debugApiElements = configurations.maybeCreate("debugSwiftApiElements");


An application doesn't provide a Swift API, otherwise it's a library. We shouldn't be adding these configurations.

Maybe just merge this for now and we can remove these later.

I thought the same thing when I was writing this and would have wanted to discuss it with you. The issue I was having is when we add a dependency from the test component on the main component, the artifactView wasn't filtering out the main component because the way we used to add it didn't add a ComponentIdentifier. This ended up compiling the code from the main component in if XCTest plugin was applied. My thinking is this allow someone to add a test on the main component in a different project. I'm not sure if this is something we want, but it's something that is valid. I will open an issue for this.

Issue created: gradle/gradle-native#333.

lacasseio

Thanks for the awesome feedback Adam. I will merge it now and open/link issues tomorrow.

lacasseio · 2017-12-07T08:42:47Z

subprojects/ide-native/src/main/java/org/gradle/ide/xcode/internal/XcodeTarget.java

    public ConfigurableFileCollection getCompileModules() {
        return compileModules;
    }
+


I agree with you on this. When I discuss this with Sterling, we figure that changing the model to correctly add @Input annotation or changing the task to take individual properties would have been too risky of going down a rabbit hole. I will open an issue for this.

lacasseio · 2017-12-07T08:43:24Z

...gTest/groovy/org/gradle/ide/xcode/XcodeSwiftExternalSourceDependenciesIntegrationTest.groovy

+
+        def appProject = xcodeProject("${rootProjectName}.xcodeproj").projectFile
+        appProject.targets.find { it.isUnitTest() }.getBuildSettings().SWIFT_INCLUDE_PATHS == toSpaceSeparatedList(file('build/modules/main/debug'), checkoutDir(repo.name, commit.id.name, repo.id).file('build/modules/main/debug'))
+        appProject.indexTarget.getBuildSettings().SWIFT_INCLUDE_PATHS == toSpaceSeparatedList(file('build/modules/main/debug'), checkoutDir(repo.name, commit.id.name, repo.id).file('build/modules/main/debug'))


You are so right. My bad for not questioning this enough when developing this code. I will open an issue for this.

lacasseio · 2017-12-07T08:47:17Z

.../language-native/src/main/java/org/gradle/language/swift/plugins/SwiftApplicationPlugin.java

+
+        Configuration implementation = application.getImplementationDependencies();
+
+        Configuration debugApiElements = configurations.maybeCreate("debugSwiftApiElements");


I thought the same thing when I was writing this and would have wanted to discuss it with you. The issue I was having is when we add a dependency from the test component on the main component, the artifactView wasn't filtering out the main component because the way we used to add it didn't add a ComponentIdentifier. This ended up compiling the code from the main component in if XCTest plugin was applied. My thinking is this allow someone to add a test on the main component in a different project. I'm not sure if this is something we want, but it's something that is valid. I will open an issue for this.

lacasseio added a:feature A new functionality from:member in:native-platform c, cpp, swift and other native languages support, etc labels Nov 30, 2017

lacasseio added this to the 4.5 RC1 milestone Nov 30, 2017

lacasseio self-assigned this Nov 30, 2017

lacasseio force-pushed the lacasseio/native/improve-xcode-ide branch 3 times, most recently from 4473fb0 to 1da0b37 Compare November 30, 2017 06:40

big-guy self-requested a review November 30, 2017 20:34

adammurdoch suggested changes Nov 30, 2017

View reviewed changes

lacasseio commented Nov 30, 2017

View reviewed changes

lacasseio force-pushed the lacasseio/native/improve-xcode-ide branch from 1da0b37 to 429c773 Compare December 4, 2017 06:51

lacasseio commented Dec 4, 2017

View reviewed changes

adammurdoch suggested changes Dec 5, 2017

View reviewed changes

lacasseio force-pushed the lacasseio/native/improve-xcode-ide branch from 86e5eb8 to d75fe42 Compare December 6, 2017 06:54

Daniel Lacasse added 13 commits December 7, 2017 13:29

Pull methods up to IdePlugin from XcodePlugin

3f0a2b0

Signed-off-by: Daniel Lacasse <[email protected]>

Use public API to create DefaultSelfResolvingDependency instance

a63bc45

Signed-off-by: Daniel Lacasse <[email protected]>

Fix a oversight in Swift compile configuration handling

8b64605

Signed-off-by: Daniel Lacasse <[email protected]>

Add unit test for source dependency in Xcode support

a564ab8

Signed-off-by: Daniel Lacasse <[email protected]>

Correctly handle source dependencies in Xcode

ef24a6d

Signed-off-by: Daniel Lacasse <[email protected]>

Revert change to IdeaPlugin

63376b6

Signed-off-by: Daniel Lacasse <[email protected]>

Revert assertion when adding project path in registry

3005252

Signed-off-by: Daniel Lacasse <[email protected]>

Revert changes for generated source

ea2c253

Signed-off-by: Daniel Lacasse <[email protected]>

Remove unecessary final

24908eb

Signed-off-by: Daniel Lacasse <[email protected]>

Remove misleading javadoc

7fde0a5

Signed-off-by: Daniel Lacasse <[email protected]>

Fix typo in test name

6e4d768

Signed-off-by: Daniel Lacasse <[email protected]>

Remove unecessary @Optional annotation

bd6fb34

Signed-off-by: Daniel Lacasse <[email protected]>

Fix typo in variable name

a46816f

Signed-off-by: Daniel Lacasse <[email protected]>

Daniel Lacasse added 3 commits December 7, 2017 13:29

Use argument paramenter instead of hardcoded string

f582341

Signed-off-by: Daniel Lacasse <[email protected]>

Convert JUnit test to spock test

aa55d2c

Signed-off-by: Daniel Lacasse <[email protected]>

Ensure source dependencies are built before Xcode project are generated

c51847c

Signed-off-by: Daniel Lacasse <[email protected]>

lacasseio force-pushed the lacasseio/native/improve-xcode-ide branch from d75fe42 to c51847c Compare December 7, 2017 03:29

adammurdoch approved these changes Dec 7, 2017

View reviewed changes

lacasseio commented Dec 7, 2017

View reviewed changes

lacasseio merged commit 94b2b96 into master Dec 7, 2017

lacasseio deleted the lacasseio/native/improve-xcode-ide branch December 7, 2017 08:48


		import static org.gradle.ide.xcode.internal.XcodeUtils.toSpaceSeparatedList

		class XcodeCppExternalSourceDependenciesIntegrationTest extends AbstractXcodeIntegrationSpec {


		Configuration implementation = application.getImplementationDependencies();

		Configuration debugApiElements = configurations.maybeCreate("debugSwiftApiElements");

Polish Xcode support for source dependencies #3660

Polish Xcode support for source dependencies #3660

Uh oh!

Conversation

lacasseio commented Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Gradle Core Team Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lacasseio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lacasseio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

lacasseio commented Nov 30, 2017 •

edited

Loading