Targets always have a Snapshot #5994
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
illicitonion
force-pushed
the
dwagnerhall/lazyfileset/snapshotseverywhere
branch
2 times, most recently
from
41536af
to
1a81185
Compare
illicitonion
force-pushed
the
dwagnerhall/lazyfileset/snapshotseverywhere
branch
from
1a81185
to
82341e7
Compare
stuhood
approved these changes
Jun 22, 2018
| """ | ||
| :param rel_root: The root for the given filespec, relative to the buildroot. | ||
| :param filespec: A filespec as generated by `FilesetRelPathWrapper`, which represents | ||
| what globs or file list it came from. Must be relative to buildroot. | ||
| :param files: A list of matched files, with declared order and duplicates preserved. |
There was a problem hiding this comment.
The "duplicates" bit was a lie anyway.
| # Build the target and handle duplicate sources. | ||
| if not vt.valid: | ||
| if self._do_validate_sources_present(vt.target): | ||
| self.execute_codegen(vt.target, vt.results_dir) | ||
| self._handle_duplicate_sources(vt.target, vt.results_dir) | ||
| sources = self._capture_sources(vt.target, vt.results_dir) |
There was a problem hiding this comment.
This would be a very good usecase for using the batchness of the capture_snapshots method, and I think we're going to need to here to get adequate performance on a large graph.
If you split out a "generate and collect sources" loop before a "inject synthetic targets" loop, you could make one call to capture_snapshots for all targets in between the two loops.
kwlzn
approved these changes
Jun 22, 2018
| 'deprecated, and now takes a slow path. Instead, class {} should have its sources ' | ||
| 'argument populated by the engine, either by using the standard parsing pipeline, or by ' | ||
| 'requesting a SourcesField product.' | ||
| 'the v2 engine.').format(self.__class__.__name__) |
There was a problem hiding this comment.
s/requesting a SourcesField product.the v2 engine./requesting a SourcesField product from the v2 engine./ ?
|
For my reference: I can reproduce the CI failures by running: |
Unless they're somehow specially generated, their Snapshot will be popualted during parsing in parallel in the engine. If they weren't generated during parsing, the scheduler arg will be used to synchronously capture one on demand.
illicitonion
force-pushed
the
dwagnerhall/lazyfileset/snapshotseverywhere
branch
from
82341e7
to
5d9ac89
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Unless they're somehow specially generated, their Snapshot will be
populated during parsing in parallel in the engine.
If they weren't generated during parsing, the scheduler arg will be used
to synchronously capture one on demand.
This enables the sources of all Targets to be used in remote execution.