Skip to content

[Explicit Module Builds] Add support for versioned PCMs #126

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 18, 2020
Merged

[Explicit Module Builds] Add support for versioned PCMs #126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions Sources/SwiftDriver/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@
# See http://swift.org/CONTRIBUTORS.txt for Swift project authors

add_library(SwiftDriver
"Dependency Scanning/ModuleDependencyBuildGeneration.swift"
"Dependency Scanning/InterModuleDependencyGraph.swift"
"Explicit Module Builds/ClandModuleBuildJobCache.swift"
"Explicit Module Builds/ExplicitModuleBuildHandler.swift"
"Explicit Module Builds/InterModuleDependencyGraph.swift"
"Explicit Module Builds/ModuleDependencyScanning.swift"

Driver/CompilerMode.swift
Driver/DebugInfo.swift
Expand All @@ -17,7 +19,6 @@ add_library(SwiftDriver
Driver/ModuleOutputInfo.swift
Driver/OutputFileMap.swift
Driver/ToolExecutionDelegate.swift
Driver/ModuleDependencyScanning.swift

Execution/JobExecutor.swift
Execution/ParsableOutput.swift
Expand Down
160 changes: 0 additions & 160 deletions Sources/SwiftDriver/Dependency Scanning/ModuleDependencyBuildGeneration.swift
View file
Open in desktop

This file was deleted.

11 changes: 7 additions & 4 deletions Sources/SwiftDriver/Driver/Driver.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ public struct Driver {
case conflictingOptions(Option, Option)
// Explicit Module Build Failures
case malformedModuleDependency(String, String)
case missingPCMArguments(String)
case missingModuleDependency(String)
case dependencyScanningFailure(Int, String)

Expand All @@ -49,6 +50,8 @@ public struct Driver {
// Explicit Module Build Failures
case .malformedModuleDependency(let moduleName, let errorDescription):
return "Malformed Module Dependency: \(moduleName), \(errorDescription)"
case .missingPCMArguments(let moduleName):
return "Missing extraPcmArgs to build Clang module: \(moduleName)"
case .missingModuleDependency(let moduleName):
return "Missing Module Dependency Info: \(moduleName)"
case .dependencyScanningFailure(let code, let error):
Expand Down Expand Up @@ -169,10 +172,10 @@ public struct Driver {
/// This will force the driver to first emit the module and then run compile jobs.
public var forceEmitModuleInSingleInvocation: Bool = false

/// The module dependency graph, which is populated during the planning phase
/// only when all modules will be prebuilt and treated as explicit by the
/// various compilation jobs.
public var interModuleDependencyGraph: InterModuleDependencyGraph? = nil
/// Handler for constructing module build jobs using Explicit Module Builds.
/// Constructed during the planning phase only when all modules will be prebuilt and treated
/// as explicit by the various compilation jobs.
public var explicitModuleBuildHandler: ExplicitModuleBuildHandler? = nil

/// Handler for emitting diagnostics to stderr.
public static let stderrDiagnosticsHandler: DiagnosticsEngine.DiagnosticsHandler = { diagnostic in
Expand Down
38 changes: 38 additions & 0 deletions Sources/SwiftDriver/Explicit Module Builds/ClangModuleBuildJobCache.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
//===----------------- ClangModuleBuildJobCache.swift ---------------------===//
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2020 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See https://swift.org/LICENSE.txt for license information
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
//
//===----------------------------------------------------------------------===//

/// Maps tuples (ModuleDependencyId, [String]), where [String] is an array of pcm build opitions,
/// to the job used to build this module with these specified build options.
struct ClangModuleBuildJobCache {

/// As tuples cannot be Hashable, wrap (ModuleDependencyId, [String]) into a struct pair
private struct ModuleArgumentPair : Hashable {
Copy link
Member

Choose a reason for hiding this comment

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

This makes me wonder whether there is a more fundamental problem here with the ModuleDependencyId abstraction. Right now, it assumes that the Clang modules are uniquely identified by their name within a Swift compiler job, but that's not true. Rather, they are uniquely identified by their name and by the extra build args needed to produce that variant of the model. How about pulling that into ModuleDependencyId directly:

enum ModuleDependencyId: Hashable {
  case swift(String)
  case clang(moduleName: String, buildArguments: [String])
}

then ClangModuleBuildJobCache would not have to exist and, more importantly, it wouldn't be possible to make the mistake of using the "wrong" Clang module or getting them confused, because ModuleDependencyId provides the right notion of identity. (This probably should be reflected in the JSON output from the dependency scanner, too).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then the logic of "discovering" which clang module versions must be built would need to be moved to the dependency scanner. The dependency scanner would then need to either mutate the existing graph to have versioned PCMs or generate a versioned-PCM graph from the start, in a way that is similar to how "discovery" is done here.

Copy link
Member

Choose a reason for hiding this comment

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

It seems to me like the dependency scanner should be generating the versioned-PCM graph, because that's an accurate representation of the dependencies. Right now, the graph that comes out of the dependency scanner does not accurately represent the modules that need to be built.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have experimented with constructing a graph from the compiler to include all versioned PCMs. The reason for me to not choose that alternative is that all transitive PCM dependencies have to be pulled into directive ones so that they can have unique command line arguments, which is not an accurate representation either.

Copy link
Member

Choose a reason for hiding this comment

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

The uniqued command line arguments are what make them unique; that's why I'm advocating for it to be part of module identity. Each module node in the dependency graph should correspond to exactly one module file that needs to be built. Anything else leaves too much interpretation to the client, and we'd rather keep the clients simple.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I understand the "transitive dependency" issue now. This is fine.

let moduleId: ModuleDependencyId
let buildArgs: [String]
}

private var cache: [ModuleArgumentPair: Job] = [:]

var allJobs: [Job] {
return Array(cache.values)
}

subscript(index: (ModuleDependencyId, [String])) -> Job? {
get {
return cache[ModuleArgumentPair(moduleId: index.0, buildArgs: index.1)]
}
set(newValue) {
cache[ModuleArgumentPair(moduleId: index.0, buildArgs: index.1)] = newValue
}
}
}

Loading