New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow intransitive unpacking of jars. #5398

Merged
merged 2 commits into from Jan 29, 2018

Conversation

Projects
None yet
3 participants
@benjyw
Copy link
Contributor

benjyw commented Jan 27, 2018

This should probably have been the default to begin with: Before
this change, unpacking a source jar will also unpack all its
binary dependencies, leaving a bunch of useless .class files in
the unpack dir. However, for backwards compatibility, the default
is to continue to do so, but at least now there's an option not to.

@benjyw benjyw requested review from ericzundel and jsirois Jan 27, 2018

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Jan 27, 2018

This feels like it should be an option on the targets or else there should be two types of products, a transitive and an intransitive, so that downstream tasks can request which style they need. Wholesale changing the unpack behavior seems like it would break certain tasks; ie: this option could only be used in a repo that does not install or else trigger those tasks.

@classmethod
def register_options(cls, register):
super(UnpackJars, cls).register_options(register)
register('--with-dependencies', type=bool, default=True,

This comment has been minimized.

@jsirois

jsirois Jan 27, 2018

Member

... maybe the idea is this is advanced, and so you can install the task in a new goal that also includes the consumer (to the right in the goal list of tasks), and you could then use pants.ini to lock that installation of the task into intransitive?

This comment has been minimized.

@mateor

mateor Jan 28, 2018

Member

That Android code actually significantly overrode the UnpackJars logic, IIRC, because it didn't map well to the Android paradigm. UnpackJars unpacks and filters for every dependency (again, IIRC. I am AFK so going largely off memory), and didn't cache the unzipped result. The Android code unpacked to a workdir and wouldn't unpack again until the filter changed or the version changed.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Jan 27, 2018

I tried adding transitive=False to the jar()s in the underlying jar_library , but this leads to conflicts (and Ivy task errors) if the same dep is brought in intransitively by some other path (this was a real-world case, not hypothetical).

I'm actually 98% sure that this should have been the behavior all along. In practice, unpacked_jars are only used to represent remote sources, to be fetched and then compiled locally. However if those remote sources happen to have dependencies declared, then the unpacked dir will contain those dependencies' .class files, which is obviously not necessary or sensible. I would be extremely surprised if anyone is, or ever was, relying on the transitive behavior.

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Jan 27, 2018

OK, dug up the deleted android examples and these bear out that that plugin expects a transitive resolve and unpack. For protobuf, our other use case, I can find no examples. It seems worthwhile to me to get @mateor and @ericzundel or @cheister to comment on this one. At the very least it seems clear this should be an advanced option. I'm fine with that path if it does turn out there are no expectations of transitivity in-practice. Otherwise, it seems like providing two product types and reacting to what other tasks actually request is the right way to go.

@mateor

This comment has been minimized.

Copy link
Member

mateor commented Jan 28, 2018

So the Android jars did indeed require unpacking all way down.

We are having a bit of a similar conversation in the RpmBuilder PR. I wrote an internal wrapper around BinaryUtils that allows extending that namespacing scheme and using them as BUILD file addressable.

They are fetched under Pants bootstrapdir, and a task will symlinks under the workdir for targets to consume. I consider it to be pretty similar to how the python_requirements are fetched and stored, or Ivy and use it pretty widely internally. But when it was proposed to upstream, I did think that it should probably either go in core Pants or be exposed as a bad idea and deleted.

Anyway, @stuhood was suggesting we switch to UnpackJars instead, which doesn't really work for us. Most cases we do not want to unpack, and several of the fetched files are quite large and don't work as well per-target, or fetched every clean.

Anyway - it seems like maybe all these questions could get solved at once - it looks like that new fetcher would completely nap to @benjyw use case:

Here are the subsystem and symlinks task with Stu's comments inline

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Jan 28, 2018

Either way, adding this as a second product is not hard, so I may as well do that.

@mateor

This comment has been minimized.

Copy link
Member

mateor commented Jan 28, 2018

Yeah, I think that makes sense. I took a bit closer look (on my phone right now) and if you are consuming a jar, and so probably better off with UnpackJars so you get the free jar resolution.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Jan 28, 2018

OK, have completely revamped this. PTAL.

Open question is - do we think that DeferredSourcesMapper, currently the only consumer of these products, does in fact always want the intransitive product, or should that flag now move here? Or should it be a per-target option?

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Jan 28, 2018

Actually, now that I think of it, is this all overkill? Should it simply be a property of the unpacked_jars target?

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Jan 28, 2018

I'm definitely very happy with a target option.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Jan 28, 2018

Done. This is slightly redundant with intransitive=True on the jar() dependency in the jar_library referenced by the unpacked_jars. But that may be inevitable, as intransitive=True may lead to conflicting dependencies.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Jan 28, 2018

Best to review the diff of all commits as one, because it's significantly smaller than reviewing each commit (since each represents a completely different implementation of this...)

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Jan 28, 2018

It looks like a rebase may be needed anyway since there are unrelated lint changes in here.

@jsirois
Copy link
Member

jsirois left a comment

Assuming the rebase is forthcoming, the changes to these files LGTM:
src/python/pants/backend/jvm/targets/import_jars_mixin.py
src/python/pants/backend/jvm/targets/unpacked_jars.py
src/python/pants/backend/jvm/tasks/ivy_imports.py
src/python/pants/backend/jvm/tasks/unpack_jars.py
tests/python/pants_test/backend/jvm/tasks/test_unpack_jars.py

Allow intransitive unpacking of unpacked_jars targets.
Adds an `intransitive` property to the target.

Also improves testing of the unpack_jars task: Removes
an old test that really just tested fingerprinting and
invalidation. Replaces it with one that actually checks
task result output.

@benjyw benjyw force-pushed the benjyw:unpack_dependencies_option branch from 9e33bf7 to 5f61192 Jan 28, 2018

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Jan 28, 2018

Yup, earlier rebase error now fixed with a force-push. Thanks!

@benjyw benjyw changed the title Add an option to the UnpackJars task to not unpack dependencies. Allow intransitive unpacking of jars. Jan 28, 2018

@benjyw benjyw merged commit efbe887 into pantsbuild:master Jan 29, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@benjyw benjyw deleted the benjyw:unpack_dependencies_option branch Jan 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment