Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

java_library: provide a way to specify non transitive dependencies #68

Closed
wants to merge 1 commit into from
Closed

java_library: provide a way to specify non transitive dependencies #68

wants to merge 1 commit into from

Conversation

davido
Copy link
Contributor

@davido davido commented Nov 21, 2013

Add compile_deps parameter to java_library function to specify the non
transitive dependencies. Because of the way how dependency graph is
implemented the deps parameter must still list all dependencies:

  java_binary(
    name = 'plugin',
    manifest_file = 'resources/MANIFEST.MF',
    deps = ['//:plugin-library'],
  )

  java_library(
    name = 'plugin-library',
    srcs = glob(...),
    deps = ['//lib:plugin-api'],
    compile_deps = ['//lib:plugin-api'],
  )

JAR file plugin.jar doesn't include the content of plugin-api.

Test Plan: https://github.com/davido/buck_test

fixes: #63

Add compile_deps parameter to java_library function to specify the non
transitive dependencies. Because of the way how dependency graph is
implemented the deps parameter must still list all dependencies:

  java_binary(
    name = 'plugin',
    manifest_file = 'resources/MANIFEST.MF',
    deps = ['//:plugin-library'],
  )

  java_library(
    name = 'plugin-library',
    srcs = glob(...),
    deps = ['//lib:plugin-api'],
    compile_deps = ['//lib:plugin-api'],
  )

JAR file plugin.jar doesn't include the content of plugin-api.

Test Plan: https://github.com/davido/buck_test

fixes: #63
@LegNeato
Copy link
Contributor

LegNeato commented Dec 3, 2013

Can you check on the Travis failure?

@rowillia
Copy link
Contributor

Hey @davido, thanks for the Pull. We are actually already looking at something similar to this, but we want to generalize it a bit more. For example, if a java_library depends on a ndk_library, the ndk_library doesn't actually need to finish building in order for the java_library to start building since the ndk_library is actually a packaging dep, not a compile time dep (e.g. any android_binary that has the java_library in it's deps needs to also have the ndk_library)

Also, I think that exported_deps does what you want in this case.

@oconnor663
Copy link
Contributor

This might be the wrong place for this discussion, but would it be a better abstraction if the java_library was allowed to declare its own packaging deps, so that every android_binary that included it didn't need to duplicate the same ndk_library dep or whatever?

@davido
Copy link
Contributor Author

davido commented Dec 17, 2013

@rowillia,

@oconnor663 makes a good point: so either extend a java_library to accept packaging_deps or have a android_library that accepts this parameter.

To stay with common src <- java_library <- java_binary chain problem, whith compile (!) time dependencies:

every one needs to express for compile time dependencies either they are transitive or not. I fail to see how exported_deps solves that problem in current master.

@oconnor663
Copy link
Contributor

I should be clear that I don't fully understand the context of this pull request. I was just thinking in the abstract :)

@davido
Copy link
Contributor Author

davido commented Dec 17, 2013

@oconnor663

I don't fully understand the context of this pull request

Why? How can i improve the description and the reproducer? You have two java_library rules: one with compile_deps param produces plugin.jar with only one file, where the original java_library produces the artifact with that file + commons-io included.

@ghost ghost assigned natthu Jan 22, 2014
@dreiss dreiss closed this Jan 22, 2014
@dreiss dreiss reopened this Jan 22, 2014
@shs96c
Copy link
Contributor

shs96c commented Apr 24, 2014

To clarify, you're asking to have a set of deps that need to be built before you can start compiling, and a second set of dependencies that need to be completed before packaging of the dependent java_binary can start?

@davido
Copy link
Contributor Author

davido commented Apr 24, 2014

@shs96c Nope. I am asking for compile_deps only. Consider core+plugin pattern. Gerrit exposes plugin-api.jar to create plugins. Obviously the whole plugin-api stuff is in Gerrit core / war file.

By compiling plugin.jar it is important to be able to express non transitive dependency:

Example:

Plugin foo has Foo.java and depends on 10 MB big gerrit-api.jar

In plugin-foo.jar artifact we need one file: Foo.class + MANIFEST:

 java_binary(
    name = 'plugin',
    manifest_file = 'resources/MANIFEST.MF',
    deps = ['//:plugin-library'],
  )

  java_library(
    name = 'plugin-library',
    srcs = glob(...),
    compile_deps = ['//lib:plugin-api'],
  )

