-
-
src/python/pants/bin/engine_initializer.py (Diff revision 1) Objects "in" the ProductGraph should not have direct access to the filesystem (ie, the ProjectTree), as that would allow them to bypass filesystem invalidation.
-
src/python/pants/engine/addressable.py (Diff revision 1) BuildFileAddress
extendsAddress
, so this is redundant. -
src/python/pants/engine/addressable.py (Diff revision 1) To be correct, this should still normalize the address as the other branch does, but should pass the additional arguments needed by the BuildFileAddress constructor.
-
src/python/pants/engine/graph.py (Diff revision 1) Rather than adding the AddressMapper reference to the AddressFamily, you should probably request it below in any functions that need it.
It looks like only
resolve_unhydrated_struct
uses it, so to get the additional argument there, you would simply add aSelectLiteral(address_mapper, AddressMapper)
argument to the definition of resolve_unhydrated_struct. -
src/python/pants/engine/mapper.py (Diff revision 1) I don't like this dependency. The
BuildFile
andBuildFileAddressMapper
APIs are the ones we're attempting to replace, so we should be trying to reduce our dependence on them.Could we change
BuildFileAddress
to hold thepath
of a BuildFile, rather than an object?
[engine] fix address node creation in v2 build graph; fix filedeps
Review Request #4250 — Created Sept. 20, 2016 and updated
Information | |
---|---|
ity | |
pants | |
3752, 3891 | |
Reviewers | |
pants-reviews | |
kwlzn, nhoward_tw, peiyu, stuhood, yujiec |
- Preserve build file when mapping to be able to create BuildFileAddress when required.
- Re-use already parsed Addresses in addressables.
- Add project_tree attribute to AddressFamily for completion.
- Add types for BuildFileAddress to validate against during BuildGraph creation.
- Working on this change made it obvious that Address/BuildFileAddress can really be the same thing. They do not benefit from inheritance as such at the moment and I do not see any future subclassing required for these. In fact, they do add to the complexity of parsing, memoizing, retriving. To keep this change simple, split the refactor to another rb.
Locally, ./pants --enable-v2-engine filedeps xx:: was failing before this change, passing now.
Also, did simple functional testing against v1 engine outout.
Change Summary:
Address Stu's comments
-
Thanks, closer!
-
src/python/pants/engine/graph.py (Diff revision 2) This check should probably move to the top with the
if not struct
check, so that those can just be accomplished together.Falling through to the other case would be a fatal case, because the AddressFamily should always contain BuildFileAddress objects, right?
-
src/python/pants/engine/graph.py (Diff revision 2) It's not enforced anywhere yet, but as mentioned before: functions in the graph should not directly access the file system, or the ProjectTree.
So (ideally) we'd remove the ProjectTree reference from
BuildFileAddress
entirely. Similar to how anAddress
object doesn't know which build_root it is relative to, aBuildFileAddress
need not have a ProjectTree.
-
Ity,
you may want to add some integration tests with ensure_engine decorator to verify filedeps works in both v1 and v2 engine and produce same output.
Change Summary:
Addressed Stu & Yujie's comments.
Diff: |
Revision 3 (+137 -50)
|
---|
-
Thanks Ity. It still looks possible to completely remove the BuildFile object from
BuildFileAddress
, but it doesn't seem like a blocker. Maybe a new ticket? -
src/python/pants/backend/project_info/tasks/ide_gen.py (Diff revision 3) This will be broken in the v2 engine, because a BuildFile will not exist.
Rather than getting the ProjectTree from the BuildFile, this Task could probably get it from
self.context
, which would mean that it would only need therel_path
of the BuildFileAddress, rather than an actual BuildFile object.If that change were to be made, it would likely be possible to remove
BuildFileAddress._build_file
entirely, which would be very nice.But, not a blocker... would be good to land this rather than changing too much more.
-
src/python/pants/engine/mapper.py (Diff revision 3) This is a pretty generic argument... if you could either avoid adding it, or carefully document it, that would be best.