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

Support For implementation Dependency Configuration #242

Closed
dmdashenkov opened this issue Jun 23, 2018 · 18 comments
Closed

Support For implementation Dependency Configuration #242

dmdashenkov opened this issue Jun 23, 2018 · 18 comments
Assignees

Comments

@dmdashenkov
Copy link

dmdashenkov commented Jun 23, 2018

As the README says, it's required to use compile dependency configuration in order to link .proto dependencies. However, the configuration is deprecated now.

Could we expect support for the implementation and api (from the java-library plugin) for those purposes?

@zpencer
Copy link
Contributor

zpencer commented Jun 25, 2018

In our tests, I verified that changing compile to api or implementation still works.

I did notice that things like protobuf files("lib/protos.tar.gz") may cause problems in the new version of gradle. It allows users to add proto files directly into the configuration, and I suspect it just has the equivalent behavior of api. If so, we need a way to let people add proto files without exposing them downstream transitively.

@dmdashenkov
Copy link
Author

Thanks for the answer, @zpencer. Could you please link the test so I can compare it to my config and find out why does it work for you, but not for me?

@zhangkun83
Copy link
Collaborator

@zpencer Sounds like we need something like "implementationProtobuf" and "apiProtobuf".

@zpencer
Copy link
Contributor

zpencer commented Jun 25, 2018

@dmdashenkov try changing https://github.com/google/protobuf-gradle-plugin/blob/master/testProjectDependent/build.gradle#L16 to say implementation project(':testProject') and run this test "testProjectDependent should be successfully executed" for gradle 4.3. This works.

In my earlier statement I was actually modifying the wrong file, my mistake. When I change it to api here I am getting a general gradle error about the config > Could not find method api() for arguments ... . But the implementation at least works. What do you see on your project?

@dmdashenkov
Copy link
Author

In my case, I use the compile config for a module which contains both Java and Protobuf definitions.
When changing to implementation, I get an error from protoc which says that an import (the file from the linked dependency) is not found.

Interestingly enough, using implementation for a project dependency (implementation project(':proj')) works just fine. However, when trying to use an external artifact (implementation "my.package:artifact:version") the described issue happens.

The visible difference when building my project in those two configurations is in the build/extracted-include-protos directory. When using compile, it contains all the used .proto files and when using implementation - only the standard Protobuf definitions (those under com.google package).

I'll proceed with my investigation and inform you if I find some more info.

@jd3nn1s
Copy link

jd3nn1s commented Oct 1, 2018

implementation does not work for me even with a project dependency, only compile does. However, switching from compile to implementation and building does not surface the error of Import X was not found or had errors. You must do a clean build to surface the error.

This is a tough issue for us because it means that we have to either use protobuf which forces additional generation, or leak transitive dependencies.

I am using gradle 4.10.2.

@dmdashenkov
Copy link
Author

@zpencer, I experience the same behavior as @jd3nn1s. Is there a chance that someone will take a look at this issue soon?

P.S. Sorry for making such a long pause in the conversation.

@zpencer
Copy link
Contributor

zpencer commented Nov 13, 2018

@zhangkun83 perhaps you can take a look at exposing the new style dependency configuration.

@dmdashenkov
Copy link
Author

Hi, guys. Are there any updates on this issue?

@jd3nn1s
Copy link

jd3nn1s commented Mar 30, 2019

If someone can provide a little direction on how to implement this I am willing to take a look at doing it.

@dmdashenkov
Copy link
Author

Seems like this is what is happening:

  • GenerateProtoTask knows nothing about configurations, it simply picks up whatever ProtobufExtract produces;
  • ProtobufExtract takes its orders from ProtobufPlugin directly; ProtobufPlugin specifies the configuration to the ProtobufExtract (as input files);
  • here ProtobufPlugin defaults to the compile configuration if none is specified;
  • if we look closely, no configuration is ever specified to the setupExtractIncludeProtosTask(..) method except for the case with Android builds.

It is really up to you, but I'd expect the plugin to use compileClasspath configuration for this purpose. Or even better, the compileClasspath property of the target SourceSet.

@dmdashenkov
Copy link
Author

@jd3nn1s, please let me know if you have any questions on my comment.

@dmdashenkov
Copy link
Author

@jd3nn1s, is there any progress? If you didn't have a chance to try yet, maybe I could give it a try sometime soon.

@voidzcy
Copy link
Collaborator

voidzcy commented May 20, 2019

@dmdashenkov
Does changing "compile" to "compileClasspath" work for you?
I tried the following, and get the same observation, implementation works fine. I have not tried with api as we don't have tests using java-library plugin, but I am suspecting using "compileClasspath" should reasonably solve all the problems. Can you provide a small example for thing that breaks for you?

@dmdashenkov try changing https://github.com/google/protobuf-gradle-plugin/blob/master/testProjectDependent/build.gradle#L16 to say implementation project(':testProject') and run this test "testProjectDependent should be successfully executed" for gradle 4.3. This works.

In my earlier statement I was actually modifying the wrong file, my mistake. When I change it to api here I am getting a general gradle error about the config > Could not find method api() for arguments ... . But the implementation at least works. What do you see on your project?

@dmdashenkov
Copy link
Author

Sorry for a long silence. I do not seem to receive some notifications from open source repos.

@voidzcy, yes, this should work. Moreover, according to the build warnings in Gradle 6.0,

The compile configuration has been deprecated for dependency declaration. This will fail with an error in Gradle 7.0. Please use the implementation or api configuration instead.

Tip: run a build with --warning-mode all to see this.

You are right, compileClasspath should cover both implementation and api (as well as the deprecated compile).

@voidzcy
Copy link
Collaborator

voidzcy commented Dec 12, 2019

I recently found this issue is actually related to #363.

I am able to reproduce what @dmdashenkov says exactly:

In my case, I use the compile config for a module which contains both Java and Protobuf definitions.
When changing to implementation, I get an error from protoc which says that an import (the file from the linked dependency) is not found.

Not only for an external artifact (implementation "my.package:artifact:version"), this issue also occurs for a project dependency (implementation project(':proj')).

Before, we had some miscommunication and did not completely understand/reproduce the issue (Mostly distracted by the title of this issue, which did not emphasize this was mainly caused by java-library plugin). But now, we know what's going on.

@dmdashenkov
Copy link
Author

@voidzcy, sorry for the lack of this information. I failed to mention this. I guess I was distracted by the fact that java-library actually applies java as well. But yes, we do use java-library in projects where this can be reproduced.

@dmdashenkov
Copy link
Author

I've migrated to 0.8.11 and now the issue seems to be gone. Thank you.

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

No branches or pull requests

5 participants