File list support #144

cltnschlosser · 2020-06-28T03:17:36Z

Major changes:

VirtualPath enum case addition: case fileList(RelativePath, FileList)
ArgsResolver handles fileList case.
ArgsResolver is now reference type since it should only write each file list once.

DougGregor

Thanks for working on this! I'd like to hear your thoughts on my alternative modeling for the file list files themselves

Sources/SwiftDriver/Jobs/Job.swift

Sources/SwiftDriver/Utilities/FileList.swift

Sources/SwiftDriver/Jobs/FrontendJobHelpers.swift

cltnschlosser · 2020-06-30T00:46:31Z

Okay, still a bit of work todo. Need to refactor the FileList object and ArgsResolver (also add the new VirtualPath case). Then need to write tests for everything, there are a lot of little edge cases caught by the lit tests.

Also I want to pull out a few changes into other smaller PRs.

Newly passing:
Driver/batch_mode_with_supplementary_filelist.swift
Driver/batch_mode_with_supplementary_filelist_unicode.swift
Driver/emit-interface.swift
Driver/filelist-threshold.swift

Mostly working (Just temporary file handling, and some path formatting(I can open swift PR for this)):
Driver/filelists.swift

Issues with Driver/filelists.swift:

Extra temporary directory layer
Temporary files not removed on failure (C++ driver tracks all temporary files and deletes them after it's done. Unless -save-temps is passed, or the job crashes and the files are marked to be saved on signal)

cltnschlosser · 2020-07-01T02:10:59Z

@DougGregor I've hit a snag in the ArgsResolver managing which filelists are written. In order to make the ArgsList actually keep track of this state it needs to be a class (or needs to be provided access to something else that tracks this state). Because two compile jobs can use the same filelist, the ArgsResolver then needs to use locking. Previously two jobs could resolve their arguments entirely in parallel.

I'm going to move forward with the locking to see how it works out, but it does seem less than ideal.

cltnschlosser · 2020-08-18T03:59:04Z

Okay, rebased this. Most tests are failing locally due to could not decode frontend target info; compiler driver and frontend executables may be incompatible. I'm using the latest toolchain on swift.org for xcode.

DougGregor · 2020-08-18T04:41:25Z

Okay, rebased this. Most tests are failing locally due to could not decode frontend target info; compiler driver and frontend executables may be incompatible. I'm using the latest toolchain on swift.org for xcode.

Maybe you're picking up the Swift compiler/frontend executable from the default toolchain. If you haven't don't so yet, I suggest overriding the environment variable SWIFT_DRIVER_SWIFT_FRONTEND_EXEC to point at the swift-frontend executable in the toolchain you downloaded. (And make sure it's a master toolchain; these changes aren't in 5.3)

cltnschlosser · 2020-08-18T04:58:57Z

Maybe you're picking up the Swift compiler/frontend executable from the default toolchain

Yep, this was it. I'm hard coding it now for testing. Looks like xcrun --find only looks inside the default toolchain.

There might be some legitimate failures.

cltnschlosser · 2020-08-18T05:01:19Z

Environment variable works as well. Using that now :)

cltnschlosser · 2020-08-18T06:19:32Z

I think the only TODO that is left is to port some of the lit tests to XCTest, so we have coverage here.

owenv

This is looking really great!

owenv · 2020-08-20T04:22:33Z

Sources/SwiftDriver/Driver/Driver.swift

@@ -1730,7 +1782,7 @@ extension Driver {
        return path
      }

-      return path.parentDirectory.appending(component: "\(moduleName).\(type.rawValue)")
+      return path.replacingExtension(with: type)


I think this and/or the change at line 1889 will break the Driver/opt-record.swift integration test, getting these path computations to match the c++ driver has been really challenging due to the architectural differences between the two implementations. Maybe we can get away with just picking better defaults in swift-driver since most clients that care about these outputs are already using output file maps...

Either way, if this can't be fixed yet and we have to make a choice between file list and optimization record tests passing in the short term, I'd choose file lists 😄

owenv · 2020-08-20T05:02:54Z

Sources/SwiftDriver/Jobs/FrontendJobHelpers.swift

