Objects "in" the ProductGraph should not have direct access to the filesystem (ie, the ProjectTree), as that would allow them to bypass filesystem invalidation.
Address, so this is redundant.
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.
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_structuses it, so to get the additional argument there, you would simply add a
SelectLiteral(address_mapper, AddressMapper)argument to the definition of resolve_unhydrated_struct.
I don't like this dependency. The
BuildFileAddressMapperAPIs are the ones we're attempting to replace, so we should be trying to reduce our dependence on them.
Could we change
BuildFileAddressto hold the
pathof 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
|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.
Address Stu's comments
This check should probably move to the top with the
if not structcheck, 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?
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
BuildFileAddressentirely. Similar to how an
Addressobject doesn't know which build_root it is relative to, a
BuildFileAddressneed not have a ProjectTree.
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.
Addressed Stu & Yujie's comments.
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?
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 the
rel_pathof the BuildFileAddress, rather than an actual BuildFile object.
If that change were to be made, it would likely be possible to remove
BuildFileAddress._build_fileentirely, which would be very nice.
But, not a blocker... would be good to land this rather than changing too much more.
This is a pretty generic argument... if you could either avoid adding it, or carefully document it, that would be best.