Include classifier in JarDependency equality / hashing

Review Request #2029 — Created April 3, 2015 and submitted

nhoward_tw
pants
1366
391a4a8...
pants-reviews
ity, jinfeng, jsirois, zundel

https://rbcommons.com/s/twitter/r/1905 made it so that calculate_classpath included classifiers in the keys of a dict it was using, but there are a number of places where JarDependencys are put in sets or are compared for equality and those didn't take classifer into account. This changes JarDependency's comparison ops to use classifier. It also moves validation of artifact / classifier into the __init__ definition so that they happen during definition rather than deep in an ivy resolve.

Changing the operators was a little tricky because there's some pants ivy code that relies on IvyModuleRefs duck typing as JarDependencys. I'm not completely sure of the effect there because it's possible that the ModuleRefs have classifiers nested in the associated artifacts. If someone else has thoughts around that I'd appreciate some suggestions.

ran ci.sh locally, travis baking off of a PR

  • 0
  • 0
  • 2
  • 1
  • 3
Description From Last Updated
ST
  1. 
      
  2. Should this blow up if both an artifact and a classifier are specified?

    Really, it seems like we'd want separate factory functions for each of these cases... some combinations of arguments should be illegal.

    1. The way it works is that only specifying a classifier allows you to have other artifacts associated with the dependency. It's a bit weird to me, and I've got some ideas for fixing it.

      If instead of mapping JarDependency to ivy <dependency>, we considered them 1:1 with artifacts, we could clean this stuff up. I think that'd need to be a separate change though as that would touch a number of other things.

      All I'm doing here is moving the existing behavior into the initializer so the error makes more sense.

  3. Is this used often enough to not be a property?

    1. good question. I think it's only used in calculating classpaths from ivy utils, so it's not that expensive to recalculate.

    1. this is only used for __rep__ and depmap. We could certainly method-ify it.

  4. In the past, it was clear that the rev should be compared in the third position... might want to comment next to the coordinate field indicating that field-ordering drives ordering.

    1. I'm actually not sure what this is used for. Since it doesn't do a proper rev comparison, it doesn't sort org=org,name=name,rev!=rev cases correctly most likely.

  5. src/python/pants/backend/jvm/targets/jar_dependency.py (Diff revision 1)
     
     
     
     
     
     
     
     

    This depends on the ordering of inputs to eq... should those comparisons use a comparator instead? Or should they explicitly convert this to an IvyModuleRef?

    1. After a little thought, I came up with a different approach that works better.

  6. 
      
NH
ST
  1. shipit

  2. Can you also add an "integration" level test for JarDependency in tests/python/pants_test/backend/jvm/tasks/test_ivy_resolve.py that tests that two artifacts are actually fetched?

  3. 
      
IT
  1. thanks for the cleanup!

  2. 
      
NH
ST
  1. 
      
  2. src/python/pants/backend/jvm/ivy_utils.py (Diff revisions 2 - 3)
     
     
     
     
     

    This conditional looks like something that should exist in JarDependency... it's not clear why the second list should contain None.

    1. Moved, and commented.

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

    Should this be an assert that we found artifacts for all classifiers we requested? IE, that:

    ...
    actual_classifiers = set([a.classifier for a in self._artifacts_by_ref[unversioned_ref]])
    if actual_classifiers != set(valid_classifiers):
      raise TaskError(..)
    
    1. Hmm. We could do both. What I'm doing here is ensuring that when you have A and B where A depends on jar1;None and B depends on jar1;classifierName, that jar1;classifierName won't be pulled in for A, and jar1;None won't be pulled in for B even if A and B are resolved together. The test I added verifies this. I added it because after adding the assertions, I noticed that they didn't have the behavior I expected.

    2. I can't raise a TaskError here testing actual classifiers == valid classifiers because it causes problems with excludes.

  4. https://docs.python.org/2/library/unittest.html#unittest.TestCase.assertNotIn

  5. 
      
ZU
  1. Ship It!
  2. 
      
NH
NH
NH
NH
NH
Review request changed

Status: Closed (submitted)

Change Summary:

Submitted 63fc8edec927e417dbb8b75468d090d4d688c3f7

Loading...