Fix for switch debug & release builds invalidates build #1731
Conversation
Sources/Build/llbuild.swift
Outdated
| var commandName: String { | ||
| return "C.\(llbuildTargetName)" | ||
|
|
||
| public func getBuildTargetName(config: String?) -> String { |
There was a problem hiding this comment.
This shouldn't be optional. We should always pass the config.
There was a problem hiding this comment.
nit: function name -> getLLBuildTargetName(config:)
Sources/Build/llbuild.swift
Outdated
| return "\(name)-release.module" | ||
| } | ||
| else if config == "debug" { | ||
| return "\(name)-debug.module" |
There was a problem hiding this comment.
Once the config is non-optional, this function can just be: return "\(name)-\(config).module"
prachipai
force-pushed
the
switchFix
branch
2 times, most recently
from
43650c8
to
c0424d8
Compare
Sources/Build/llbuild.swift
Outdated
| if config == "release" { | ||
| return "\(name)-release.module" | ||
| } | ||
| return "\(name)-debug.module" |
There was a problem hiding this comment.
This can just be return "\(name)-\(config).module"
Sources/Build/llbuild.swift
Outdated
| @@ -267,11 +266,11 @@ public struct LLBuildManifestGenerator { | |||
| } | |||
| } | |||
|
|
|||
| var buildTarget = Target(name: target.target.llbuildTargetName) | |||
| var buildTarget = Target(name: target.target.getLLBuildTargetName(config: plan.buildParameters.configuration.dirname)) | |||
There was a problem hiding this comment.
Since we need it twice in this method, its better to extract it into a new variable:
let buildConfig = plan.buildParameters.configuration.dirname
..
var buildTarget = Target(name: target.target.getLLBuildTargetName(config: buildConfig))
Sources/Build/llbuild.swift
Outdated
| @@ -249,7 +249,6 @@ public struct LLBuildManifestGenerator { | |||
| switch dependency { | |||
| case .target(let target): | |||
| addStaticTargetInputs(target) | |||
|
|
|||
Sources/Commands/SwiftTool.swift
Outdated
| @@ -565,28 +565,57 @@ public class SwiftTool<Options: ToolOptions> { | |||
| return try _manifestLoader.dematerialize() | |||
| } | |||
|
|
|||
| func computeLLBuildTargetName(for subset: BuildSubset) throws -> String? { | |||
| func shouldRegenerateManifest() throws -> Bool { | |||
Sources/Commands/SwiftTool.swift
Outdated
| return | ||
| } | ||
| try runLLBuild(manifest: parameters.llbuildManifest, llbuildTarget: llbuildTargetName) | ||
| } | ||
|
|
||
| /// Build a subset of products and targets using swift-build-tool. | ||
| func build(plan: BuildPlan, subset: BuildSubset) throws { | ||
| guard let llbuildTargetName = subset.llbuildTargetName(for: plan.graph, diagnostics: diagnostics) else { | ||
| guard !plan.graph.rootPackages[0].targets.isEmpty else { |
There was a problem hiding this comment.
Not sure why is this diff here, bad merge/rebase?
Getting rid of optional parameter
prachipai
force-pushed
the
switchFix
branch
10 times, most recently
from
3945506
to
8324f0c
Compare
Sources/Build/llbuild.swift
Outdated
| @@ -266,12 +266,12 @@ public struct LLBuildManifestGenerator { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
Sources/Commands/SwiftTool.swift
Outdated
| @@ -564,29 +564,29 @@ public class SwiftTool<Options: ToolOptions> { | |||
| func getManifestLoader() throws -> ManifestLoader { | |||
| return try _manifestLoader.dematerialize() | |||
| } | |||
|
|
|||
| func computeLLBuildTargetName(for subset: BuildSubset) throws -> String? { | |||
|
|
|||
|
We need to add this config in products as well. You can notice linker re-running in executable packages: swift package init --type executable |
|
Added for products too! |
|
Is this ready for rereview? |
|
We're thinking to write some tests to make sure other things aren't getting broken. Ankit was worried about this. |
|
This looks good. |
|
@swift-ci smoke test |
|
Yay yay yay! Can’t wait for a toolchain... |
No description provided.