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

Fix extra_jvm_options for `jvm_app` targets #6572

Merged
merged 6 commits into from Oct 4, 2018

Conversation

Projects
None yet
3 participants
@blorente
Copy link
Contributor

blorente commented Sep 28, 2018

Problem

extra_jvm_options is a payload field passed to jvm_binary targets. jvm_app targets wrap jvm_binary targets. When calling jvm_run on jvm_apps, we tried to get extra_jvm_options from the app target, as opposed to the inner jvm_binary target. This led to a missing field error.

Solution

Take the extra_jvm_options from the binary target in all cases.

Result

Fixes #6560

blorente added some commits Sep 27, 2018

@blorente

This comment has been minimized.

Copy link
Contributor

blorente commented Sep 28, 2018

@blorente

This comment has been minimized.

Copy link
Contributor

blorente commented Sep 28, 2018

I'll fix the travis failures shortly

@illicitonion
Copy link
Contributor

illicitonion left a comment

Can we add a test that fails before this commit and passes after?

@@ -1,12 +1,18 @@
jvm_binary(
extra_jvm_options = ['-Dproperty.color=orange', '-Dproperty.size=2', '-DMyFlag', '-Xmx1m'],

This comment has been minimized.

@illicitonion

illicitonion Sep 28, 2018

Contributor

Are the changes to this file necessary? If not, revert?

This comment has been minimized.

@stuhood

stuhood Sep 28, 2018

Member

Changing something under testprojects should in theory cause them to be covered by the integration test shards (the "test_testprojects_*" tests).

But adding an explicit test near your old one would be preferable.

This comment has been minimized.

@blorente

blorente Oct 2, 2018

Contributor

Not all the changes are necessary, but the addition of target app_noopts at the end is. The other changes are there for style consistency, but they can be reverted.

@blorente

This comment has been minimized.

Copy link
Contributor

blorente commented Oct 2, 2018

I have added a target in the testproject that deals with this change, as well as a comment explaining better why payload.extra_jvm_options doesn't work.

I will add explicit tests under tests shortly.


from os.path import join
python_binary(
name=join('python_app'),

This comment has been minimized.

@illicitonion

illicitonion Oct 2, 2018

Contributor

Why join? This is a no-op?

This comment has been minimized.

@blorente

blorente Oct 2, 2018

Contributor

No, that is a leftover. I will remove it tomorrow.

@illicitonion
Copy link
Contributor

illicitonion left a comment

Thanks!

@stuhood

stuhood approved these changes Oct 4, 2018

Copy link
Member

stuhood left a comment

Will merge... feel free to follow up to address the comment if you'd like.

# This means that, when invoked with ./pants run, it will run regardless of whether
# the target is a jvm target.
# As a result, not all targets passed here will have good defaults for extra_jvm_options
extra_jvm_options = binary.payload.get_field_value("extra_jvm_options", [])

This comment has been minimized.

@stuhood

stuhood Oct 4, 2018

Member

Alternatively, you could move this line into the if isinstance(.., JvmBinary) block below?

@stuhood stuhood merged commit 24d20e7 into pantsbuild:master Oct 4, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment