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

build: correctly declare api dependencies #334

Merged
merged 1 commit into from
Feb 27, 2025

Conversation

nielspardon
Copy link
Contributor

Currently, both the isthmus and core projects are declaring all dependencies as implementation dependencies. Gradle has two types of dependencies api and implementation dependencies. From the Gradle documentation:

The api configuration should be used to declare dependencies which are exported by the library API, whereas the implementation configuration should be used to declare dependencies which are internal to the component.

https://docs.gradle.org/current/userguide/java_library_plugin.html#sec:java_library_separation

For the core project some of the protobuf Java classes are part of the Substrait Java API, e.g. com.google.protobuf.ByteString is exposed as part of the io.substrait.expression.Expression interface. Which means we should declare com.google.protobuf:protobuf-java as an api dependency instead of an implementation dependency.

Similarly, in the isthmus project classes coming from the dependency on the core project are part of the isthmus API and should be reported as an api dependency. Additionally, the Apache Calcite classes are part of the isthmus API and so the org.apache.calcite:calcite-core should also be declared as an api dependency.

In order to correctly declare these api dependencies we need to switch from the java to the java-library Gradle plugin.

If such dependencies are not declared as api dependencies Gradle projects depending on the core or isthmus libraries are forced to repeat the dependency declaration on the respective dependencies.

Signed-off-by: Niels Pardon <par@zurich.ibm.com>
@CLAassistant
Copy link

CLAassistant commented Feb 27, 2025

CLA assistant check
All committers have signed the CLA.

@vbarua vbarua self-requested a review February 27, 2025 16:50
Copy link
Member

@vbarua vbarua left a comment

Choose a reason for hiding this comment

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

Thanks for this change, along with the super-clear explanation ✨

@vbarua vbarua changed the title feat: correctly declare api dependencies build: correctly declare api dependencies Feb 27, 2025
@vbarua vbarua merged commit fe08fd4 into substrait-io:main Feb 27, 2025
13 checks passed
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

Successfully merging this pull request may close these issues.

3 participants