Enabling publishing in twitter commons

Review Request #60 — Created March 5, 2014 and submitted

tejal
commons
https://github.com/twitter/commons/issues/242
pants-reviews
jsirois, skumaran, travis, wickman
1. Enabling publishing of java and scala sources in one jar.
Ran Ci:
[tw-172-25-24-104 pants_twitter_commons]$ ./build-support/bin/ci.sh
.....
[== CI SUCCESS ==]
[tw-172-25-24-104 pants_twitter_commons]$ 

Ran: 
PANTS_DEV=1 ./pants.bootstrap goal clean-all publish src/scala/com/twitter/common/example/BUILD:jvm-run-example-lib -ldebug --no-publish-dryrun --publish-local=~tdesai/.m2  -ldebug 

I get:

    In the published scala-lob.pom, the dependencies for java_sources pants('src/java/com/twitter/common/examples/helloworld:lib') are added. (See attachment)
     The scala-lib.jar contains the scala sources and java_sources

[tw-172-25-24-104 pants_twitter_commons]$ unzip -l .pants.d/publish/com.twitter.common.example/scala_lib/scala_lib-0.0.1-SNAPSHOT.jarArchive:  .pants.d/publish/com.twitter.common.example/scala_lib/scala_lib-0.0.1-SNAPSHOT.jar

  Length     Date   Time    Name
 --------    ----   ----    ----
        0  01-01-80 00:00   com/
        0  01-01-80 00:00   com/twitter/
        0  01-01-80 00:00   com/twitter/common/
        0  01-01-80 00:00   com/twitter/common/example/
     1116  03-13-14 10:43   com/twitter/common/example/JvmRunExample$.class
      775  03-13-14 10:43   com/twitter/common/example/JvmRunExample.class

        0  01-01-80 00:00   com/twitter/common/examples/
        0  01-01-80 00:00   com/twitter/common/examples/helloworld/
      510  03-13-14 10:43   com/twitter/common/examples/helloworld/HelloWorld.class
 --------                   -------
     2401                   9 files
[tw-172-25-24-104 pants_twitter_commons]$
Loading file attachments...

  • 1
  • 0
  • 3
  • 1
  • 5
Description From Last Updated
Is there any way we can accomplish this change without creating a scala dependency on a library that is otherwise ... WI wickman
TE
TE
  1. 
      
  2. This is copied over from science master
    
    https://cgit.twitter.biz/science/tree/src/python/twitter/pants/targets/scala_library.py#86
  3. Checkstyle changes.
  4. If is instance scala lib, add java_sources targets to the jar.
  5. Added the fingerprinting logic.
  6. style check
  7. Moved over from jar_create since they also are needed in jar_publish tests
  8. 
      
TE
TE
TE
JS
  1. File is there: https://github.com/twitter/commons/blob/master/build-support/bin/ci.sh
    So you probably just have to rebase against master.
  2. 
      
JS
  1. 
      
  2. This change is additive - ScalaLibrary targets with non-empty java_sources get their pre-existing classfile and source jars augmented with the java_sources targets owned classfiles and sources.  I think the change also needs to eliminate the corresponding java_sources jars - ie: there should be exactly 1 classfile jar and 1 sources jar for each ScalaLibrary+[JavaLibrary] cyclic set of targets.
    
    On the JarPublish side this will prevent publication of cyclic poms and instead only the augmented ScalaLibrary jars and pom will be published.  So work is needed there to both:
    
    given: ScalaLibraryA, JavaLibraryA, JavaLibraryB where ScalaLibraryA.java_sources=[JavaLibraryA, JavaLibraryB]
    1.) only publish the augmented jars for ScalaLibraryA
    2.) publish an augmented pom for ScalaLibraryA that includes dependencies from JavaLibraryA & JavaLibraryB
  3. s/java_target/source/ ? shadowing java_target is confusing as is the variable name itself in this context
  4. Please uphold the doc discipline established in this file.
  5. This is too specific for the baseclass - push down into your test please.
    1. this is required by jar_publish and jar_create tests. Hence i wanted to put it in here.
    2. moved and made this better!
  6. tests/python/twitter/pants/tasks/BUILD (Diff revision 2)
     
     
    Please add a TODO to transition off mox to mock so its more clear why we have 2 mock libs here currently.  Bonus points if the TODO links a pantsbuild/pants issue to track.
  7. This is the TaskError raising line - we hope - best to prove this by lifting the prior lines out of the with context.
  8. 
      
TE
TE
TE
TE
TE
TE
  1. ping!!!!!
  2. 
      
