Add check if jars exists before registering products

Review Request #1808 — Created Feb. 20, 2015 and submitted

tejal
pants
57c5f61...
pants-reviews
benjyw, ity, jsirois, stuhood, zundel

Commits:
Add check if jar exists before registering products and also move the check out of with context

Pants, registers a empty jar product when there were no products generated by the target.

I saw this happen for sure when a target contained an empty scala file.
compile task did not produce any classes for that target and hence a jar with zero length got created.
(Obviously the fix was the remove the empty scala file, but pants needs to be more robust to handle such cases)

The reason for this was in commit 570498ed51fee6a6558bfe3fae2bf63d641d26bd Benjy added a change to add empty products for a target. Hence, the condition https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/jvm/tasks/jar_task.py#L342 always returned true.
I added the method nonzero for python 2.x and bool for python 3.x in MultipleRootedProducts and RootedProducts to return true only if there were any products registered.
Added test case for this scenario.

We also saw this happen in idea --intransitive phase. The jar phase registered products for synthetic java_thrift_library target created from twitter internal task. We create 2 synthetic targets (scala and java) for one thrift_library and attach same sources.
At runtime, however only one jar product was created but two got registered.
We saw this error
{code}
IOError: [Errno 2] No such file or directory: u'/Users/bzhang/workspace/source/birdcage/.pants.d/jar/jar/.pants.d.gen.scrooge.java-finagle..pants.d.gen.idl-extract.3rdparty.jvm.com.twitter.ibis.deprecated-ibis-thrift-service-java.3rdparty.jvm.com.twitter.ibis.deprecated-ibis-thrift-service-java.jar'

IOError: [Errno 2] No such file or directory: u'/Users/tdesai/projects/source2/birdcage/.pants.d/jar/jar/ibis.ibis-executor.src.test.java.java.jar'
{code}

I am happy to hear any other potential fixes.

yes:
https://travis-ci.org/pantsbuild/pants/builds/51559910

  • 0
  • 0
  • 1
  • 2
  • 3
Description From Last Updated
TE
JS
  1. 
      
  2. This certainly needs a test and a comment.
    
    That said - JarBuilder.add_target returns a list of targets that actually contributed files to the jar.  So I'm not understanding how a non-empty list of these targets can every be coupled with no jar write.  Should the check instead be?:
    ```python
    contributing_targets = self._jar_builder.add_target(jarfile, target)
    if target in contributing_targets:
      self.context.products.get('jars').add(target, self.workdir).append(jar_name)
    ```
    1. The JarCreateTest does test the normal workflow.

      Re: JarBuilder.add_target, let me try to reproduce this and see if the "target in contributing_target" catches this condition.

    2. Its the non-normal workflow I'm interested in a test for.  This is clearly a subtle failure mode and its exactly the subtle ones that are best documented and regression-proofed.
    3. +1 for adding a test for this case.

  3. 
      
TE
  1. 
      
  2. The JarCreateTest does test the normal workflow.

    Re: JarBuilder.add_target, let me try to reproduce this and see if the "target in contributing_target" catches this condition.

  3. 
      
TE
ZU
  1. 
      
  2. src/python/pants/goal/products.py (Diff revision 2)
     
     

    Stu just added UnionProducts. Do you need to override __bool__ and __nonzero__ for that variant as well?

  3. 
      
ST
  1. Given, pants does not disallow same sources owned by multiple
    targets, having this check in place sounds reasonable.
    Would be good to add a comment/TODO to this effect (in a few spots), because I think that we absolutely want this to be enforced at some point.

    1. Hm, markdown fail.

    2. @stuhood, My analysis was partially right.
      I saw this happen in Science pants. Could not reporduce in pants_internal, multiple targets owning same source files produced one non empty jar.

  2. 
      
TE
TE
JS
  1. 
      
  2. This is worth a comment I think.  It would be good to explain briefly why the target added may not have been returned from add_target.  Perhaps:
    # Skip adding a product when this target did not contribute files to the jar.
  3. tests/python/pants_test/base_test.py (Diff revision 4)
     
     
    This needs a more specific name at the least and probably a doc comment - it only works for MultipleRootedProducts data and that's not currently obvious at all unless you read the code.
  4. tests/python/pants_test/base_test.py (Diff revision 4)
     
     
    Similar complaint - this is for old-style non-"data" products, but that's only revealed by reading the code.
    1. complaint stands - just dropped the opened issue - did not intend to mark "issue" here.
  5. I'm not a fan of adding falsy support to products just to support these 3 tests.  How about just spell out the checks here instead?  That will make for an arguably clearer set of checks anyhow.
  6. 
      
TE
TE
TE
TE
TE
Review request changed

Status: Closed (submitted)

TE
  1. Follow up code changed here -> https://github.com/pantsbuild/pants/pull/1173/files

  2. 
      
Loading...