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

Avoid clean on running xcodebuild if New Build System is used. #555

Merged
merged 6 commits into from Sep 8, 2018

Conversation

norio-nomura
Copy link
Collaborator

Since Xcode’s New Build System uses llbuild, compiler arguments for Module can be retrieved from manifest of llbuild instead of parsing xcodebuild’s output. Until this changes, the clean action was used for xcodebuild to ensure compiler arguments, but it is not necessary when using the New Build System.

@jpsim
Copy link
Owner

jpsim commented Sep 3, 2018

This is promising, thank you! Although there's one slight problem: -showBuildSettings doesn't generate the manifest file, so if derived data is empty, it will still run a full build.

However, @keith recently discovered that even though xcodebuild -dry-run isn't technically supported with the llbuild-based build system, and running the command returns a non-zero exit code, the manifest file is still correctly generated.

So we could solve this by running -dry-run and ignore the result before continuing with checkNewBuildSystem.

But we still need a full build afterwards, because otherwise we get some types that resolve as <<error type>>, so we'd need to revert 4f130be.

 internal func checkNewBuildSystem(xcodeBuildArguments: [String],
                                   moduleName: String? = nil,
                                   inPath path: String) -> [String]? {
-    // Check `PROJECT_TEMP_ROOT`
+    // `-dry-run` generates the manifest file when using the new build system.
+    _ = XcodeBuild.run(arguments: xcodeBuildArguments + ["-dry-run"], inPath: path)
+
+    // Extract `PROJECT_TEMP_ROOT`
     fputs("Checking xcodebuild -showBuildSettings\n", stderr)
     guard let output = XcodeBuild.run(arguments: xcodeBuildArguments + ["-showBuildSettings"], inPath: path),
         let match = regexForProjectTempRoot.firstMatch(in: output, range: NSRange(output.startIndex..<output.endIndex, in: output)),
@@ -254,7 +257,8 @@ internal func checkNewBuildSystem(xcodeBuildArguments: [String],
         }.first.map { filter(arguments: $0) }
 
         if result != nil {
-            fputs("Assuming New Build System is used.\n", stderr)
+            fputs("Assuming New Build System is used.\nRunning xcodebuild\n", stderr)
+            _ = XcodeBuild.run(arguments: xcodeBuildArguments, inPath: path)
         }

@norio-nomura
Copy link
Collaborator Author

If we still need to build after -dry-run, I think that it is better to examine the manifest after building it normally. 🤔

@norio-nomura
Copy link
Collaborator Author

If we still need to build after -dry-run, I think that it is better to examine the manifest after building it normally. 🤔

As below without reverting 4f130be:

diff --git a/Source/SourceKittenFramework/Xcode.swift b/Source/SourceKittenFramework/Xcode.swift
index 78b22ac..b74090d 100644
--- a/Source/SourceKittenFramework/Xcode.swift
+++ b/Source/SourceKittenFramework/Xcode.swift
@@ -215,7 +215,11 @@ ${PROJECT_TEMP_ROOT}
 internal func checkNewBuildSystem(xcodeBuildArguments: [String],
                                   moduleName: String? = nil,
                                   inPath path: String) -> [String]? {
-    // Check `PROJECT_TEMP_ROOT`
+    // generates the manifest file when using the new build system.
+    fputs("Running xcodebuild\n", stderr)
+    _ = XcodeBuild.run(arguments: xcodeBuildArguments, inPath: path)
+
+    // Extract `PROJECT_TEMP_ROOT`
     fputs("Checking xcodebuild -showBuildSettings\n", stderr)
     guard let output = XcodeBuild.run(arguments: xcodeBuildArguments + ["-showBuildSettings"], inPath: path),
         let match = regexForProjectTempRoot.firstMatch(in: output, range: NSRange(output.startIndex..<output.endIndex, in: output)),

@jpsim
Copy link
Owner

jpsim commented Sep 5, 2018

Yes, you're totally right. 👍

@jpsim
Copy link
Owner

jpsim commented Sep 5, 2018

Can you get PROJECT_TEMP_ROOT from the xcodebuild log?

@norio-nomura
Copy link
Collaborator Author

The minimum xcodebuild log is as follows:

# Xcode 10
$ xcodebuild -workspace Carthage/Checkouts/Commandant/Commandant.xcworkspace/ -scheme Commandant build -verbose
note: Using new build system
note: Planning build
note: Constructing build description

** BUILD SUCCEEDED ** [0.731 sec]

# Xcode 9
$ DEVELOPER_DIR=/Applications/Xcode.app xcodebuild -workspace Carthage/Checkouts/Commandant/Commandant.xcworkspace/ -scheme Commandant build -verbose
=== BUILD TARGET Result-Mac OF PROJECT Result WITH CONFIGURATION Debug ===

Check dependencies

=== BUILD TARGET Commandant OF PROJECT Commandant WITH CONFIGURATION Debug ===

Check dependencies

** BUILD SUCCEEDED **

So using -showBuildSettings is required to getting PROJECT_TEMP_ROOT.

@norio-nomura norio-nomura changed the title Avoid clean on running xcodebuild if New Build System is used. [WIP] Avoid clean on running xcodebuild if New Build System is used. Sep 7, 2018
Since Xcode’s New Build System uses `llbuild`, compiler arguments for `Module` can be retrieved from manifest of `llbuild` instead of parsing `xcodebuild`’s output. Until this changes, the `clean` action was used for `xcodebuild` to ensure compiler arguments, but it is not necessary when using the New Build System.
… on Xcode 10+

We do not need to avoid using same `derivedDataPath`, since `clean` action is not used in `Module.init(xcodeBuildArguments:name:inPath:)` with assuming that New Build System is used on Xcode 10.
@norio-nomura norio-nomura changed the title [WIP] Avoid clean on running xcodebuild if New Build System is used. Avoid clean on running xcodebuild if New Build System is used. Sep 7, 2018
@norio-nomura
Copy link
Collaborator Author

Rewrote and rebased.

Copy link
Owner

@jpsim jpsim left a comment

Choose a reason for hiding this comment

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

Hmm, I think projects that don’t use the new build system will now be built twice 😬. It would still be worthwhile if most projects use the new build system.

What do you think?

@norio-nomura
Copy link
Collaborator Author

projects that don’t use the new build system will now be built twice

That will be the worst case. If the target module is not updated by the first build, second build with clean action will be required. In such case, I expect that the first build will be done in a short time.

@jpsim
Copy link
Owner

jpsim commented Sep 8, 2018

In such case, I expect that the first build will be done in a short time.

For CI jobs that run jazzy for example (which relies on this functionality), the first build will build from scratch, then the second clean build will also build from scratch, doubling the time spent building the project.

@norio-nomura
Copy link
Collaborator Author

In that case, it should return after the first build.

@norio-nomura
Copy link
Collaborator Author

In that case, it should return after the first build.

Sorry, I should mention that I also added parseCompilerArguments() to first build.

@jpsim
Copy link
Owner

jpsim commented Sep 8, 2018

Ah I missed that! Let me take another look.

Copy link
Owner

@jpsim jpsim left a comment

Choose a reason for hiding this comment

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

🎉

@jpsim jpsim merged commit 5106023 into master Sep 8, 2018
@jpsim jpsim deleted the nn-new-build-system branch September 8, 2018 01:59
@norio-nomura
Copy link
Collaborator Author

Thanks! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants