Skip to content

Remove slightly dangerous optimization #4317

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
Nov 2, 2022
Merged

Remove slightly dangerous optimization #4317

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
1 change: 1 addition & 0 deletions Sources/Basics/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ add_library(Basics
SwiftVersion.swift
SQLiteBackedCache.swift
Version+Extensions.swift
VFSOverlay.swift
WritableByteStream+Extensions.swift)
target_link_libraries(Basics PUBLIC
SwiftCollections::OrderedCollections
Expand Down
53 changes: 53 additions & 0 deletions Sources/Basics/VFSOverlay.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
//===----------------------------------------------------------------------===//
//
// This source file is part of the Swift open source project
//
// Copyright (c) 2022 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See http://swift.org/LICENSE.txt for license information
// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
//
//===----------------------------------------------------------------------===//

import class Foundation.JSONEncoder
import struct TSCBasic.AbsolutePath
import protocol TSCBasic.FileSystem

public struct VFSOverlay: Encodable {
public struct File: Encodable {
enum CodingKeys: String, CodingKey {
case externalContents = "external-contents"
case name
case type
}

private let externalContents: String
private let name: String
private let type = "file"

public init(name: String, externalContents: String) {
self.name = name
self.externalContents = externalContents
}
}

enum CodingKeys: String, CodingKey {
case roots
case useExternalNames = "use-external-names"
case version
}

private let roots: [File]
private let useExternalNames = false
private let version = 0

public init(roots: [File]) {
self.roots = roots
}

public func write(to path: AbsolutePath, fileSystem: FileSystem) throws {
// VFS overlay files are YAML, but ours is simple enough that it works when being written using `JSONEncoder`.
try JSONEncoder.makeWithDefaults(prettified: false).encode(path: path, fileSystem: fileSystem, self)
}
}
40 changes: 21 additions & 19 deletions Sources/PackageLoading/ManifestLoader.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,29 +442,26 @@ public final class ManifestLoader: ManifestLoaderProtocol {
completion: @escaping (Result<EvaluationResult, Error>) -> Void
) throws {
do {
if localFileSystem.isFile(manifestPath) {
try withTemporaryDirectory { tempDir, cleanupTempDir in
let manifestTempFilePath = tempDir.appending(component: "manifest.swift")
try localFileSystem.writeFileContents(manifestTempFilePath, bytes: ByteString(manifestContents))
Copy link
Contributor

Choose a reason for hiding this comment

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

The change looks good in that it gets rid of the special-casing. Just so I understand, it does change so that now we always write the temporary file, whether or not we already have an original source file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right, for two reasons:

  • this could lead to reading from the wrong file, because we were always reading from the local FS, no matter which one we were actually using to get the contents of the manifest (this originally sparked this PR)
  • I'd like us to gain the ability to add to the manifest (e.g. by adding imports), this will allow things like making our non-implementation-only import of Foundation conditional on older tools-versions

Copy link
Contributor

Choose a reason for hiding this comment

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

Absolutely, I was thinking along exactly the same lines for bullet two below. Also for automatically getting struct definitions from things like plugins. As long as the VFS option is solid in terms of the diagnostics (which the tests should show), then I think this is great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yah, diagnostics look solid and also other context, like the delegate methods etc. We're now always passing down the original path to the loading machinery and the only thing that knows about the "real" path is the VFS.


let vfsOverlayTempFilePath = tempDir.appending(component: "vfs.yaml")
try VFSOverlay(roots: [
VFSOverlay.File(name: manifestPath.pathString, externalContents: manifestTempFilePath.pathString)
]).write(to: vfsOverlayTempFilePath, fileSystem: localFileSystem)

try self.evaluateManifest(
at: manifestPath,
vfsOverlayPath: vfsOverlayTempFilePath,
packageIdentity: packageIdentity,
toolsVersion: toolsVersion,
delegateQueue: delegateQueue,
callbackQueue: callbackQueue,
completion: completion
)
} else {
try withTemporaryFile(suffix: ".swift") { tempFile, cleanupTempFile in
try localFileSystem.writeFileContents(tempFile.path, bytes: ByteString(manifestContents))
try self.evaluateManifest(
at: tempFile.path,
packageIdentity: packageIdentity,
toolsVersion: toolsVersion,
delegateQueue: delegateQueue,
callbackQueue: callbackQueue
) { result in
dispatchPrecondition(condition: .onQueue(callbackQueue))
cleanupTempFile(tempFile)
completion(result)
}
delegateQueue: delegateQueue,
callbackQueue: callbackQueue
) { result in
dispatchPrecondition(condition: .onQueue(callbackQueue))
cleanupTempDir(tempDir)
completion(result)
}
}
} catch {
Expand All @@ -477,6 +474,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
/// Helper method for evaluating the manifest.
func evaluateManifest(
at manifestPath: AbsolutePath,
vfsOverlayPath: AbsolutePath? = nil,
packageIdentity: PackageIdentity,
toolsVersion: ToolsVersion,
delegateQueue: DispatchQueue,
Expand Down Expand Up @@ -512,6 +510,10 @@ public final class ManifestLoader: ManifestLoaderProtocol {
var cmd: [String] = []
cmd += [self.toolchain.swiftCompilerPathForManifests.pathString]

if let vfsOverlayPath = vfsOverlayPath {
cmd += ["-vfsoverlay", vfsOverlayPath.pathString]
}

// if runtimePath is set to "PackageFrameworks" that means we could be developing SwiftPM in Xcode
// which produces a framework for dynamic package products.
if runtimePath.extension == "framework" {
Expand Down
4 changes: 2 additions & 2 deletions Tests/PackageLoadingTests/PD_4_2_LoadingTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -609,7 +609,7 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {

XCTAssertNoDiagnostics(observability.diagnostics)
XCTAssertEqual(try delegate.loaded(timeout: .now() + 1), [manifestPath])
XCTAssertEqual(try delegate.parsed(timeout: .now() + 1), expectCached ? [] : [manifestPath])
XCTAssertEqual(try delegate.parsed(timeout: .now() + 1).count, expectCached ? 0 : 1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switching to count here since the file name doesn't seem relevant.

XCTAssertEqual(manifest.displayName, "Trivial")
XCTAssertEqual(manifest.targets[0].name, "foo")
}
Expand Down Expand Up @@ -669,7 +669,7 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {

XCTAssertNoDiagnostics(observability.diagnostics)
XCTAssertEqual(try delegate.loaded(timeout: .now() + 1), [manifestPath])
XCTAssertEqual(try delegate.parsed(timeout: .now() + 1), expectCached ? [] : [manifestPath])
XCTAssertEqual(try delegate.parsed(timeout: .now() + 1).count, expectCached ? 0 : 1)
XCTAssertEqual(manifest.displayName, "Trivial")
XCTAssertEqual(manifest.targets[0].name, "foo")
}
Expand Down
10 changes: 5 additions & 5 deletions Tests/PackageLoadingTests/PD_5_6_LoadingTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,19 +221,19 @@ class PackageDescription5_6LoadingTests: PackageDescriptionLoadingTests {

let fileManager = FileManager.default
let contents = (try? fileManager.contentsOfDirectory(atPath: Context.packageDirectory)) ?? []
let swiftFiles = contents.filter { $0.hasPrefix("TemporaryFile") && $0.hasSuffix(".swift") }

let package = Package(name: swiftFiles.joined(separator: ","))
let package = Package(name: contents.joined(separator: ","))
"""

let observability = ObservabilitySystem.makeForTesting()
let (manifest, validationDiagnostics) = try loadAndValidateManifest(content, observabilityScope: observability.topScope)
XCTAssertNoDiagnostics(observability.diagnostics)
XCTAssertNoDiagnostics(validationDiagnostics)

let name = parsedManifest.components?.last ?? ""
let swiftFiles = manifest.displayName.split(separator: ",").map(String.init)
XCTAssertNotNil(swiftFiles.firstIndex(of: name))
let files = manifest.displayName.split(separator: ",").map(String.init)
// Since we're loading `/Package.swift` in these tests, the context's package directory is supposed to be /.
let expectedFiles = try FileManager.default.contentsOfDirectory(atPath: "/")
XCTAssertEqual(files, expectedFiles)
}

func testCommandPluginTarget() throws {
Expand Down