Track required outputs by following output chains #1398
Conversation
Fixes #1397 Add `OptionalOutputTracker` to cache the status of which outputs from optional phases are required. Use the tracker to check which failed outputs were required during the current build. Also pass the tracker `createMergedOutputDirectories` and replace the logic which crawled the other direction.
|
I took a look at putting this logic in If we do that though this will close out #1033 since it would be reflected by the server as well. |
| () => | ||
| generatedNode.outputs | ||
| .any((o) => isRequired(o, currentlyChecking)) || | ||
| _assetGraph |
There was a problem hiding this comment.
I don't quite get this logic - why do we care about other outputs in the phase?
There was a problem hiding this comment.
Oh - I think this is trying to handle the additional outputs of the same action?
Right now I think it is checking all outputs of the same phase which is a lot more than we actually want I believe. We could add an additional where after outputsForPhase that checks if the primary input is the same?
There was a problem hiding this comment.
There was a problem hiding this comment.
Ah I see, yeah that makes sense. Will switch to only those with the same primary input
There was a problem hiding this comment.
We still need to either do the expectedOutputs trick I did there or add a where after the outputsToPhase though to retain the old logic right?
There was a problem hiding this comment.
yeah added a where clause to check the primary inputs
There was a problem hiding this comment.
Lol I really wish github updated comments without having to refresh
|
Added a regression test and doc comments. PTAL |
|
This should make finishing #1033 easier as well yes? |
|
Yes I believe that gets a lot easier to fix. Just need to decide how to structure it. There isn't a clear way to pass the |
Ya - I think multiple instances is probably ok, or we could expose a |
I was thinking I'd like to make a I have been playing around a bit but I'm not really happy with it yet. I didn't want that work to block getting this fixed so I figured we could submit as is and refactor soon. |
| expect(result.stderr, contains('Failed')); | ||
|
|
||
| // Run a build with dart2js that should also fail. Makes the failing DDC | ||
| // outptus no longer necessary for the build but does not invalidate them |
| expect(result.exitCode, isNot(0)); | ||
| expect(result.stderr, contains('Failed')); | ||
|
|
||
| // Run a build with dart2js that should also fail. Makes the failing DDC |
There was a problem hiding this comment.
Not super stoked on relying on the internals here... also note that the ddc output is going to get marked as needing an update as well which might change the logic or hit different codepaths (not sure how we treat failures that didn't rerun).
I would instead create a new file with an error in it (but not imported anywhere) and expect as successful build, then add an import to it and expect a failure, then remove the import again and expect it to pass. This is I expect a common actual user scenario and better representation since there is still an up to date failing action that should simply be ignored.
There was a problem hiding this comment.
As a bonus it's also much faster :)
Skipping the check for the successful build since I don't think it changes the meaning of the test.
Fixes #1397
Add
OptionalOutputTrackerto cache the status of which outputs fromoptional phases are required. Use the tracker to check which failed
outputs were required during the current build. Also pass the tracker
createMergedOutputDirectoriesand replace the logic which crawled theother direction.