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

Add extra_jvm_options to jvm_binary targets #6310

Merged
merged 7 commits into from Sep 13, 2018

Conversation

Projects
None yet
3 participants
@blorente
Copy link
Contributor

blorente commented Aug 6, 2018

Problem

Targets could not include custom jvm options to be passed to the jvm_run task.
This creates the need of wrapper scripts around calls to ./pants run ...
Closes issue: #5868

Solution

The jvm_binary.py target now accepts extra_jvm_options as a parameter.
The jvm_run.py task now injects those options into the run command.

Result

Users can now specify extra_jvm_options for jvm_binary targets.

@stuhood

stuhood approved these changes Aug 6, 2018

Copy link
Member

stuhood left a comment

Thanks!

@@ -335,6 +336,8 @@ def __init__(self,
(aka monolithic aka fat) binary jar. The order of the rules matters: the first rule which
matches a fully-qualified class name is used to shade it. See shading_relocate(),
shading_exclude(), shading_relocate_package(), and shading_exclude_package().
:param list extra_jvm_options: A list of key value pairs of jvm options to use when running the

This comment has been minimized.

@stuhood

stuhood Aug 6, 2018

Member

Probably clearer not to refer to "key-value pairs"... sometimes they will be key-value pairs, but saying that makes it sound like it's not a collection of "literal options to be passed to the JVM". The example is good, but it might also be good to include an -Xmx4g option, to make it clear that any JVM option is supported.

This comment has been minimized.

@blorente

blorente Aug 6, 2018

Contributor

I agree with adding the plain flag case to the test project, but for the comment wording I was following the example set here: https://github.com/blorente/pants/blob/e5a5e5ed40ae7312d523207c4ddd888909c6ff8c/src/python/pants/backend/jvm/targets/junit_tests.py#L54

I agree that the nomenclature is misleading, I'll change both places.

with self.setup_cmdline_run(extra_jvm_options=options) as cmdline:
for option in options:
self.assertTrue(option in cmdline)

This comment has been minimized.

@stuhood

stuhood Aug 6, 2018

Member

This will likely fail to lint due to the extra line here.

Consider installing the commit hooks as described here: https://www.pantsbuild.org/howto_contribute.html#getting-pants-source-code (after installing them, you can skip them whenever you like by running git commit ... -n). If you don't install, consider running build-support/bin/pre-commit.sh before pushing PRs.

This comment has been minimized.

@blorente

blorente Aug 6, 2018

Contributor

Thanks! I somehow didn't think to ask myself where the hooks were, but I'm glad we have them, thanks for the reminder.

@stuhood stuhood requested a review from illicitonion Aug 6, 2018

@blorente

This comment has been minimized.

Copy link
Contributor

blorente commented Aug 6, 2018

I still need to make some changes, such as what Stu suggested and running the CI checks. It might be worth waiting a bit (later today or tomorrow) before reviewing it again.

@illicitonion
Copy link
Contributor

illicitonion left a comment

Looks great, thanks!

options = ['-Dexample.property1=1', '-Dexample.property2=1']
with self.setup_cmdline_run(extra_jvm_options=options) as cmdline:
for option in options:
self.assertTrue(option in cmdline)

This comment has been minimized.

@illicitonion

illicitonion Aug 7, 2018

Contributor

self.assertIn(option, cmdline) will give you a better failure message :)

@blorente blorente force-pushed the blorente:blorente/add-extra-jvm-options branch 2 times, most recently from beedcc4 to 38768f8 Aug 14, 2018

@@ -80,6 +80,10 @@ def execute(self):
else:
binary = target

# Some targets will not have extra_jvm_options in their payload,

This comment has been minimized.

@stuhood

stuhood Aug 28, 2018

Member

Does that mean that this needs a default value? If it's None, adding it to self.jvm_options below will fail.

This comment has been minimized.

This comment has been minimized.

@stuhood

stuhood Aug 28, 2018

Member

If it's guaranteed to be in the payload, then the comment is inaccurate I think? Not a big deal.

This comment has been minimized.

@blorente

blorente Aug 29, 2018

Contributor

