[Incremental] Support reading priors and incremental external dependencies. #472
Conversation
| @CodaFi You might want to wait till this is more polished and functional, but it's fine it you want to start looking now. |
| @swift-ci please test |
davidungar force-pushed the now-what branch 2 times, most recently from 488bbaf to fabdaac Compare | @swift-ci please test |
| | ||
| var asExternal: ExternalDependency { | ||
| ExternalDependency(self) | ||
| try! ExternalDependency(self, in: localFileSystem) |
There was a problem hiding this comment.
This ought to be a function that yields the error back to the caller.
| /// line. | ||
| private var consumed: [Bool] = [] | ||
| | ||
| public static let mock = ParsedOptions() |
There was a problem hiding this comment.
Why is this part of the interface?
There was a problem hiding this comment.
Beats me! Good catch, I'm removing it.
| //===----------------------------------------------------------------------===// | ||
| | ||
| extension Sequence { | ||
| public func flatMapToSet<Outputs: Sequence, Output: Hashable> |
There was a problem hiding this comment.
Let's not call this flatMap since it doesn't respect the usual monad laws. Perhaps mapUnique?
There was a problem hiding this comment.
mapUnique is good. I'll also be thinking about @slavapestov 's suggestion. See how it comes out with lazy collections.
There was a problem hiding this comment.
After using lazy collections, I think there was only one use of this, so I removed it.
| | ||
| /// A type to hold things that have changed (and will need to cause compilations) | ||
| | ||
| public struct ChangedThings<Thing: Hashable>: Sequence { |
There was a problem hiding this comment.
How about ChangeSet<Thing: Hashable> so it'll read as "changeset of Things"
There was a problem hiding this comment.
Yesterday, I changed the word "changed" to "invalidated", so now it is "SetOfInvalidated", so it reads as SetOfInvalidated<Thing>. But the aliases are still InvalidatedInputs, etc.
There was a problem hiding this comment.
I'm a bit tempted to eliminate this entire type and just go back to sets. What do you think?
There was a problem hiding this comment.
I decided the abstraction wasn't pulling its weight. Removed it.
| Did a first pass. A lot of the logic in this PR is obscured by the unruly diffs. I recommend splitting the data-structure-y bits out into separate PRs so we can tidy things up here. |
Thank you so much, I really appreciate you taking a look. I did some renaming to make things more consistent today, and I plan to look at your comments tomorrow and see about other cleanups as well as starting to split things up into digestible pieces. |
| @swift-ci please test |
| @swift-ci please test |
davidungar force-pushed the now-what branch 2 times, most recently from 7d80088 to 24f8783 Compare | /// FIXME: This option is transitory. We intend to make this the | ||
| /// default behavior. This option should flip to a "disable" bit after that. | ||
| public static let enableCrossModuleIncrementalBuild = Options(rawValue: 1 << 4) | ||
| public static let isCrossModuleIncrementalBuildEnabled = Options(rawValue: 1 << 4) |
There was a problem hiding this comment.
This is more the name of a predicate than the name of an element of an option set.
There was a problem hiding this comment.
Yes, that was a mistake. Sorry. Wanted to rename the boolean var.
| @_spi(Testing) public let alwaysRebuildDependents: Bool | ||
| @_spi(Testing) public let isCrossModuleIncrementalBuildEnabled: Bool | ||
| @_spi(Testing) public let verifyDependencyGraphAfterEveryImport: Bool | ||
| @_spi(Testing) public let emitDependencyDotFileAfterEveryImport: Bool |
There was a problem hiding this comment.
Any objections to just storing an Options here instead of exploding all the bits out into independent fields?
There was a problem hiding this comment.
The option set is a great way to represent all the options, and may grow over time. The intention here was to only capture the ones that mattered, but if almost all of them are needed, then might as well store the option set. I don't know... there's something appealing about the predicates; it feels like the decomposition isn't quite right, yet. Maybe there ought to be a configuration object that includes the options, the reporter, the diagnostic engine, etc. It could have predicate interfaces to a stored option set....
There was a problem hiding this comment.
Looking at it again... I like that a caller can just say info. alwaysRebuildDependents, so if storing the options, I would still want the accessor. But then it seems a bit better to just store the bits. How strongly do you feel? What's the argument for the other side? I can live with the other way.
| self.disappeared = previous.filter {!currentSet.contains($0)} | ||
| } | ||
| | ||
| func wasPresentBefore(_ file: VirtualPath) -> Bool { |
There was a problem hiding this comment.
This function name is quite confusing. Perhaps this reads better as just a raw member access a la self.sourceFiles.previous.contains(file)
There was a problem hiding this comment.
Yes, I've gone back-and-forth. Maybe there's a better name?
There was a problem hiding this comment.
How about isANewInput? Caller reads better that way, anyway.
| precondition(sourceFiles.disappeared.isEmpty, | ||
| "Would have to remove nodes from the graph if reading prior") | ||
| if readPriorsFromModuleDependencyGraph { | ||
| return readPriorGraphAndCollectInputsInvalidatedByChangedOrAddedExternals() |
There was a problem hiding this comment.
Hey, what if we made a protocol and two implementations for each path here and decided the actual computation methodology at this point? I think that would help to modularize the code and eliminate the need for a lot of these extremely long names.
There was a problem hiding this comment.
Sounds good. I need to see a concrete example to get the picture. It does feel like a lot of these functions are points in a sparse multidimensional space, where two of the dimensions are the input and output types.
There was a problem hiding this comment.
Maybe in a clean-up phase after landing?
| Thanks, @CodaFi , for taking a look. |
davidungar force-pushed the now-what branch 2 times, most recently from 0157a14 to f0194c9 Compare | @swift-ci please test |
| @swift-ci please test macos platform |
| @swift-ci please test macos platform |
| @swift-ci please test |
| @swift-ci please test |
davidungar changed the title | @CodaFi I'm not sure how to break this down further for review. Care to dive in? Or do you have a suggestion for a further break-up? |
| @swift-ci please test |
| @swift-ci please test |
2 participants
After thinking about strategies, here is a rough cut, unfinished. It passes the XC tests locally, and I want to see if it will pass on CI.
I have yet to test it with the legacy lit tests, nor test how it works for incremental, cross-module builds.
And there are #warnings, where it needs more work.
EDIT: Now passes lit tests
EDIT: I think it might be ready for landing, modulo what is found by a review.