Add transitive jar dependencies to depmap project info goal for intellij plugin.

Review Request #1047 — Created Sept. 18, 2014 and submitted

tejal
pants
d3d109b...
fkorotkov, jsirois, stuhood, zundel

Add transitive jar dependencies to depmap goal for Pants Intellij plugin

When creating projects using the plugin, the depmap showed direct dependencies for 3rdparty jar libraries.
This change add the trasitive dependency jars to the depmap output.
The transitive jars are required to correctly configure intellij modules.

yes.

Added integration tests but need to verify the contents.

PR created here:
https://github.com/pantsbuild/pants/pull/626
{Coverage increased (+0.04%) when pulling 09790ec on add_transitive_jar_deps into ad7d09d on master.}

Locally tested
./pants tests/python/pants_test/tasks:depmap_integration

  • 0
  • 0
  • 2
  • 1
  • 3
Description From Last Updated
FK
  1. This is awesome! You rock!

  2. Just to verify. jar_library can have several jars that's why we are getting jars for dep and not all dep.jar_dependencies. Right?

  3. 
      
TE
  1. 
      
  2. the get_transitive_jars() loops through all dep.jar_dependencies

    We are pulling all transitive jars.

  3. 
      
TE
ZU
  1. I agree we should add some sanity checks on the contents of the depmap JSON before letting it go. If nothing else, that the libraries have some known number of entries, spot check that a particular library is among them. For the 'targets' section, again, count the number of entries and spot check "testprojects/src/java/com/pants/testproject/unicode/main:main":

  2. I was thinking this could be a good candidate to test with a 'goldfile' style test where you check in a known good results file and then just make sure the results from calling 'depmap' match.

    Unfortunately, there are a lot of absolute paths in here like:

    "libraries": {
    "com.chuusai:shapeless_2.9.2:1.2.4": [
    "/Users/zundel/.ivy2/cache/com.chuusai/shapeless_2.9.2/jars/shapeless_2.9.2-1.2.4.jar"
    ],

        ...
    

    source_root : "/Users/zundel/Src/pants/testprojects/src/java/com/pants/testproject/unicode/main"

    1. i can use the pants_cache dir property and construct the path.

  3. 
      
TE
TE
TE
ZU
  1. 
      
  2. Add a test for this option == False?

  3. I don't really understand how goal depmap does anything to create 'modules'. It sends a bunch of information to the IDE, but it is up to the IDE to decide how to deal with that (Modules sounds like an ide concept.) Reading the code doesn't help me understand this much either. It looks like it just makes all targets in one BUILD file appear under one name (which may or may not actually appear in the BUILD file?) That doesn't seem to match the description.

    1. yes that is true. I think at twitter we have this issue of having same thirft files in multiple thrift dependencies. As a side effect, same thrift-gen scala files belonged to one or more modules.
      IDE does not deal well with same sources being part of multiple modules.

      Hence, we decided to add a flag to test adding all the code generated files in one module, some thing that ide does.

      Fedor is also working on the plugin side to have better handling of common sources.

  4. I got interested in this. Another alternative to this is to create a custom JSONEncoder as seen in:

    http://stackoverflow.com/questions/8230315/python-sets-are-not-json-serializable

    class SetEncoder(json.JSONEncoder):
    ... def default(self, obj):
    ... if isinstance(obj, set):
    ... return list(obj)
    ... return json.JSONEncoder.default(self, obj)
    ...
    json.dumps(set([1,2,3,4,5]), cls=SetEncoder)

    1. will take a look.

  5. 
      
ZU
  1. One more thought on adding in command line options like --single-module-per-codegen

    When updating the depmap JSON generation, I think it might be better to always add in extra information to the file to support new features in the IDE and not modify the way the JSON output is created, unless it is creating some kind of performance impediment. This is to prevent the kind of option collision we are currently experiencing in goal ide/idea where different options don't interact well with each other. For example, here it seems like the information we really need is to tag the target with the property 'target.is_codegen' . We could always output that kind of information and let the IDE decide how it wants to deal with it.

    1. ah ok. good point.

    2. yeh. let's just add a flag to each target if it's a generated target. we'll merge them while creating an IDE project.

  2. 
      
IT
  1. 
      
  2. these return statements look almost identical, extract function?

    1. Removed those.

  3. early exit? if not ivy_info, you can move initializing the orderedeset past the ivy_info check

  4. use .get(..) for map access throughout - the subscript access with throw when the key doesnt exist

  5. instead of chaining these asserts in one method -- take them out into their own methods, so you can pinpoint errors easily when any of these fail

  6. 
      
TE
TE
IT
  1. 
      
  2. lets move the boilerplate out to a setup method or something similar - there is a lot of repetition throughout this test.

  3. 
      
TE
  1. 
      
  2. This cant be setUp since, the test_depmap_without_resolve uses different parameters to the goal and also different test target.

  3. 
      
ZU
  1. Ship It!

  2. 
      
TE
Review request changed

Status: Closed (submitted)

JS
  1. 
      
  2. This test and test_depmap_without_resolve should be red flags - if depmap requires a resolve it should do this in prepare such that ./pants depmap --depmap-project-info ... just works. If the idea is that the only user of the --depmap-project-info is in fact the idea plugin and that plugin knows to call resolve (ie: your goal is to not impose resolve overhead on human users of depmap), that's a good sign that all the code in the --depmap-project-info branches should be factored out to its own IDEA-specific Task.

    Filed: https://github.com/pantsbuild/pants/issues/935

    1. Plugin calls depmap twice. First time wihtout resolve to get a project mapping so we can open the project quickly. And in background it calls depmap the second time with resolve and configures jars afterwards. It allows us to open a project and start indexing it in parallel with jar resolving.

    2. That's fine in the very specific context of use of the depmap goal by IDEA, but its extremely confusing to have this mixed into the depmap goal a user uses (and that a user would not know to call resolve + use the flag). For that reason I think it makes alot of sense to split out the behavior needed by the IDEA plugin to a seperate Task. That task can be installed without a description and not show up in goals help, etc...

    3. ... at least it confused Patrick and I for an hour or so and we're presumably power-users!

    4. Originally it was a separate goal :-) https://rbcommons.com/s/twitter/r/600/ check second review change after Ity's and Larry's comments.

      But I think it's better to have a separate goal for getting ide specific information about targets.

    5. It can be a separate task, however we still need to do ./pants goal resolve new_task <>,

      Lasttime i spoke with Patrik, the new scheduling based on product types will not support scheduling to depend on options.
      As Fedor mentioned, we first run a task to only get internal source dependencies (which is what depmap does).
      Then run resolve and consume resolve task products.

      Other option is replace this by two tasks, one which depends on resolve product types and one which does not.
      1. ./pants depmap_replacement
      2. ./pants resolve_depmap_replacement

    6. I think at the end of the day scheduling with product types will in fact have access to / need options to support recursive cases like building scrooge inline before using scrooge (in the case where you use scrooge and its hosted in the same repo - aka birdcage). In that case you need to have the option that specifies the tool spec.

      Even if not so, your idea of a pair of tasks makes sense.

  3. 
      
Loading...