In IvyImports, make a single call to ivy to map all jar files.

Review Request #1364 — Created Nov. 19, 2014 and discarded

zundel
pants
zundel/ivy-imports-map-jars-once
286, 809
pants-reviews
dturner-tw, ity, jsirois, patricklaw

If you have many targets with jars to map in, this is a significant performance increase.

Added a new unit test.
Ran this in our internal repo where we have a lot of uses of java_protobuf_library() using imports=.

CI running at: https://travis-ci.org/pantsbuild/pants/builds/41531811

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
ZU
  1. We should accept this patch for no reason other than it boosts our coverall number significantly!

    Coverage increased (+10.67%) when pulling 15dc1a7 on ericzundel:zundel/ivy-imports-map-jars-once into 3697878 on pantsbuild:master.

    I was wondering why. I guess there is a lot of ivy code we don't have unit tests for, but 10% sounds like an outsized increase?

    1. I've gotten similar messages after running CIs on changes that didn't add any tests at all. I think maybe the recent sharding has confused the coverage analysis.

  2. 
      
PA
  1. Ship It!

  2. 
      
DT
  1. 
      
  2. Why are we checking resolve_for here when we just checked it a few lines ago?

    1. umm, yeah, that didn't make any sense.

  3. 
      
IT
  1. lgtm!

    1. This seems to be the order that everyone uses:

      system imports

      external lib imports (twitter)

      pants project imports

    1. The way I have them seems to be the way everyone else sorts from textwrap import dedent in the tests that I spot checked: test_scrooge_gen, test_builder, test_build_file_address_mapper,py

  2. 
      
ZU
ZU
ZU
ZU
  1. 
      
  2. Althought I just submitted this, I just noticed that the 'target' parameter is just the last one in the list. it is used to create a directory and information for imports_map.

    I'm reverting it and will revise the change.

  3. 
      
ZU
Review request changed

Status: Discarded

Change Summary:

After looking into it, It will take a lot of work to replicate this with one call and directly replace what's being done. I'm going to focus on jar_source_set since that eliminates my needs for import in normal compilation.
Loading...