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.

Sign up for GitHub

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

Jump to bottom

Support deploy_env in java_binary rule #6013

Closed
wants to merge 1 commit into from
Closed

Support deploy_env in java_binary rule #6013

wants to merge 1 commit into from

Conversation

hmemcpy
Copy link

@hmemcpy hmemcpy commented Aug 28, 2018

This adds support for deploy_env, an internal feature that supports transitive classpath subtraction to create deployable jars suited for different environments.
This PR cherrypicks @ulfjack's commit, as described in #1402, and will help with scenarios describe there, as well as #5856.

@jin jin added the team-Rules-Java Issues for Java rules label Aug 28, 2018
lberki
lberki reviewed Sep 3, 2018
View reviewed changes
Copy link
Contributor

@lberki lberki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a plan. Can I have a test case?

@hmemcpy
Copy link
Author

hmemcpy commented Sep 12, 2018

Sorry for the late response (holiday season here :))
Are there any utils in existing tests that verify the contents of written jars? We have some internally at Wix that crack open a jar and dump its contents, to run some asserts on that, but I wonder if there are tools in the repo I could use?
Or, any better ways to write a test for such a feature?

@ittaiz
Copy link
Member

ittaiz commented Oct 31, 2018

@lberki can you please comment on @hmemcpy's question?

@lberki
Copy link
Contributor

lberki commented Nov 6, 2018

Given what you want there is to verify that some classes are not in the deploy jar and on the classpath, I'd just do unzip -l and check that the runfiles manifest for the java_binary doesn't contain jars it should not.

@statik
Copy link

statik commented Jan 3, 2019

@hmemcpy I'm excited for this PR. Is there anything I can do to help with testing?

@ghost
Copy link

ghost commented Jan 7, 2019

Given what you want there is to verify that some classes are not in the deploy jar and on the classpath, I'd just do unzip -l and check that the runfiles manifest for the java_binary doesn't contain jars it should not.

I have a pending change which adds a test case in https://github.com/beasleyr-vmw/bazel/commit/095ec4543ff405f2e77f483877dcd094104bb5c5. I'm waiting for the CLA process to complete. In the meantime, would a Bazel developer mind reviewing the change? (Any chance we could get this into 0.22?)

@ittaiz
Copy link
Member

ittaiz commented Jan 8, 2019 via email

@ulfjack
Copy link
Contributor

ulfjack commented Jan 8, 2019

There isn't anything to review, really - I believe that's the code I posted which we're already using internally. The main obstacle is adding a test.

@ulfjack
Copy link
Contributor

ulfjack commented Jan 8, 2019

To clarify, unless someone raises a serious concern with this change that I'm not aware of at this time, I'm happy to sign off on this change and merge it.

@ittaiz
Copy link
Member

ittaiz commented Jan 8, 2019 via email

@lberki
Copy link
Contributor

lberki commented Jan 10, 2019

I certainly have no concerns. Merge this?

@lberki
Copy link
Contributor

lberki commented Jan 10, 2019

On second thought, we do have (not-open-sourced-yet) test cases internally.

@ittaiz
Copy link
Member

ittaiz commented Jan 10, 2019 via email

@lberki
Copy link
Contributor

lberki commented Jan 10, 2019

Already running :)

@bazel-io bazel-io closed this in a92347e Jan 10, 2019
@ittaiz ittaiz deleted the deploy_env branch January 16, 2019 12:39
@lberki
Copy link
Contributor

lberki commented Feb 12, 2019

Erm, sorry for the long time to review https://github.com/beasleyr-vmw/bazel/commit/095ec4543ff405f2e77f483877dcd094104bb5c5 -- it somehow ended up in a private inbox of mine. Looks good, as soon as the CLA is signed, I'll import it.

@ghost ghost mentioned this pull request Feb 12, 2019
Closed
@ghost
Copy link

ghost commented Feb 12, 2019

Erm, sorry for the long time to review beasleyr-vmw@095ec45 -- it somehow ended up in a private inbox of mine. Looks good, as soon as the CLA is signed, I'll import it.

The CLA process is complete, so I rebased and whipped up a formal PR. Thanks!

bazel-io pushed a commit that referenced this pull request Mar 1, 2019
@rbeasley @copybara-github
Add java_binary.deploy_env test case
5d3b74e 
This adds a test case for `java_binary.deploy_env` per @lberki's request in
issue #6013.

Testing Done:
- `bazel test //src/test/shell/bazel:bazel_java_test --test_arg=test_build_with_deploy_env`

Closes #7403.

PiperOrigin-RevId: 236291519
@lberki lberki mentioned this pull request Mar 14, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lberki lberki lberki left review comments

Assignees

@lberki lberki

@ulfjack ulfjack

Labels
cla: yes team-Rules-Java Issues for Java rules
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@hmemcpy @ittaiz @lberki @statik @ulfjack @jin @googlebot