Skip to content

[Incremental] Add first file removal test #694

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

Merged
merged 4 commits into from
Jun 3, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,7 @@ extension IncrementalCompilationState {
}
}
}

@_spi(Testing) public func writeDependencyGraph() throws {
// If the cross-module build is not enabled, the status quo dictates we
// not emit this file.
Expand All @@ -515,6 +516,12 @@ extension IncrementalCompilationState {
on: self.driver.fileSystem,
compilerVersion: recordInfo.actualSwiftVersion)
}

@_spi(Testing) public static func removeDependencyGraphFile(_ driver: Driver) {
if let path = driver.buildRecordInfo?.dependencyGraphPath {
try? driver.fileSystem.removeFileTree(path)
}
}
}

// MARK: - OutputFileMap
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ extension IncrementalCompilationState {
driver.diagnosticEngine
).compute()
else {
Self.removeDependencyGraphFile(driver)
return nil
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -466,7 +466,7 @@ extension ModuleDependencyGraph {
// MARK: - verification
extension ModuleDependencyGraph {
@discardableResult
func verifyGraph() -> Bool {
@_spi(Testing) public func verifyGraph() -> Bool {
nodeFinder.verify()
}
}
Expand Down
6 changes: 6 additions & 0 deletions Sources/SwiftDriver/Utilities/VirtualPath.swift
Original file line number Diff line number Diff line change
Expand Up @@ -707,6 +707,12 @@ extension TSCBasic.FileSystem {
}
}

func removeFileTree(_ path: VirtualPath) throws {
try resolvingVirtualPath(path) { absolutePath in
try self.removeFileTree(absolutePath)
}
}

func getFileInfo(_ path: VirtualPath) throws -> TSCBasic.FileInfo {
try resolvingVirtualPath(path, apply: getFileInfo)
}
Expand Down
248 changes: 243 additions & 5 deletions Tests/SwiftDriverTests/IncrementalCompilationTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ final class IncrementalCompilationTests: XCTestCase {
var masterSwiftDepsPath: AbsolutePath {
derivedDataPath.appending(component: "\(module)-master.swiftdeps")
}
var priorsPath: AbsolutePath {
derivedDataPath.appending(component: "\(module)-master.priors")
}
func swiftDepsPath(basename: String) -> AbsolutePath {
derivedDataPath.appending(component: "\(basename).swiftdeps")
}
var autolinkIncrementalExpectations: [String] {
[
"Incremental compilation: Queuing Extracting autolink information for module \(module)",
Expand Down Expand Up @@ -239,7 +245,7 @@ fileprivate extension Driver {
}
}

// MARK: - Actual incremental tests
// MARK: - Simpler incremental tests
extension IncrementalCompilationTests {

// FIXME: why does it fail on Linux in CI?
Expand Down Expand Up @@ -271,7 +277,6 @@ extension IncrementalCompilationTests {
#endif
}


/// Ensure that the mod date of the input comes back exactly the same via the build-record.
/// Otherwise the up-to-date calculation in `IncrementalCompilationState` will fail.
func testBuildRecordDateAccuracy() throws {
Expand Down Expand Up @@ -313,6 +318,116 @@ extension IncrementalCompilationTests {
}
}

// MARK: - Incremental file removal tests
/// In order to ensure robustness, test what happens under various conditions when a source file is
/// removed.
/// The following is a lot of work to get something that prints nicely. Need an enum with both a string and an int value.
fileprivate enum RemovalTestOption: String, CaseIterable, Comparable, Hashable, CustomStringConvertible {
case
removeInputFromInvocation,
removeSourceFile,
removePreviouslyAddedInputFromOutputFileMap,
removeSwiftDepsFile,
restoreBadPriors

private static let byInt = [Int: Self](uniqueKeysWithValues: allCases.enumerated().map{($0, $1)})
private static let intFor = [Self: Int](uniqueKeysWithValues: allCases.enumerated().map{($1, $0)})

var intValue: Int {Self.intFor[self]!}
init(fromInt i: Int) {self = Self.byInt[i]!}

static func < (lhs: RemovalTestOption, rhs: RemovalTestOption) -> Bool {
lhs.intValue < rhs.intValue
}
var mask: Int { 1 << intValue}
static let maxIntValue = allCases.map {$0.intValue} .max()!
static let maxCombinedValue = (1 << maxIntValue) - 1

var description: String { rawValue }
}

/// Only 5 elements, an array is fine
fileprivate typealias RemovalTestOptions = [RemovalTestOption]

extension RemovalTestOptions {
fileprivate static let allCombinations: [RemovalTestOptions] =
(0...RemovalTestOption.maxCombinedValue) .map(decoding)

fileprivate static func decoding(_ bits: Int) -> Self {
RemovalTestOption.allCases.filter { opt in
(1 << opt.intValue) & bits != 0
}
}
}

extension IncrementalCompilationTests {
/// While all cases are being made to work, just test for now in known good cases
func testRemovalInPassingCases() throws {
try testRemoval(includeFailingCombos: false)
}

/// Someday, turn this test on and test all cases
func testRemovalInAllCases() throws {
throw XCTSkip("unimplemented")
try testRemoval(includeFailingCombos: true)
}

func testRemoval(includeFailingCombos: Bool) throws {
#if !os(Linux)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's special about Linux here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't recall at the moment, but these tests don't seem to work there. I'll file a radar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tests don't work there. I'll file a radar on myself to investigate.

let knownGoodCombos: [[RemovalTestOption]] = [
[.removeInputFromInvocation],
]
for optionsToTest in RemovalTestOptions.allCombinations {
if knownGoodCombos.contains(optionsToTest) {
try testRemoval(optionsToTest)
}
else if includeFailingCombos {
try testRemoval(optionsToTest)
}
}
#endif
}

private func testRemoval(_ options: RemovalTestOptions) throws {
guard !options.isEmpty else {return}
print("*** testRemoval \(options) ***", to: &stderrStream); stderrStream.flush()

let newInput = "another"
let topLevelName = "nameInAnother"
try testAddingInput(newInput: newInput, defining: topLevelName)
if options.contains(.removeSourceFile) {
removeInput(newInput)
}
if options.contains(.removeSwiftDepsFile) {
removeSwiftDeps(newInput)
}
if options.contains(.removePreviouslyAddedInputFromOutputFileMap) {
// FACTOR
Copy link
Contributor

Choose a reason for hiding this comment

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

Which entry is removed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to removePreviouslyAddedInputFromOutputFileMap

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed for clarity.

OutputFileMapCreator.write(module: module,
inputPaths: inputPathsAndContents.map {$0.0},
derivedData: derivedDataPath,
to: OFM)
}
let includeInputInInvocation = !options.contains(.removeInputFromInvocation)
do {
let wrapperFn = options.contains(.restoreBadPriors)
? preservingPriorsDo
: {try $0()}
try wrapperFn {
try self.checkNonincrementalAfterRemoving(
removedInput: newInput,
defining: topLevelName,
includeInputInInvocation: includeInputInInvocation)
}
}
try checkRestorationOfIncrementalityAfterRemoval(
removedInput: newInput,
defining: topLevelName,
includeInputInInvocation: includeInputInInvocation,
afterRestoringBadPriors: options.contains(.restoreBadPriors))
}
}

// MARK: - Incremental test stages
extension IncrementalCompilationTests {
/// Setup the initial post-build state.
Expand Down Expand Up @@ -622,6 +737,95 @@ extension IncrementalCompilationTests {
XCTAssert(graph.contains(sourceBasenameWithoutExt: newInput))
XCTAssert(graph.contains(name: topLevelName))
}

/// Check fallback to nonincremental build after a removal.
///
/// - Parameters:
/// - newInput: The basename without extension of the removed input
/// - defining: A top level name defined by the removed file
/// - includeInputInInvocation: include the removed input in the invocation
private func checkNonincrementalAfterRemoving(
removedInput: String,
defining topLevelName: String,
includeInputInInvocation: Bool
) throws {
let extraArguments = includeInputInInvocation
? [inputPath(basename: removedInput).pathString]
: []
try doABuild(
"after removal of \(removedInput)",
checkDiagnostics: true,
extraArguments: extraArguments,
expectingRemarks: [
"Incremental compilation: Incremental compilation has been disabled, because the following inputs were used in the previous compilation but not in this one: \(removedInput).swift",
"Found 2 batchable jobs",
"Forming into 1 batch",
"Adding {compile: main.swift} to batch 0",
"Adding {compile: other.swift} to batch 0",
"Forming batch job from 2 constituents: main.swift, other.swift",
"Starting Compiling main.swift, other.swift",
"Finished Compiling main.swift, other.swift",
"Starting Linking theModule",
"Finished Linking theModule",
],
whenAutolinking: autolinkLifecycleExpectations)
.verifyNoGraph()

verifyNoPriors()
}

/// Ensure that incremental builds happen after a removal.
///
/// - Parameters:
/// - newInput: The basename without extension of the new file
/// - topLevelName: The top-level decl name added by the new file
@discardableResult
private func checkRestorationOfIncrementalityAfterRemoval(
removedInput: String,
defining topLevelName: String,
includeInputInInvocation: Bool,
afterRestoringBadPriors: Bool
) throws -> ModuleDependencyGraph {
let extraArguments = includeInputInInvocation
? [inputPath(basename: removedInput).pathString]
: []
let expectations = afterRestoringBadPriors
? [
"Incremental compilation: Read dependency graph",
"Incremental compilation: Enabling incremental cross-module building",
"Incremental compilation: May skip current input: {compile: main.o <= main.swift}",
"Incremental compilation: May skip current input: {compile: other.o <= other.swift}",
"Incremental compilation: Skipping input: {compile: main.o <= main.swift}",
"Incremental compilation: Skipping input: {compile: other.o <= other.swift}",
"Incremental compilation: Skipping job: Linking theModule; oldest output is current",
"Skipped Compiling main.swift",
"Skipped Compiling other.swift",
].map(Diagnostic.Message.remark)
: [
"Incremental compilation: Created dependency graph from swiftdeps files",
"Incremental compilation: Enabling incremental cross-module building",
"Incremental compilation: May skip current input: {compile: main.o <= main.swift}",
"Incremental compilation: May skip current input: {compile: other.o <= other.swift}",
"Incremental compilation: Skipping input: {compile: main.o <= main.swift}",
"Incremental compilation: Skipping input: {compile: other.o <= other.swift}",
"Incremental compilation: Skipping job: Linking theModule; oldest output is current",
"Skipped Compiling main.swift",
"Skipped Compiling other.swift",
].map(Diagnostic.Message.remark)
let graph = try doABuild(
"after after removal of \(removedInput)",
checkDiagnostics: true,
extraArguments: extraArguments,
expecting: expectations,
expectingWhenAutolinking: autolinkLifecycleExpectations.map(Diagnostic.Message.remark))
.moduleDependencyGraph()

graph.verifyGraph()
graph.ensureOmits(sourceBasenameWithoutExt: removedInput)
graph.ensureOmits(name: topLevelName)

return graph
}
}

// MARK: - Incremental test perturbation helpers
Expand All @@ -632,6 +836,18 @@ extension IncrementalCompilationTests {
try! localFileSystem.writeFileContents(path) { $0 <<< contents }
}

private func removeInput(_ name: String) {
print("*** removing input \(name) ***", to: &stderrStream); stderrStream.flush()
try! localFileSystem.removeFileTree(inputPath(basename: name))
}

private func removeSwiftDeps(_ name: String) {
print("*** removing swiftdeps \(name) ***", to: &stderrStream); stderrStream.flush()
let swiftDepsPath = swiftDepsPath(basename: name)
XCTAssert(localFileSystem.exists(swiftDepsPath))
try! localFileSystem.removeFileTree(swiftDepsPath)
}

private func replace(contentsOf name: String, with replacement: String ) {
print("*** replacing \(name) ***", to: &stderrStream); stderrStream.flush()
let path = inputPath(basename: name)
Expand All @@ -648,10 +864,27 @@ extension IncrementalCompilationTests {
$0 <<< contents
}
}

private func readPriors() -> ByteString? {
try? localFileSystem.readFileContents(priorsPath)
}

private func writePriors( _ contents: ByteString) {
try! localFileSystem.writeFileContents(priorsPath, bytes: contents)
}

private func preservingPriorsDo(_ fn: () throws -> Void ) throws {
let contents = try XCTUnwrap(readPriors())
try fn()
writePriors(contents)
}

private func verifyNoPriors() {
XCTAssertNil(readPriors().map {"\($0.count) bytes"}, "Should not have found priors")
}
}

// MARK: - Graph inspection

fileprivate extension Driver {
func moduleDependencyGraph() throws -> ModuleDependencyGraph {
do {return try XCTUnwrap(incrementalCompilationState?.moduleDependencyGraph) }
Expand All @@ -660,6 +893,9 @@ fileprivate extension Driver {
throw error
}
}
func verifyNoGraph() {
XCTAssertNil(incrementalCompilationState)
}
}

fileprivate extension ModuleDependencyGraph {
Expand All @@ -677,12 +913,14 @@ fileprivate extension ModuleDependencyGraph {
}
func ensureOmits(sourceBasenameWithoutExt target: String) {
nodeFinder.forEachNode { node in
XCTAssertFalse(node.contains(sourceBasenameWithoutExt: target))
XCTAssertFalse(node.contains(sourceBasenameWithoutExt: target),
"graph should omit source: \(target)")
}
}
func ensureOmits(name: String) {
nodeFinder.forEachNode { node in
XCTAssertFalse(node.contains(name: name))
XCTAssertFalse(node.contains(name: name),
"graph should omit decl named: \(name)")
}
}
}
Expand Down