Only make input paths absolute when -working-directory is passed #1708
Conversation
|
@swift-ci please test |
There was a problem hiding this comment.
The direction of the change works for me. Forgot distributed build is using the relative path to do the compilation.
Note there is an alternative to use -scanner-prefix-map to alter the path returned from the scanner but I guess that option requires knowing the path on the remote node before hand.
| @@ -389,7 +389,7 @@ extension VirtualPath { | |||
| } | |||
|
|
|||
| fileprivate func intern(virtualPath path: VirtualPath) -> VirtualPath.Handle { | |||
| return self.queue.sync(flags: .barrier) { | |||
| return self.queue.sync(flags: .barrier) { | |||
There was a problem hiding this comment.
Unnecessary white space.
| try Self.applyWorkingDirectory(workingDirectory, to: &self.parsedOptions) | ||
| } | ||
|
|
||
| if let specifiedWorkingDir = self.workingDirectory { |
There was a problem hiding this comment.
Maybe the code will be cleaner (save all the ?? fileSystem.currentWorkingDirectory) if self.workingDirectory is always set regardless of if -working-directory is passed but only calls applyWorkingDirectory if the option is passed.
There was a problem hiding this comment.
I tried that initially, but then the output paths (and profile data paths) end up absolute. I didn't track down where that was happening, but there seems to be multiple places in the code that would end up prefixing paths if workingDirectory was set.
There was a problem hiding this comment.
Maybe driver will think that working-directory is passed via command-line so it is passing working directory (in abs path) frontend flag plus some other flags derived from working directory?
Sounds fine with me.
| @@ -45,12 +45,15 @@ private var testInputsPath: AbsolutePath { | |||
| } | |||
| } | |||
|
|
|||
| func toPath(_ path: String, base: AbsolutePath = localFileSystem.currentWorkingDirectory!) throws -> VirtualPath { | |||
| return try VirtualPath(path: path).resolvedRelativePath(base: base) | |||
| func toPath(_ path: String, isRelative: Bool = true) throws -> VirtualPath { | |||
There was a problem hiding this comment.
I am slightly prefer leaving the tests in the new state but add -working-directory to the driver command.
Also, would be nice to add a test to check the behavior of path arguments with/without -working-directory
There was a problem hiding this comment.
I am slightly prefer leaving the tests in the new state but add -working-directory to the driver command.
For the changed tests? For testDOTFileEmission that would work, for the others it would require adding isRelative: false to all the toPath() calls, which would be a bigger change.
Also, would be nice to add a test to check the behavior of path arguments with/without -working-directory
I added testRelativeInputs on L7239 for this
| try Self.applyWorkingDirectory(workingDirectory, to: &self.parsedOptions) | ||
| } | ||
|
|
||
| if let specifiedWorkingDir = self.workingDirectory { |
There was a problem hiding this comment.
Maybe driver will think that working-directory is passed via command-line so it is passing working directory (in abs path) frontend flag plus some other flags derived from working directory?
Sounds fine with me.
|
@swift-ci please test |
|
@swift-ci Please test Windows platform |
| XCTAssertEqual(emitModuleJob.outputs[4].file, try toPath("Test.private.swiftinterface")) | ||
| XCTAssertEqual(emitModuleJob.outputs[5].file, try toPath("Test-Swift.h")) | ||
| XCTAssertEqual(emitModuleJob.outputs[6].file, try toPath("Test.tbd")) | ||
| XCTAssertEqual(emitModuleJob.outputs[6].file, try VirtualPath(path: "./Test.tbd")) |
There was a problem hiding this comment.
@rmaz do you recall why this change was needed for only these specific outputs?
There was a problem hiding this comment.
I don't, but changing it to toPath() gives us:
XCTAssertEqual failed: ("./Test.swiftinterface") is not equal to ("Test.swiftinterface")
So somewhere a CWD of . is being prefixed only on the swiftinterface path and the tbd path.
There was a problem hiding this comment.
After some code wandering, it is because the path is unspecified in the compiler args, so falls back to this codepath to determine it:
swift-driver/Sources/SwiftDriver/Driver/Driver.swift
Lines 3623 to 3626 in eba1952
This ends up calling dirname on a relative path with no folder components, which ends up here:
https://github.com/swiftlang/swift-tools-support-core/blob/b464fcd8d884e599e3202d9bd1eee29a9e504069/Sources/TSCBasic/Path.swift#L598-L605
This ends up calculating the parent as . and generates the filename as modulename.swiftinterface.
This is a partial revert of a9f35bc
When remotely building and sharing Swift artifacts it is important to maintain
relative paths to avoid serializing any paths that will not be valid on different
machines. Change the logic back to the previous model, and the one used
in the old swift driver, where paths are made absolute only when a
-working-directoryflag is passed.