Skip to content

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

Merged
jpsim merged 6 commits into
masterfrom
nn-new-build-system
Sep 8, 2018
Merged

Avoid clean on running xcodebuild if New Build System is used.#555
jpsim merged 6 commits into
masterfrom
nn-new-build-system

Conversation

@norio-nomura

Copy link
Copy Markdown
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

jpsim commented Sep 3, 2018

Copy link
Copy Markdown
Owner

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
Copy Markdown
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
Copy Markdown
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

jpsim commented Sep 5, 2018

Copy link
Copy Markdown
Owner

Yes, you're totally right. 👍

@jpsim

jpsim commented Sep 5, 2018

Copy link
Copy Markdown
Owner

Can you get PROJECT_TEMP_ROOT from the xcodebuild log?

@norio-nomura

Copy link
Copy Markdown
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
Copy Markdown
Collaborator Author

Rewrote and rebased.

@jpsim jpsim left a comment

Copy link
Copy Markdown
Owner

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
Copy Markdown
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

jpsim commented Sep 8, 2018

Copy link
Copy Markdown
Owner

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
Copy Markdown
Collaborator Author

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

@norio-nomura

Copy link
Copy Markdown
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

jpsim commented Sep 8, 2018

Copy link
Copy Markdown
Owner

Ah I missed that! Let me take another look.

@jpsim jpsim left a comment

Copy link
Copy Markdown
Owner

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
Copy Markdown
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.

2 participants