Implement compile_classpath using UnionProducts

Review Request #1761 - Created Feb. 12, 2015 and submitted

Information
Stu Hood
pants
b25077a...
Reviewers
pants-reviews
benjyw, jsirois, nhoward_tw, patricklaw, tejal, zundel

This implements the first portion of the "Split the compile_classpath product" task from the classpath isolation design proposal.

  • Add UnionProduct to represent the classpath as a union of OrderedSets of per-target classpaths
  • Use add_for_targets and get_for_targets to implement a mashed up classpath for most tasks
  • Use the parsed ivy report to split the classpath during ivy_resolve
  • Adds a non-integration test for ivy_resolve

Because jvm_compile fetches the classpath on a per-partition basis, this causes a small difference in what is provided at compile time. But both junit_run and jvm_run are still using get_for_targets with the entire set of targets in the context, so the runime classpath should be equivalent to before this change.

https://github.com/pantsbuild/pants/pull/1092

Issues

  • 0
  • 8
  • 2
  • 10
Description From Last Updated
Benjy Weinberger
Stu Hood
Stu Hood
Patrick Lawson
Stu Hood
John Sirois
John Sirois
John Sirois
Eric Ayers
Benjy Weinberger
Eric Ayers
John Sirois
Stu Hood
Review request changed

Status: Closed (submitted)

Eric Ayers

Hi Stu, I am trying to integrate the latest pants into our repo and am getting a regression that git bisect traces back to this PR. I see you noted that there are some differences. In our case its a noticable one.

ivy report header @ 57fd7d0 (one before this commit)

Modules 125
Revisions 141 (0 searched searched, 0 downloaded downloaded, 16 evicted evicted, 0 errors error)
Artifacts 125 (0 downloaded, 0 failed)
Artifacts size 64543 kB (0 kB downloaded, 64543 kB in cache)

ivy report header @ 3b9451c (this RB's commit)

Modules 120
Revisions 135 (0 searched searched, 0 downloaded downloaded, 15 evicted evicted, 0 errors error)
Artifacts 120 (0 downloaded, 0 failed)
Artifacts size 62093 kB (0 kB downloaded, 62093 kB in cache)

We have a missing symbol in the build. I will keep digging to find out what exactly is different, but so far I see jooq-console as a difference.

PANTS_DEV=1 ./pants depmap <target>

still shows the dependency
...
internal-integration.jooq.src.main.java.lib
internal-3rdparty.org.jooq.jooq-console
org.jooq-jooq-console-3.1.0

It is a direct dependency of one of our libraries. It is not included at all in the latter report.

There is something odd about this library though. We include a later version of jooq in general (3.5.0), but this jooq library is fixed at an earlier version.

I'm sending you a tarball with the 2 reports and output of depmap (that output didn't change between versions of pants).

  1. Well, 3.1.0's pom references a parent and that parent has invalid xml: https://repo1.maven.org/maven2/org/jooq/jooq-parent/3.1.0/jooq-parent-3.1.0.pom
    Not sure how ivy handles a parse error or how that handling would act different under Stu's change, so this may not be related.
    Note though that 3.5.0's parent does not have the invalid XML issue: https://repo1.maven.org/maven2/org/jooq/jooq-parent/3.5.0/jooq-parent-3.5.0.pom
  2. I just diffed the 'ivy.xml' files. The one that is broken does not even request jooq-console. Wouldn't the xml parsing of which you speak com after the ivy.xml was constructed?

  3. I trimmed some irrelevant excludes out of the diff for brevity:

    diff ivy-works.xml ivy-broken.xml 
    14c14
    <   <info organisation="internal" module="732db7bbb323a3124e96e0593cbe57e6a1977971"
    ---
    >   <info organisation="internal" module="33c586f0ac787dfb0eccca7e227daa4a4a6e0fd7"
    725,771d724
    <     <dependency org="com.googlecode.flyway"
    <                 name="flyway-core"
    <                 rev="2.0.3"
    
    3547,3640d3499
    <     <dependency org="org.jooq"
    <                 name="jooq-console"
    <                 rev="3.1.0"
    
    <     <dependency org="org.jooq"
    <                 name="jooq-meta"
    <                 rev="3.5.0"
    
    
    <     <dependency org="org.postgresql"
    <                 name="postgresql"
    <                 rev="9.2-1004-jdbc41"
    
  4. "Wouldn't the xml parsing of which you speak com after the ivy.xml was constructed?" <- yes - it would only happen during a transitive resolve that touched the jooq-console node.
  5. Can you find a minimal set of jar_library definitions that reproduce the problem, and then I can work from there?

  6. This may be like a needle in a haystack. I tried a very simple mod to "HelloWorld" in the Pants repo that simply included multiple jar_libraries includeing jooq, jooq-console, jooq-meta and they all resovled properly through the ivy report:

    jvm_binary(name = 'main-bin',
      source = 'HelloMain.java',
      main = 'com.pants.examples.hello.rb1761.HelloMain',
      basename = 'hello-example',
      dependencies = [
        ':org.jooq.jooq',
        ':org.jooq.jooq-meta',
        ':org.jooq.jooq-console',
      ]
    )
    
    jar_library(name='org.jooq.jooq',
        jars=[
          jar(org='org.jooq',
               name='jooq',
               rev='3.5.0',),
        ],
    )
    jar_library(name='org.jooq.jooq-meta',
        jars=[
          jar(org='org.jooq',
               name='jooq-meta',
               rev='3.5.0',),
        ],
    )
    jar_library(name='org.jooq.jooq-console',
        jars=[
          jar(org='org.jooq',
               name='jooq-console',
               rev='3.1.0',),
        ],
    )
    

    of course, the example that is failing is much more complicated than that.

  7. I've now tested this change against a variety of large internal targets (4+ minutes each to build), but I wasn't able to reproduce the issue you mention. I did encounter a few cases of undeclared 3rdparty deps being caught at compile time though (due to the more precise compile_classpath.)

    If you can come up with a smaller reproduction, definitely let me know.

  8. We ceratinly don't have strict classpath hygeine. I am able to fix the problem by adding more deps here and there. I think I need to understand the problem better, but I don't undertand why they wouldn't already be in place. The libraries in question show up multiple times in the depmap output, but not once in the ivy.xml file. So even if they aren't declared on every target that uses them, they should still be pulled into ivy resolve especially right after a clean-all. Since they aren't even passed to the ivy resolve, I'm guessing that what Ivy consideres the transitive dependencies of a target is not the same as what's computed in depmap.

  9. I am unable to reproduce the ivy.xml difference today. Also, I think I've found a problem on our side that accounts for at least the majority of the problems. We were omitting a common dependency declared in parent pom.xml files when generating our BUILD files. I'm going to fix up these problems and I will follow up later.

  10. My apologies for the distractions. It looks like I am my own worst enemy :-(

    The real problem I first noticed I think was fixed by David's Context.targets() fix (https://rbcommons.com/s/twitter/r/1840/) and fixing missing some dependencies that were not even declared transitively. In the mean time I was attempting to fix a problem that wasn't really there and introduced the problem with the missing ivy.xml by trying to override 'relevant_targets' to be more broad in ivy_resolve. The bug I introduced was subtle. It only broke 4% of the targets in our repo.

Loading...