I think I lost some context from when I wrote this, so I'll probably have to double check. However, I think that the problem was that jvm_run was being called with non-jvm_binary targets. If that was the case, the problem was that reflection could not find the field, and thus target.payload.extra_jvm_options raised an exception. However, get_field_value returns the default if the value is not there.

This probably makes the bit here unnecessary, but I would argue for keeping it.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Aug 28, 2018

Restarted the flaky shards.

@@ -357,6 +360,7 @@ def __init__(self,
'manifest_entries': FingerprintedField(ManifestEntries(manifest_entries)),
'main': PrimitiveField(main),
'shading_rules': PrimitiveField(shading_rules or ()),
'extra_jvm_options': PrimitiveField(list(extra_jvm_options or ())),

This comment has been minimized.

@blorente

blorente Aug 28, 2018

Contributor

Here

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Aug 28, 2018

I'm really not sure what is up with the travis failure, but it doesn't appear to be a flake. Would you mind rebasing to master?

@blorente blorente force-pushed the blorente:blorente/add-extra-jvm-options branch from 38768f8 to 4a6d980 Aug 29, 2018

@blorente

This comment has been minimized.

Copy link
Contributor

blorente commented Aug 29, 2018

I rebased, and now different tests break. I'll try to run ci locally to repro it.

@blorente

This comment has been minimized.

Copy link
Contributor

blorente commented Aug 29, 2018

This is weird... I rebased from master, but shard 42 keeps failing. The thing is, shard 42 doesn't exercise my integration test, and it also fails at downloading, not running.

The other failure has me totally baffled. Could it be that by changing test_jvm_run to include an extra named parameter, some test that depended on positional parameters failed?

@blorente blorente force-pushed the blorente:blorente/add-extra-jvm-options branch from 4a6d980 to 62c4483 Aug 29, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Aug 29, 2018

@blorente : Ah... I think I might see the issue: the error message sortof cryptically explains itself:

Exception message: Could not satisfy all requirements for ctypes_test==0.0.1+36df877074a4d30bde4598fb0bc0a7ea1f58c1ce.defaultfingerprintstrategy.1f6528e9e3c3.897870053801:
    ctypes_test==0.0.1+36df877074a4d30bde4598fb0bc0a7ea1f58c1ce.defaultfingerprintstrategy.1f6528e9e3c3.897870053801, ctypes_test==0.0.1+4c18d006fafdbe13c4343ef8eb51955733fe056b.defaultfingerprintstrategy.1de2eff99f85.897870053801

Means that one of the test projects is expecting a particular fingerprint, and is getting something else instead.

But most things under testprojects/**/* should be expected to fail when run this way, and to have other test coverage. So I think you should be ok to add one or both of these targets:

$ rg 'ctypes_test'
testprojects/src/python/python_distribution/ctypes_with_third_party/setup.py
12:  name='ctypes_test',

testprojects/src/python/python_distribution/ctypes/setup.py
12:  name='ctypes_test',

To https://github.com/pantsbuild/pants/blob/master/tests/python/pants_test/projects/test_testprojects_integration.py#L96

@blorente blorente force-pushed the blorente:blorente/add-extra-jvm-options branch from aee6275 to e517b23 Aug 30, 2018

@blorente

This comment has been minimized.

Copy link
Contributor

blorente commented Aug 30, 2018

@stuhood: It seems to still fail, even though it doesn't exercise those targets (travis). Shoud I also add testprojects/src/python/python_distribution/ctypes:ctypes?

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Sep 12, 2018

A lot has changed around integration testing of the python/ctypes work: would you mind rebasing this to try again? Sorry for the trouble!

@blorente blorente force-pushed the blorente:blorente/add-extra-jvm-options branch 3 times, most recently from ea41c1e to 12fb201 Sep 12, 2018

@blorente blorente force-pushed the blorente:blorente/add-extra-jvm-options branch from 12fb201 to b1df723 Sep 13, 2018

@illicitonion illicitonion merged commit 7146760 into pantsbuild:master Sep 13, 2018

1 check passed

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

@blorente blorente deleted the blorente:blorente/add-extra-jvm-options branch Oct 29, 2018

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