Please add Patrick/Mateo to this review, as they have different packaging infrastructure, and would be interested.
expose products for jvm bundle create and python binary create tasks
Review Request #3959 — Created May 31, 2016 and discarded
|mateor, patricklaw, peiyu, stuhood|
bundle.jvm currently does not expose a product_type for any archives it creates (only the loose bundle directory). And binary.python-binary-create currently does not expose any products at all. We would like these tasks to expose products, in order to be able to consume those products in downstream (internal) tasks.
In particular, JVM apps (at least at Twitter) are generally published as archived (zip'd/tar'd) bundles. And python apps are deployed directly as pexes.
1. Expose deployable_archive product in jvm bundle_create task (arhives, including zip, tar, tar.gz, tar.bz2).
2. Expose same deployable_archive product in python_binary_create task (pexes).
3. Above products live in .pants.d, and they have symlinks created in dist dir.
4. Modify integration and unit test cases to accommodate changes in (3).
5. Add test cases.
|Can we change the product name so that jvm and python do not share a product? We have an open ...||mateor|
|As with the change to the go code, this invalidation block should be as tiny as possible. The body of ...||stuhood|
|This comment no longer applies so it should be amended or deleted.||mateor|
|I am finding the if statements on the archive to be confusing. There are checks for archiver, archivepath, app.archive and ...||mateor|
This case continues to get more confusing. IMO, we should see what people would think about deprecating this option.
Relatedly: products should always live under
.pants.d, so it might be a good idea to put the product there with a known-unique name (ie, with
use_basename_prefix=False). Then, the name used under
distwould be independent, and could perhaps just be a symlink to the copy under
I think that combining those changes would mean that we always bundle
self.context.targets()using their target ids under
.pants.dand use that as the product, but we additionally symlink the
distunder their basenames.
I am very glad to see this done! This ship it is contigent on putting the products into pants.d
Can we change the product name so that jvm and python do not share a product? We have an open ticket to continue decoupling those backends and I would prefer not to backslide further. If you have something downstream that needs to consume both than I think it should just require the two distinct products.
jvm_deployablesor something like it?
=stu with the deprecation. I would honestly prefer to deprecate than continue to add code paths. Another option would be to bite the bullet and make the JvmApp and JvmBinary bundling into separate tasks with a common superclass.
Same comment as above, can this be explicitly a python product?
In fact, can it be explicitly a 'pex' product? We do not use pex but we do have python bundles. Kris Wilson and patrick have discussed a generic python resolver that is pluggable between pex and vex (our internal bundle) but since this is currently coupled to pex it would be best to make that clear in the product name.
Revision 2 (+248 -124)
The structure looks good to me... just want to see the
invalidatedblocks tightened up before shipping.
As with the change to the
gocode, this invalidation block should be as tiny as possible. The body of the block should basically just be per-target decision to either build the bundle, or use the bundle that is already built, and it should use split out methods to do each thing.
Also, this block is not currently looking at which
vts are valid... for any
vt.valid == True, the archive should already exist.
See https://github.com/pantsbuild/pants/blob/master/contrib/scrooge/src/python/pants/contrib/scrooge/tasks/thrift_linter.py#L99-L114 for a very good example of how to do the
So you mentioned in the comments that you want to have the
jvm_archivesonly be added for JvmApp. You'll have to be a bit careful to make sure that the product is always populated - as it stands the only time the
deployable_archivesproduct is populated is when the jvm_app has an archive defined in the payload.
archiver(when JvmApp has a defined archive-type) must be True in order to populate the
archivepathmust both be True in order for the archivesymlink to be created.
I am honestly unsure how much of this is by design - I have to confess to being a bit lost as to what is happening there. A request for some extra clarity is below.
This comment no longer applies so it should be amended or deleted.
I am finding the if statements on the archive to be confusing. There are checks for
app.archiveand on line 146,
That last check is only passing because we import the pants.fs.archive - it should always be true. If it was meant to be
archiverit is still unneeded since
archivepathbeing True is coupled to
I believe that was missed because there are so many variables around
archive- can you coalesce those checks to be
if app.archiveor something like it? You might not be able to - I haven't tried myself. But this is hard to scan as is.