+    // Question: outputs.count > fileListThreshold makes sense, but c++ does the following:
+    if primaryInputs.count * FileType.allCases.count > fileListThreshold {


I think the c++ driver has to overestimate here because it doesn't know how many outputs there will be when it does this calculation. It should be ok to use a tighter bound here if possible

owenv · 2020-08-20T05:05:15Z

Sources/SwiftDriver/Jobs/FrontendJobHelpers.swift

+// I don't like this as a global function, but it needs to be used by both Driver and Toolchain
+func createTemporaryFileName(prefix: String, suffix: String? = nil) -> String {


Maybe the ArgsResolver could be responsible for appending the number? Not sure if that would help much.

owenv · 2020-08-20T05:12:55Z

Sources/SwiftDriver/Execution/ArgsResolver.swift

+      // This uses Yams to escape and quote strings, but not to output the whole yaml file because
+      // it sometimes outputs mappings in explicit block format (https://yaml.org/spec/1.2/spec.html#id2798057)
+      // and the frontend (llvm) only seems to support implicit block format.


Yeah, I can confirm the llvm yaml parser doesn't handle explicit block format unfortunately

…tputPair

cltnschlosser · 2020-09-11T15:29:24Z

Okay, rebased. Resolved concerns from comments and added tests. Should be all good now :)

owenv · 2020-09-12T03:18:04Z

@swift-ci please test

owenv

Everything here looks good to me, thanks again for working on this!

I do think we'll want to rework the temporary file naming at some point to avoid collisions better. TSC has a withTemporaryFile utility for this, but I think it has the same problem as withTemporaryDirectory - it always uses the local file system. I'm fine with merging this as-is as long as @DougGregor doesn't have any remaining concerns!

DougGregor

This looks good to me, thank you for picking it up again!

cltnschlosser · 2020-09-12T14:27:28Z

Okay, merge when ready

compnerd · 2020-09-12T18:36:00Z

This is breaking the swift-package-manager build :-(

swiftlang/swift-driver#144 introduced a new case in the `VirtualPath` type, resulting in the switch no longer being exhaustive. Abort if this is encountered until it can be handled properly.

owenv · 2020-09-12T18:43:03Z

Looks like a switch needs to be updated to support the new case added to VirtualPath:

/home/buildnode/jenkins/workspace/oss-swift-incremental-RA-linux-ubuntu-16_04/swiftpm/Sources/Build/ManifestBuilder.swift:855:9: error: switch must be exhaustive
10:12:41         switch file {
10:12:41         ^

DougGregor reviewed Jun 28, 2020

View reviewed changes

Sources/SwiftDriver/Jobs/Job.swift Outdated Show resolved Hide resolved

Sources/SwiftDriver/Utilities/FileList.swift Outdated Show resolved Hide resolved

Sources/SwiftDriver/Jobs/FrontendJobHelpers.swift Outdated Show resolved Hide resolved

cltnschlosser mentioned this pull request Jun 30, 2020

Fixup module source info output #152

Merged

cltnschlosser force-pushed the cs_fileLists branch 2 times, most recently from 9ef58e3 to 4768b9e Compare August 18, 2020 03:52

cltnschlosser marked this pull request as ready for review August 18, 2020 03:56

owenv reviewed Aug 21, 2020

View reviewed changes

cltnschlosser added 6 commits September 11, 2020 09:08

Filelist support

597fc22

Fix for missing supplementary outputs. Also removes notion of InputOu…

6132321

…tputPair

Don't try to place a file next to standardOutput

8f1a52d

Add FileList.swift to CMakeLists

a3a857d

Resolve PR comments

f0295d9

FileList tests

48e5ff9

cltnschlosser force-pushed the cs_fileLists branch from 0f3e9a1 to 48e5ff9 Compare September 11, 2020 15:27

owenv approved these changes Sep 12, 2020

View reviewed changes

owenv requested a review from DougGregor September 12, 2020 03:28

DougGregor approved these changes Sep 12, 2020

View reviewed changes

DougGregor merged commit e9b27e8 into swiftlang:master Sep 12, 2020

compnerd mentioned this pull request Sep 12, 2020

Revert "File list support" #251

Closed

compnerd mentioned this pull request Sep 12, 2020

Build: repair the build after apple/swift-driver#144 swiftlang/swift-package-manager#2923

Closed

owenv mentioned this pull request Sep 12, 2020

Handle new SwiftDriver.VirtualPath case in ManifestBuilder swiftlang/swift-package-manager#2924

Merged

		// Question: outputs.count > fileListThreshold makes sense, but c++ does the following:
		if primaryInputs.count * FileType.allCases.count > fileListThreshold {

		// I don't like this as a global function, but it needs to be used by both Driver and Toolchain
		func createTemporaryFileName(prefix: String, suffix: String? = nil) -> String {

File list support #144

File list support #144

Uh oh!

Conversation

cltnschlosser commented Jun 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DougGregor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cltnschlosser commented Jun 30, 2020

Uh oh!

cltnschlosser commented Jul 1, 2020

Uh oh!

cltnschlosser commented Aug 18, 2020

Uh oh!

DougGregor commented Aug 18, 2020

Uh oh!

cltnschlosser commented Aug 18, 2020

Uh oh!

cltnschlosser commented Aug 18, 2020

Uh oh!

cltnschlosser commented Aug 18, 2020

Uh oh!

owenv left a comment

Choose a reason for hiding this comment

Uh oh!

owenv Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

owenv Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

owenv Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

owenv Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

cltnschlosser commented Sep 11, 2020

Uh oh!

owenv commented Sep 12, 2020

Uh oh!

owenv left a comment

Choose a reason for hiding this comment

Uh oh!

DougGregor left a comment

Choose a reason for hiding this comment

Uh oh!

cltnschlosser commented Sep 12, 2020

Uh oh!

compnerd commented Sep 12, 2020

Uh oh!

owenv commented Sep 12, 2020

Uh oh!

Uh oh!

cltnschlosser commented Jun 28, 2020 •

edited

Loading