Pants publish to support publishing extra publish artifacts as individual artifacts with classifier attached.

Review Request #1889 — Created March 9, 2015 and submitted

tejal
pants
1f459b0...
pants-reviews
areitz, benjyw, ity, jsirois, zundel

Since last 2 weeks, i am trying to work around publishing idl only thrift jars.
The discussion started here https://groups.google.com/forum/#!search/Proposal$3A$20Make$20JarPublish$20task$20more$20configurable./pants-devel/S5l2_NXBtnw/Mf1pLj7P-kgJ

John, has more history around this but in short,
At twitter, old pants used to publish idl only thirft jars a separate artifact.
This artifact had its own pom and also has a classifier attached to it.

In contrast to this, OS pants supports extra publish parameter and publishes the idl thrift jars under the same artifact but with a different classifier.

We all agree OS pants is the right way to got about it.
However that would mean changing all internal consumption of thrift jars at twitter.
We are deep into migrating to new pants and making this switch right now is going to push is over the deadline. It also costs us maintaining the branch for longer till we rollout the new way of consumption.

Since we are the only one consuming publish extra plugins, this rb changes the publishing to old pants way.

I added integration tests to compare the contents of artifact pom file and ivy.xml file generated during publishing in the https://rbcommons.com/s/twitter/r/1879/

If you notice, these test case files have not been changed touched in this RB.
This does not break the normal publish flow nor does change the contents of generated pom and ivy files.

To describe the changes verbally
1. I have added classifier to all the method of the dependency writer
2. Instead of checking for product type 'jars', the publishing loops through product type jar and any other product types mentioned in the publish.publish_extras config.
3. It will barf with error 'No product mapping in {0} for {1}. ' only if none of the above products are found.
4. The publish will create individual artifacts for 'jars' and any other product mentioned in publish.publish_extras config. By individual artifact i mean, a separate pom, ivy.xml.

Please let me know how you feel about this.

Thanks
Tejal

yes.
Locally verified the extra published artifacts can be consumed internally.

New Travis url
https://travis-ci.org/pantsbuild/pants/builds/53863412

  • 0
  • 0
  • 4
  • 0
  • 4
Description From Last Updated
AR
  1. 
      
  2. Do you want to hard-code 'idl' here?

  3. 
      
IT
  1. Tejal and I had a quick chat - she is going to work on moving this split pom/ivy.xml behavior under a flag so we can preserve the default/correct publish behavior.

  2. 
      
TE
TE
TE
TE
TO
  1. 
      
  2. extra_confs changed from None to []. Is that intentional?

    Also, classifier is empty string in some places and None in others. Shouldn't it be consistent (and probably None to be more Python like)?

    1. The default classifier is ''
      https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/jvm/tasks/jar_publish.py#L647

      Hence i assigned it the default value ''.
      I was torn between propogating this default value to all method definitions or just assigning None for others.

    2. As a proxy for Patrick I'd say that setting extra_confs=[] is not good because it is setting a mutable default argument.

      def write(self, target, path, confs=None, extra_confs=None, classifier=''):
        ...
        extra_confs = extra_confs or []
      
  3. Same comment here about classifer about empty strings vs. None

  4. Is return True right here? If the options are set for individual plugins, do we even need the product_mapping that is generated above?

    It feels weird that we return True here and the method doesn't return anywhere else, unless I am missing something. (and that's entirely possible)

    1. Changed it ro return
      Return should suffice.

  5. 
      
BE
  1. I'm not familiar with the issues or the code any more, so I will resign from this review.

  2. 
      
AR
  1. 
      
  2. I'm not sure that it makes sense to have a CHANGELOG.txt for both the jar and the IDL-only jar, but it doesn't hurt anything to have this, either.

  3. 
      
ZU
  1. 
      
  2. I don't understand this line. Shouldn't it be:

    {{#classifier}}conf="{{classifier}}"{{/classifier}}
    

    if you want to exclude this line all together if classifier isn't set. If you just want the empty string, you don' tneed the {{# ...} {{/...}}, do you?

  3. 
      
TE
ZU
  1. Ship It!
  2. 
      
IT
  1. thanks for cleaning this up - looks much better. Please create a TODO issue to remove this when we deprecate this behavior.

  2. indent

  3. indent

  4. 
      
TE
TE
Review request changed

Status: Closed (submitted)

Loading...