Remove artifacts from JarDependency; Kill IvyArtifact.

Review Request #2858 — Created Sept. 22, 2015 and submitted

1410, 2235
areitz, gmalmquist, kwlzn, mateor, nhoward_tw, patricklaw, tejal, zundel
The `IvyArtifact` and its collection inside `JarDependency` objects is
killed in favor of declaring 1 jar dependency per needed artifact.  This
was already the way to declare custom classifier dependencies, but the
vestigal artifacts system was engaged prior to this change.  Now
artifact attributes are rolled up into `JarDependency` and only the ivy
resolution code knows to aggregate multiple jars for the same coordinate
with different classifiers into 1 ivy dependency.  The user-facing model
shows no signs of this implementation detail.

The only remaining evidence of ivy in the BUILD UI for jars is the
`type_` paramater which is now deprecated in favor of `ext`.

Tests are updated and `JarPublish` is simplified on the ivy end where
the ivy.xml need not record external dependencies since it just drives
the publication machinery for poms and various jars and other artifacts.

 3rdparty/BUILD                                                                                                                    |  18 +++--                                                                                                                       |  18 +++--
 contrib/spindle/src/python/pants/contrib/spindle/targets/                                                |   3 -
 src/python/pants/backend/android/tasks/                                                                        |   4 +-
 src/python/pants/backend/core/tasks/                                                                                |   2 +-
 src/python/pants/backend/jvm/BUILD                                                                                                |   1 -
 src/python/pants/backend/jvm/                                                                                         | 158 +++++++++++++++++++-------------------
 src/python/pants/backend/jvm/                                                                                          |   3 +-
 src/python/pants/backend/jvm/targets/BUILD                                                                                        |   1 +
 src/python/pants/backend/jvm/targets/                                                                            | 208 +++++++++++++-------------------------------------
 src/python/pants/backend/jvm/targets/                                                                                   |   9 +--
 src/python/pants/backend/jvm/tasks/BUILD                                                                                          |   1 +
 src/python/pants/backend/jvm/tasks/                                                                        |   2 +-
 src/python/pants/backend/jvm/tasks/                                                                                 |  11 ++-
 src/python/pants/backend/jvm/tasks/                                                                              |  45 ++++++-----
 src/python/pants/backend/jvm/tasks/                                                                                 |  90 +++++++++-------------
 src/python/pants/backend/jvm/tasks/                                                                                 |   4 +-
 src/python/pants/backend/project_info/tasks/                                                                             |   4 +-
 src/python/pants/backend/project_info/tasks/                                                                            |   2 +-
 testprojects/src/scala/org/pantsbuild/testproject/publish/classifiers/BUILD                                                       |  22 +++---
 tests/python/pants_test/backend/codegen/tasks/BUILD                                                                               |   1 +
 tests/python/pants_test/backend/codegen/tasks/                                                                    |   5 +-
 tests/python/pants_test/backend/jvm/targets/BUILD                                                                                 |  13 ----
 tests/python/pants_test/backend/jvm/targets/                                                                |  60 ---------------
 tests/python/pants_test/backend/jvm/targets/                                                                    |   4 +-
 tests/python/pants_test/backend/jvm/tasks/                                                                     |  75 +++++++++---------
 tests/python/pants_test/base/                                                                                |   2 +-
 tests/python/pants_test/jvm/BUILD                                                                                                 |   1 -
 tests/python/pants_test/jvm/                                                                            |   2 +
 tests/python/pants_test/tasks/jar_publish_resources/org.pantsbuild.testproject.publish.hello/welcome/ivy-0.0.1-SNAPSHOT.xml       |   3 -
 tests/python/pants_test/tasks/jar_publish_resources/org.pantsbuild.testproject.publish/classifiers/classifiers-0.0.1-SNAPSHOT.pom |   4 +-
 tests/python/pants_test/tasks/jar_publish_resources/org.pantsbuild.testproject.publish/classifiers/ivy-0.0.1-SNAPSHOT.xml         |  17 -----
 tests/python/pants_test/tasks/jar_publish_resources/org.pantsbuild.testproject.publish/jvm-example-lib/ivy-0.0.1-SNAPSHOT.xml     |   3 -
 tests/python/pants_test/tasks/                                                                                 |  18 ++---
 34 files changed, 293 insertions(+), 521 deletions(-)
CI went green here:
  • 0
  • 0
  • 4
  • 2
  • 6
