Skip to content
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

Use zinc to create the context jar for zinc and rsc #7833

Merged
merged 3 commits into from Jun 5, 2019

Conversation

Projects
None yet
2 participants
@stuhood
Copy link
Member

commented Jun 2, 2019

Problem

We are not currently using zinc's -jar option to jar our outputs, which means that in order to use the --use-classpath-jars option (without adding a wrapper script) we would need to capture a snapshot of loose output class files, download it, zip it, and then re-upload... which would mostly defeat the purpose of that option.

Solution

After fixing a minor issue with zinc (#7834), use its -jar option to produce the context jar. javac_compile continues to use the existing python codepath for this.

In order to continue to inject resources created for annotation processors and compiler plugins, even when the compiler is itself producing the output jar, we now provide the extra_resources as a Digest that a compiler should mix in.

Result

--use-classpath-jars can be used with hermetic zinc and rsc.

@stuhood stuhood force-pushed the twitter:stuhood/use-zinc-jarring branch from f2a9920 to 82980a1 Jun 2, 2019

stuhood added a commit that referenced this pull request Jun 2, 2019

Sort entries in output zinc jars (#7834)
### Problem

The jars produced by the `zinc` `-jar` flag were not ending up sorted due to conversion to a `Map` before writing. Additionally, they contained a duplicate entry (that `findbugs` would choke on) as an artifact of how relativization was happening.

### Solution

Preserve the sorted order that is created by the `TreeSet` that we collect entries into, and do not create an entry for the "root" directory of the inputs.

### Result

Pants is able to consume the `-jar` option successfully in #7833.

@stuhood stuhood force-pushed the twitter:stuhood/use-zinc-jarring branch from 82980a1 to fb969b7 Jun 2, 2019

@stuhood stuhood marked this pull request as ready for review Jun 2, 2019

@stuhood stuhood force-pushed the twitter:stuhood/use-zinc-jarring branch 3 times, most recently from 8a3f6d0 to 54cdc44 Jun 3, 2019

@stuhood

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

I'm still expecting some test failures (in particular: I'm not 100% sure that annotation processors will work with zinc-hermetic), but this should be reviewable.

stuhood added some commits Jun 2, 2019

Move to providing extra_resources via a Digest that each jvm_compile …
…implementation either materializes or merges.
Use zinc to create the context jar for `zinc_compile` and `rsc_compil…
…e` (and move the existing python jar creation to `javac_compile`).

@stuhood stuhood force-pushed the twitter:stuhood/use-zinc-jarring branch from 54cdc44 to 5fe5cb4 Jun 4, 2019

@stuhood

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

I'm expecting this to be green on the next run: a review would be very welcome!

Commits are useful to review independently.

@cosmicexplorer
Copy link
Contributor

left a comment

I might rename this to "write extra resources in the JvmCompile base class" or something -- but I might also write what's there now. Thanks for making this change and removing more random v1 file writes!!

'resolver': 'coursier',
}
}
def test_hermetic_binary_with_3rdparty_dependencies(self):

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Jun 4, 2019

Contributor

It would be really incredibly super cool if we could simply move the _for_all_execution_environments decorator at

into this file (or a util/ subdir or something) and just add use_classpath_jars into it. Thinking about it now, that's almost definitely something we'd want to generate tests for, since each individual test right now takes a very long time to run. If we could make a TODO + issue link that'd be pretty easy to get to this week or later next.

This comment has been minimized.

Copy link
@stuhood

stuhood Jun 5, 2019

Author Member

See #7824: I think that following this week, we should try to collapse at least one permutation out.

if self.get_options().use_classpath_jars:
return fast_relpath(cc.jar_file.path, get_buildroot())
else:
return fast_relpath(cc.classes_dir.path, get_buildroot()) + '/**'

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Jun 4, 2019

Contributor
Suggested change
return fast_relpath(cc.classes_dir.path, get_buildroot()) + '/**'
return '{}/**'.format(fast_relpath(cc.classes_dir.path, get_buildroot()))

This comment has been minimized.

Copy link
@stuhood

stuhood Jun 5, 2019

Author Member

the irony of format is that it is seemingly more likely to result in getting b'my/filename'/**, since it isn't typesafe, heh.

@@ -41,7 +41,7 @@ python_tests(
':base_compile_integration_test',
],
tags={'integration'},
timeout=240,
timeout=360,

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Jun 4, 2019

Contributor

See comment above on generating tests vs looping within a single test.

@@ -489,7 +459,8 @@ def _compile_hermetic(self, jvm_options, ctx, classes_dir, zinc_args,
req = ExecuteProcessRequest(
argv=tuple(argv),
input_files=merged_input_digest,
output_directories=(classes_dir,),
output_files=(jar_file,) if self.get_options().use_classpath_jars else (),
output_directories=() if self.get_options().use_classpath_jars else (classes_dir,),

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Jun 4, 2019

Contributor

I would prefer putting these in the same if, something like:

if self.get_options().use_classpath_jars:
  output_files = (jar_file,)
  output_directories = ()
else:
  output_files = ()
  output_directories = (classes_dir,)
@@ -32,7 +32,9 @@
from pants.util.memo import memoized_method, memoized_property


_ZINC_COMPILER_VERSION = '0.0.9'
# TODO: To use this with the nailgun strategy, will need a publish of `rsc_2.12`: this will
# block landing this.

This comment has been minimized.

Copy link
@stuhood

stuhood Jun 4, 2019

Author Member

This can be removed now.

This comment has been minimized.

Copy link
@stuhood

stuhood Jun 5, 2019

Author Member

...but since this is green, I'm going to followup to remove it with the fix for #7850.

@stuhood

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

CI is green and I've validated this on our internal usecases: going to merge and then followup to apply the review comments as part of the fix for #7850.

@stuhood stuhood merged commit 5d44e7e into pantsbuild:master Jun 5, 2019

1 check passed

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

@stuhood stuhood deleted the twitter:stuhood/use-zinc-jarring branch Jun 5, 2019

@stuhood stuhood added this to the 1.17.x milestone Jun 5, 2019

stuhood added a commit that referenced this pull request Jun 5, 2019

Use zinc to create the context jar for zinc and rsc (#7833)
We are not currently using `zinc`'s `-jar` option to jar our outputs, which means that in order to use the `--use-classpath-jars` option (without adding a wrapper script) we would need to capture a snapshot of loose output class files, download it, zip it, and then re-upload... which would mostly defeat the purpose of that option.

After fixing a minor issue with `zinc` (#7834), use its `-jar` option to produce the context jar. `javac_compile` continues to use the existing python codepath for this.

In order to continue to inject resources created for annotation processors and compiler plugins, even when the compiler is itself producing the output jar, we now provide the extra_resources as a `Digest` that a compiler should mix in.

`--use-classpath-jars` can be used with `hermetic` `zinc` and `rsc`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.