Fix errors due to iterating over None-types in ivy resolve.

Review Request #3596 — Created March 22, 2016 and submitted

gmalmquist
pants
gmalmquist/dont-return-none-jar_dependencies_for_target
3073
pants-reviews
nhoward_tw, stuhood, zundel
IvyFetchResolveResult._jar_dependencies_for_target was returning
None instead of an empty iterable, which was breaking the for-loop
in IvyInfo.get_resolved_jars_for_coordinates.

This makes _jar_dependencies_for_target return an empty tuple
instead of None.

Manually confirmed that this fixes the problem in our internal repo.
CI confirmed that this didn't break any existing test-cases.

I haven't found a way to repro this in open-source yet (the artifact that was causing ivy to choke internally is an internal-only artifact that happens to be a tarball). I tried adding a test using an arbitrary tarball I found on maven central, but it didn't reproduce the original error.

CI passed here: https://travis-ci.org/pantsbuild/pants/builds/117770399

  1. I'd really like a test for this. Not because I don't think this fixes it but so that it doesn't breka gain. There have to be some non jar artifacts we could find in maven central.

    1. There are non jar artifacts in maven central. I tried it with http://search.maven.org/#artifactdetails%7Corg.crsh%7Ccrsh.distrib%7C1.3.0-beta2%7Cjar, but there error did not occur.

    2. I also found: http://search.maven.org/#artifactdetails%7Corg.apache.tomcat%7Ctomcat%7C9.0.0.M4%7Cpom

      may have to do with both not type jar and has a classifier?

    3. The artifacts that fail in our repo don't have classifiers

    4. I tried it with several more tarballs, including the apache tomcat artifact you linked to. They all work just fine with no changes to pants :-/

    5. You could write a unit test that creates a result with the property you're looking for.

  2. 
      
  1. Ship It!
  2. 
      
  1. Ship It!
  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

In 6d6c24446f94a79d35818f14400e80c525362782, Thanks Eric and Matt. Hopefully we can eventually discover a way to regression test this.

Loading...