Fish Trophy

nhoward_tw got a fish trophy!

[engine] Fix running changed with v2 flag; Replace context address_mapper when using engine; Fix excludes filespecs in engine globs

Review Request #4114 — Created July 23, 2016 and submitted

benjyw, ity, jsirois, patricklaw, stuhood

Changed was failing to work because when the change calculator attempted to look up targets, it would use the existing address mapper. That address mapper would parse the BUILD files for itself, and then pass a BuildFileAddress object to the LegacyBuildGraph. That would blow up because the engine doesn't know how to compute from one of those.

This change introduces a LegacyAddressMapper that uses the engine. This ensures that the interaction between the engine backed build graph and the address mapper will have matching types. To do this, I extracted a common interface class and reimplemented the methods on AddressMapper that are used from other classes.

While working on this, I ran into a bug with exclude handling in the engine's glob implementation. The globs for the excludes weren't being translated into filespecs correctly, which lead to things blowing up. I could break that out as a separate changeset if this is too big.

This patch includes a fix for it and some regression tests.

Note: This introduces a validation that filespecs are prefixed with the relpath of the fileset they are a part of. This is something that changed assumed to be true, but wasn't in all cases. It may cause using absolute paths in BUILD files as part of glob declarations to fail.

Wrote an integration test for changed, got it passing. Added some regression tests around excludes and got them passing. CI away on linked PR.

  • 0
  • 0
  • 6
  • 1
  • 7
Description From Last Updated
  1. This ended up being clean, which is great. Before landing this though, I'd like to see a proposal for allowing this operation to be cached inside the engine in the future, as caching the result of source mapping will likely be an important part of seeing benefit from the daemon. As it stands, the SourceMapper runs post-fork inside of a Task.

    A strawman proposal might be to have a pluggable set of "target root selectors", where the default is to expand the specs from the command line, but the changed goals (somehow?) trigger a different root selection algo that uses the SourceMapper.

    1. Working on some ideas. In the mean time, I've got responses to your comments.

    2. What would you think about introducing a pre-fork hook for tasks? We could do things like alternate_target_roots and maybe prepare using it.
      We'd have to do more calls into task classes, but I think we could come up with a good ruleset for it.

    3. Hm... actually, from what I can tell, alternate_target_roots is probably already a sufficient API for this, as long as we can ensure it runs pre-fork?

  2. src/python/pants/bin/ (Diff revision 2)


    1. Maybe it should warn if they don't match? I think that'd be reasonable.

  3. src/python/pants/bin/ (Diff revision 2)

    Any reason not to just do that here?

    1. Not that I could think of. Added the note to highlight the question. In retrospect, I should have just made the change and reviewed it myself to highlight. I'll do that next time.

  4. src/python/pants/bin/ (Diff revision 2)

    This is the same as the BuildFileAddressMapper construction above... should probably refactor this to remove the duplication...?

    1. Sure. I could extract a helper fn. Though I think if I pull the address mapper above from the cached graph I think it won't be necessary.

  5. Doesn't seem like it.

    1. I'll change that while I'm in here.

  6. Given that the only downside of the dirname(source) == spec_path approach is that it blames all addresses in the directory, rather than only those in a particular BUILD file, I'd prefer to drop the BuildFileAddress usage here. In the vast majority of cases there is only one BUILD file per directory.

    1. I think that's reasonable.

    2. I decided on a different approach.

  7. This ignores the root, but should probably convert it into the constructor parameter for DescendantAddresses?

  8. The extra parens here don't improve clarity; in fact, I'm not confused as to whether you meant to create a tuple here...?

    1. s/not/now/

    2. IntelliJ refactoring sometimes adds unnecessary parens. :/. Thanks for catching that.

  9. src/python/pants/engine/ (Diff revision 2)

    Nice find!

  1. Thanks Nick.

  2. src/python/pants/bin/ (Diff revision 4)

    It seems like in both cases you can just use the _address_mapper from the graph, assuming the creation paths below are correct?

    1. The engine based build graph doesn't have a backing address mapper. Otherwise that would work.

  3. src/python/pants/engine/legacy/ (Diff revision 4)

    Worth commenting (as above) on why this usually doesn't matter very much.

  4. src/python/pants/engine/legacy/ (Diff revision 4)

    Args should either be all on same line, or all on separate lines.

  5. This should always be there, shouldn't it? So no default needed...

    If it isn't, it would be because you weren't running in the new engine, which we're not trying to support.

  6. For symmetry, can mark @property?

  7. Since you've normalized, you should be able to use fast_relpath here to enforce that globs don't traverse out.

  8. IIRC, this is the contract of the "lazy" source mapper?

    In this case, rather than a super long test name, it might be better to have a shorter name and longer comment explaining why the property is desirable.

    1. The spec source mapper already had this behavior. This test ensures both implementations do.

  9. Should (also?) consider using the decorator that Kris introduced in to mark up some existing tests.

    1. Hm. For these tests in particular, it's not that helpful since my aim is to test that both engines provide the same output.

  1. Ship It!
    1. Nick,
      I raised some doubts below. Not sure if I missed anything, but I would like to point out.

  2. I have seen this kind of class inherit AbstractClass. But I don't have strong opinion.

  3. src/python/pants/engine/legacy/ (Diff revision 6)

    I have some doubts here.

    are you trying to make sure filespec is relative to buildroot here?

    1. does filespec have "exclude" attribute instead of "excludes"?

    2. according to your changes in BaseGlobs, filespec.get('excludes', None) is a list of dicts. but looking at FilesetRelPathWrapper.to_filespec, it doesn't work out quite right.

      1. Good catch. to_filespec's kwarg is excludes, but Filesets use exclude. Tricky. Also, this isn't the only engine code to make this mistake :(. I think I might change that while I'm looking because that makes things rather confusing.
      2. Oh, I see. It's because filespec.get('exclude') would be a list of dicts rather than objects with a filespec attr, or strings, so when to_filespec attempts to turn them into the appropriate dict, it'll fail.
  4. src/python/pants/engine/legacy/ (Diff revision 6)

    nit: 1 parameter per line?

  5. it seems the spec_path argument is not really used except assigned to self._spec_path.

    Is it necessary to make it inherit Locatable?

    1. Locatable is a marker class that causes the graph engine tasks to pass spec_path

  1. LGTM!

Review request changed

Status: Closed (submitted)

Change Summary:

Submitted as