Description From Last Updated
  1. There are a lot of folks on this review, but you probably have a limited bit to check out.

    • For Eric and Nick, its the core change - simplifying JarDependency and the IvyUtils fallout to handle this.
    • For Andy and Tejal its the fallout in JarPublish.
    • For Garrett and Mateo its the fallout in Unpack{Jars,Libraries}.
    • For Kris its Depmap.
    • For Patrick it was general interest in the ivy changes this continues from last week.

    Thanks all for staring at your bits!

  2. Not your change, but this error should probably include the coordinates for the rev that failed to parse, otherwise it could be tricky to find.

    1. Actually, `lenient` does not raise, so dropped the try/except.  Beefed up tests.
  3. src/python/pants/backend/jvm/ (Diff revision 1)

    How about listing the jars here rather than the conflicting dependency named tuples? That way it'd at least have all the information provided in the jar_library declarations.

    1. JarDependency has no full featured string representation though and Dependency does, including the key bits relevant to conflict.  I'm going to push back on this since my TODO has the right answer - we need the BUILD addresses of targets the jars come from which is doable, but too big for this RB.  Formalized the TODO in an issue though:
  4. Could we call out this removal a little more explicitly in the title so that it shows up more clearly in the changelog?

    Maybe something like
    Remove artifacts from JarDependency; Kill IvyArtifact.

    I think that makes sense, since the simplification as I'm reading it is removing artifacts from the JarDependency data model and pulling the thread for that change.

  5. Is this used? I don't see where /tmp/ivy.xml is referenced from. Also, the inline import seems atypical for pants code to me.

    1. Thanks for spotting this - it was debugging cruft, removed.
  6. I'm not sure I understand why this changed. It seems like this means the ivy file will be dropped somewhere else. Is this where /tmp/ivy.xml comes in?

    1. It was never needed (*).  The main thrust of the change was moving the artifacts concept out of JarDependency and into its users who cared - IvyXXX and JarPublish.  IvyXXX needs to know how to map many disconnected jars into 1 <dependency/> and that's impemented.  I realized I needed to use similar logic in JarPublish to emit it's ivy.xmls, but then I realized those ivy.xmls were not being used in the same way and did not need <dependency/> elements at all.  That extreme would have blown up the RB, and just dropping external dependencies was enough to stay simple and not replicate that IvyXXX aggregation logic, but in the course of poking around at exactly what info was needed by ivy to do a publish, I disocvered the deliver to was always un-needed cruft.
      (*) in fact it is needed simply to keep the CWD clean of ivy files, added back, but with a clarifying comment and filename to make clear its a hack to avoid ivy cruft from ivy files we don't need.
  7. Makes sense, the original code looks like a bug. Could you add a a regression test for this?

    1. I corrected the IT which was already setup to test for this but just checked in (1) bad golden file.
  8. Hmm. I'm not sure I like this. The existing code is sorted as it is to ensure dependencies and jar_dependencies are in alphanumeric sorted order to make them easier to scan. This would revert them to a graph based ordering which will make using depmap trickier.

    Maybe we should have a test that validates the ordering since it's a UX thing, just to make that more clear.

    1. yeah, not sure I get this either. what's the rationale here?

    2. The sorting was relying on < defined on JarDependency as a tuple sort over org,name,rev,classifier. That operator scared the heck out of me and I wanted to make sure it was unused to implement say IvyXXX or JarPublish specific logic about how to sort deps. I only found use here and the sorting didn't make sense to me since the ordering for dependencies that matters in a jvm is the classpath order which is preserved by the walks, and which this destroyed. I can resurrect the sorting logic locally though since it sounds like this is not a bug but a feature!

  9. nit: this test's name could probably do with an update.

  10. Is there another test that covers the change in this api? How would you feel about bringing this one back and update it to the new behavior?

    1. The method was always bogus - cleaned up the publishing pipeline, pom template and the twitter --individual-plugins hack and this fell away.  The remaining straight=up single classifier or no dependencies are (still) tested in the IT.
  11. Thanks for this. I think that clearing this up will make it much easier to interact with JarDependency elements in the build graph. I've got a few comments, and I'd also like to see more tests. I'm surprised that a functional change like this didn't affect more unit tests, but maybe the movement towards this has covered them already.

  1. I just looked at the part in unpacked_libraries, leaving the rest to the other owners.

  2. Random comment: I know it was like this before, but it seems odd that classifier isn't incorporated into the key the way it is in the second loop. I guess that's because we currently don't allow specifying 'classifer' on Artifact() definition, but that may be an unintentional omission.

    1. Pants doesn't support classifier for these - which are JavaLibrary and ScalaLibrary internal dep targets.
    2. This is probably the confusing bit above: `jar = self._as_versioned_jar(internal_dep)` - The ivy.xml / pom modelling needs to see internal XXXLibrary targets as jars for the purposes of publishing dependencies, so the pushdb is used to turn an XXXLibrary with a provides clause into its jar representation - purely org, name, (provided by the provides definition) and rev (provided by the pushdb).
  1. lgtm for the jarpublish change

    1. That bit was pretty broken, a semi-large diff is coming to fix and add tests.
  1. Ship it!

  2. src/python/pants/backend/jvm/ (Diff revisions 1 - 3)

    Thanks for the bigger todo here. This makes a lot of sense.

  1. Thanks all - submitted @
Review request changed

Status: Closed (submitted)