We cannot do it with vanilla Buck today and were forced to create java_library2 [1].

Because if we do this:

 java_binary(
    name = 'plugin',
    manifest_file = 'resources/MANIFEST.MF',
    deps = ['//:plugin-library'],
  )

  java_library(
    name = 'plugin-library',
    srcs = glob(...),
   deps = ['//lib:plugin-api'],
  )

We would get in plugin-foo.jar 10 MB from the gerrit-plugin + one Foo.class file from the plugin.

[1] https://gerrit.googlesource.com/bucklets/+/master/java_library2.bucklet

@shs96c
Copy link
Contributor

shs96c commented Apr 25, 2014

OK. Understanding you now. How about a general "provided_deps" parameter for java rules?

@spearce
Copy link
Contributor

spearce commented Apr 25, 2014

On Fri, Apr 25, 2014 at 7:50 AM, Simon Stewart notifications@github.com wrote:

OK. Understanding you now. How about a general "provided_deps" parameter for java rules?

provided_deps would work. :-)

@shs96c
Copy link
Contributor

shs96c commented May 2, 2014

I'm working on a fix for this now.

oconnor663 pushed a commit that referenced this pull request May 2, 2014
Summary:
This is useful for the case where a java_library will be running in a host container
that will supply a set of dependencies (such as a JEE server) that are needed in order
to compile the classes.

Addresse github issue 68: #68

Test Plan:
buck test --all
Added a test to ensure that a java_binary does not include provided_deps
@shs96c
Copy link
Contributor

shs96c commented May 2, 2014

This was implemented in cea4e34

@shs96c shs96c closed this May 2, 2014
@spearce
Copy link
Contributor

spearce commented May 2, 2014

I am having trouble using this:

  TypeError: java_library() got an unexpected keyword argument 'provided_deps'

This is my rule:

java_library(
  name = 'init-base',
  srcs = INIT_BASE_SRCS,
  resources = INIT_BASE_RSRCS,
  deps = [
    ':init-api',
    ...  ],
  provided_deps = ['//gerrit-launcher:launcher'],
  visibility = [
    '//gerrit-war:',
    '//gerrit-acceptance-tests/...',
  ],
)

@spearce
Copy link
Contributor

spearce commented May 2, 2014

Never mind, I am an idiot, I forgot to recompile Buck.

@shs96c
Copy link
Contributor

shs96c commented May 12, 2014

Out of curiosity, is the provided_deps parameter meeting your needs?

Simon

On Fri, May 2, 2014 at 10:16 PM, Shawn O. Pearce
notifications@github.comwrote:

Never mind, I am an idiot, I forgot to recompile Buck.


Reply to this email directly or view it on GitHubhttps://github.com//pull/68#issuecomment-42079919
.

@davido
Copy link
Contributor Author

davido commented May 12, 2014

@shs96c Thank you so much for fixing it: works like a charm! We have removed our work around in Gerrit Code Review [1] itself and in bucklets buck library [2] that we are using for other projects in Gerrit ecosystem [3].

[1] https://gerrit-review.googlesource.com/56780
[2] https://gerrit-review.googlesource.com/56823
[3] https://gerrit-review.googlesource.com/56824

@shs96c
Copy link
Contributor

shs96c commented May 13, 2014

Great news! Thanks for letting me know :)

Simon

On Mon, May 12, 2014 at 9:11 PM, David Ostrovsky
notifications@github.comwrote:

@shs96c https://github.com/shs96c Thank you so much for fixing it:
works like a charm! We have removed our work around in Gerrit Code Review
[1] itself and in bucklets buck library [2] that we are using for other
projects in Gerrit ecosystem [3].

[1] https://gerrit-review.googlesource.com/56780
[2] https://gerrit-review.googlesource.com/56823
[3] https://gerrit-review.googlesource.com/56824


Reply to this email directly or view it on GitHubhttps://github.com//pull/68#issuecomment-42881636
.

davido added a commit to davido/bucklets that referenced this pull request Mar 21, 2015
In [1] provided_deps parameter was added to java_library() rule making
our custom java_library2() unnecessary. This upstream change addresses
github issue #68 [2].

[1] facebook/buck@cea4e34
[2] facebook/buck#68

Change-Id: I51c5385ba3e4c73f209ed9213272377d53806e22
@facebook-github-bot
Copy link
Contributor

@davido updated the pull request.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

java_library: provide a way to specify non transitive dependencies
9 participants