TE
TE
TE
TE
TE
TE
TR
  1. 
      
  2. BUILD.twitter (Diff revision 7)
     
     
    This should go into a different file as its not Twitter-specific.
  3. Please eliminate this class and use pants.ini for configuration.
    1. The option is already defined for javadoc. However this was needed as the defaults were different for "jar" and "publish"
      Addressed it in rb #116.
      
      Looking for your feedback.
  4. This depends on another target that owns the sources, which means someone else could depend on src/java/com/twitter/common/examples/helloworld:lib directly. This could cause issues where the same sources are owned in multiple places.
    
    We should have source ownership in one place - how about owning java files directly? That likely means they should be moved into the scala tree alongside other stuff this target owns.
    1. Yes, it can be a solution, however i dont have enough background on why this isnt the case. 
      @jsirois knows this better.
      
    2. @Travis: 
      I talked to John over chat. He told me, java_sources was actually pointing to a glob before and it was changed to pants target in 2011 in this review
      https://reviewboard.twitter.biz/r/54361/
      
      The reasons were:
      1.) the rule for every other target in pants is it can only own sources at or below in the dir tree
      2.) projects will migrate to pants with pre-established dir structures, like the very prevalent maven structure, in which you have src/main/scala and src/main/java - this means we ought to support keeping where they living separate worlds.
      
      
  5. 
      
WI
  1. 
      
  2. Is there any way we can accomplish this change without creating a scala dependency on a library that is otherwise language-agnostic?
    
    we need to be sure to separate platform (JVM) concerns from language (Java/Scala) concerns whenever possible, and "we're already doing it" is not an acceptable rationale.  :-\
    1. yes. 2 approaches. 
      1. scala_library(name='lib', sources=globs('*.scala'), java_sources=globs('src/java/com/twitter/example/*.java'))
      
      That is add java_sources as a filesets instead of pointing to a JavaTarget.
      Pro: All the checks for ScalaLibrary will be removed. No False dependency of scala_lib(":lib") on java_lib target listed in java_sources. (your point)
      Con: If these sources are owned by some other java target then we will have 2 targets owning same source and also possibly publishing them.
      
      
      2. As Travis suggested, This can be done by adding java_sources directly in the scala tree.
      (But i am not sure why this is not a possibility)
      
      
    2. yes. 2 approaches. 
      1. scala_library(name='lib', sources=globs('*.scala'), java_sources=globs('src/java/com/twitter/example/*.java'))
      
      That is add java_sources as a filesets instead of pointing to a JavaTarget.
      Pro: All the checks for ScalaLibrary will be removed.
      Con: If these sources are owned by some other java target then we will have 2 targets owning same source and also possibly publishing them.
      
      
      2. As Travis suggested, This can be done by adding java_sources directly in the scala tree.
      
      
  3. 
      
TE
TE
TE
  1. 
      
  2. BUILD.twitter (Diff revision 7)
     
     
    Moved to another review. 
  3. Moved to another review 
  4. 
      
TE
TE
TE
  1. ping!
  2. 
      
JS
  1. Only 1 issue, marked as such.  This will need porting to pantsbuild/pants soon when the all-clear is sounded by Benjy.  You might want to note as a head's up on pants-devel that this review is ready to land on twitter/commons and that you're aware it will conflict with Patrick Lawson's work - although that's probably going straight into pantsbuild/pants.
  2. The sorting done above is actually key to prevent over-re-publishing, so sort the source filenames here)
  3. Please add a TODO to investigate moving source ownership knowledge into ScalaLibrary itself so that this extra branch and knowledge of ScalaLibrary is not needed here.
    1. i think we discussed over the review why moving the ownership is a bad idea:
      1. we advocate placing the build file as close as possible to the source tree.  Having scala/java classes in one subtree is confusing.
      2. If we refer to a glob pointing to the java_sources, then potentially, we are exposing to same set of sources owned in  two targets. The java_target defined in the java source tree.  
      
      Now, we can define something like this:
      Scala_library(name="scala",
        sources=[ globs(*.scala), pants('src/java/com/twitter/example:java_example').sources()]
      
      In this case, we are adding a new feature/capability to define sources.
      
  4. kill trailing ws
  5. This is subclass specific (jvm targets) to some degree so really belongs in a subclass or mixin.  Maybe just inline this in your test for now with a TODO to investigate re-use in other tests.
  6. 
      
TE
  1. 
      
  2. well it is used in scala_library and can java_library. 
    It can be used to define python_library or any other resources. 
    
    For e.g. tests/python/twitter/pants/tasks/test_listtargets.py has its own version of create_library.
    I can add a TODO to replace that with this create_target.
    
    
    But i feel this is the right place to put this method. Even tough its used specifically in my tests i see a potential of it being used elsewhere.
  3. 
      
TE
TE
TE
Review request changed

Status: Closed (submitted)

Change Summary:

ported over to pants/pantsbuild.
Loading...