You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2022/11/28 21:13:43 UTC
[GitHub] [calcite] bchapuis opened a new pull request, #2988: Remove Proj4J from the api dependencies
bchapuis opened a new pull request, #2988:
URL: https://github.com/apache/calcite/pull/2988
Proj4J uses the EPSG dataset and has restricting terms of use. Making Proj4J a compileOnly dependency addresses this issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] julianhyde commented on a diff in pull request #2988: [CALCITE-5399] Remove Proj4J from the api dependencies
Posted by GitBox <gi...@apache.org>.
julianhyde commented on code in PR #2988:
URL: https://github.com/apache/calcite/pull/2988#discussion_r1035407621
##########
site/_docs/howto.md:
##########
@@ -841,6 +841,8 @@ your key to the keyservers used by Nexus, see above.
that the `META-INF` directory contains `LICENSE`,
`NOTICE`
* Check PGP, per [this](https://httpd.apache.org/dev/verification.html)
+* Check that Proj4J is not among the api and implementation dependencies,
Review Comment:
I've made some further remarks in the jira case, and +1 when those are fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] bchapuis commented on a diff in pull request #2988: [CALCITE-5399] Remove Proj4J from the api dependencies
Posted by GitBox <gi...@apache.org>.
bchapuis commented on code in PR #2988:
URL: https://github.com/apache/calcite/pull/2988#discussion_r1034935144
##########
site/_docs/howto.md:
##########
@@ -841,6 +841,8 @@ your key to the keyservers used by Nexus, see above.
that the `META-INF` directory contains `LICENSE`,
`NOTICE`
* Check PGP, per [this](https://httpd.apache.org/dev/verification.html)
+* Check that Proj4J is not among the api and implementation dependencies,
Review Comment:
Yes, this is a good idea. Would adding something like `if ./gradlew dependencies | grep -q proj4j; then exit 1; else exit 0; fi` in a workflow be sufficient?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] bchapuis commented on a diff in pull request #2988: [CALCITE-5399] Remove Proj4J from the api dependencies
Posted by GitBox <gi...@apache.org>.
bchapuis commented on code in PR #2988:
URL: https://github.com/apache/calcite/pull/2988#discussion_r1034935144
##########
site/_docs/howto.md:
##########
@@ -841,6 +841,8 @@ your key to the keyservers used by Nexus, see above.
that the `META-INF` directory contains `LICENSE`,
`NOTICE`
* Check PGP, per [this](https://httpd.apache.org/dev/verification.html)
+* Check that Proj4J is not among the api and implementation dependencies,
Review Comment:
Yes, this is a good idea. Would adding something like `if ./gradlew dependencies | grep -q proj4j; then exit 1; else exit 0; fi` in a workflow be sufficient? Should it be a dedicated job?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] julianhyde commented on a diff in pull request #2988: [CALCITE-5399] Remove Proj4J from the api dependencies
Posted by GitBox <gi...@apache.org>.
julianhyde commented on code in PR #2988:
URL: https://github.com/apache/calcite/pull/2988#discussion_r1035403678
##########
site/_docs/howto.md:
##########
@@ -841,6 +841,8 @@ your key to the keyservers used by Nexus, see above.
that the `META-INF` directory contains `LICENSE`,
`NOTICE`
* Check PGP, per [this](https://httpd.apache.org/dev/verification.html)
+* Check that Proj4J is not among the api and implementation dependencies,
Review Comment:
I think proj4j are going to fix the licensing issue on their end. I think a CI job would be overkill given that this fix will be short-lived.
I suggest adding a 'Add Proj4j as a dependency' Jira case, to be fixed in 1.34, and revisit then.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] bchapuis commented on a diff in pull request #2988: [CALCITE-5399] Remove Proj4J from the api dependencies
Posted by GitBox <gi...@apache.org>.
bchapuis commented on code in PR #2988:
URL: https://github.com/apache/calcite/pull/2988#discussion_r1034723743
##########
core/src/main/java/org/apache/calcite/runtime/ProjectionTransformer.java:
##########
@@ -40,6 +40,14 @@
/**
* Transforms the projection of a geometry.
+ *
+ * This class uses Proj4J to transform the projection of a geometry
+ * and should not be used beyond the scope of the Spatial Type Extensions.
+ * Proj4J is released under the Apache License 2.0, however, it also uses the EPSG dataset,
+ * which has restricting <a href="https://epsg.org/terms-of-use.html">terms of use</a>.
+ * As a result, Proj4J is not suitable for inclusion in Apache Calcite
+ * and this class will throw {@code ClassNotFoundException}s
+ * if Proj4J is not added to the classpath by the user.
Review Comment:
This is a good idea, I added notices in `reference.md` and `spatial.md`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] bchapuis commented on a diff in pull request #2988: [CALCITE-5399] Remove Proj4J from the api dependencies
Posted by GitBox <gi...@apache.org>.
bchapuis commented on code in PR #2988:
URL: https://github.com/apache/calcite/pull/2988#discussion_r1034797722
##########
core/build.gradle.kts:
##########
@@ -57,6 +54,11 @@ dependencies {
api("org.checkerframework:checker-qual")
api("org.slf4j:slf4j-api")
+ api("org.locationtech.jts:jts-core")
+ api("org.locationtech.jts.io:jts-io-common")
+ compileOnly("org.locationtech.proj4j:proj4j")
+ testImplementation("org.locationtech.proj4j:proj4j")
Review Comment:
> Maybe you did this already but if not it would be nice to create a very simple maven project that depends on `calcite-core` and uses the Spatial extension to verify that indeed you get the desired `ClassNotFoundException`.
Good idea, I created a simple project and it confirms that everything behave as expected.
https://github.com/bchapuis/calcite-proj4j
Here is the dependency tree without the proj4j version 1.1.5 dependency:
```
[INFO] org.example:calcite-proj4j:jar:1.0-SNAPSHOT
[INFO] \- org.apache.calcite:calcite-core:jar:1.33.0-SNAPSHOT:compile
[INFO] +- org.apache.calcite:calcite-linq4j:jar:1.33.0-SNAPSHOT:compile
[INFO] +- com.fasterxml.jackson.core:jackson-annotations:jar:2.14.0:compile
[INFO] +- com.google.errorprone:error_prone_annotations:jar:2.11.0:compile
[INFO] +- com.google.guava:guava:jar:31.1-jre:compile
[INFO] | +- com.google.guava:failureaccess:jar:1.0.1:compile
[INFO] | +- com.google.guava:listenablefuture:jar:9999.0-empty-to-avoid-conflict-with-guava:compile
[INFO] | +- com.google.code.findbugs:jsr305:jar:3.0.2:compile
[INFO] | \- com.google.j2objc:j2objc-annotations:jar:1.3:compile
[INFO] +- org.apache.calcite.avatica:avatica-core:jar:1.22.0:compile
[INFO] | +- org.apache.calcite.avatica:avatica-metrics:jar:1.22.0:compile
[INFO] | +- com.google.protobuf:protobuf-java:jar:3.17.1:compile
[INFO] | +- org.apache.httpcomponents.client5:httpclient5:jar:5.1.3:runtime
[INFO] | | \- org.apache.httpcomponents.core5:httpcore5-h2:jar:5.1.3:runtime
[INFO] | \- org.apache.httpcomponents.core5:httpcore5:jar:5.1.3:runtime
[INFO] +- org.apiguardian:apiguardian-api:jar:1.1.2:compile
[INFO] +- org.checkerframework:checker-qual:jar:3.12.0:compile
[INFO] +- org.slf4j:slf4j-api:jar:1.7.33:compile
[INFO] +- org.locationtech.jts:jts-core:jar:1.19.0:compile
[INFO] +- org.locationtech.jts.io:jts-io-common:jar:1.19.0:compile
[INFO] | \- com.googlecode.json-simple:json-simple:jar:1.1.1:compile
[INFO] +- com.fasterxml.jackson.core:jackson-core:jar:2.14.0:compile
[INFO] +- com.fasterxml.jackson.core:jackson-databind:jar:2.14.0:compile
[INFO] +- com.fasterxml.jackson.dataformat:jackson-dataformat-yaml:jar:2.14.0:runtime
[INFO] | \- org.yaml:snakeyaml:jar:1.33:runtime
[INFO] +- com.google.uzaygezen:uzaygezen-core:jar:0.2:runtime
[INFO] +- com.jayway.jsonpath:json-path:jar:2.7.0:runtime
[INFO] | \- net.minidev:json-smart:jar:2.4.7:runtime
[INFO] | \- net.minidev:accessors-smart:jar:2.4.7:runtime
[INFO] | \- org.ow2.asm:asm:jar:9.1:runtime
[INFO] +- com.yahoo.datasketches:sketches-core:jar:0.9.0:runtime
[INFO] | \- com.yahoo.datasketches:memory:jar:0.9.0:runtime
[INFO] +- commons-codec:commons-codec:jar:1.13:runtime
[INFO] +- net.hydromatic:aggdesigner-algorithm:jar:6.0:runtime
[INFO] | +- commons-lang:commons-lang:jar:2.4:runtime
[INFO] | \- commons-logging:commons-logging:jar:1.1.3:runtime
[INFO] +- org.apache.commons:commons-dbcp2:jar:2.6.0:runtime
[INFO] | \- org.apache.commons:commons-pool2:jar:2.6.1:runtime
[INFO] +- org.apache.commons:commons-lang3:jar:3.8:runtime
[INFO] +- org.apache.commons:commons-math3:jar:3.6.1:runtime
[INFO] +- commons-io:commons-io:jar:2.11.0:runtime
[INFO] +- org.codehaus.janino:commons-compiler:jar:3.1.8:runtime
[INFO] \- org.codehaus.janino:janino:jar:3.1.8:runtime
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] bchapuis commented on a diff in pull request #2988: [CALCITE-5399] Remove Proj4J from the api dependencies
Posted by GitBox <gi...@apache.org>.
bchapuis commented on code in PR #2988:
URL: https://github.com/apache/calcite/pull/2988#discussion_r1034797722
##########
core/build.gradle.kts:
##########
@@ -57,6 +54,11 @@ dependencies {
api("org.checkerframework:checker-qual")
api("org.slf4j:slf4j-api")
+ api("org.locationtech.jts:jts-core")
+ api("org.locationtech.jts.io:jts-io-common")
+ compileOnly("org.locationtech.proj4j:proj4j")
+ testImplementation("org.locationtech.proj4j:proj4j")
Review Comment:
> Maybe you did this already but if not it would be nice to create a very simple maven project that depends on `calcite-core` and uses the Spatial extension to verify that indeed you get the desired `ClassNotFoundException`.
Good idea, I created a simple project, and it confirms that everything behave as expected.
https://github.com/bchapuis/calcite-proj4j
Here is the dependency tree without the proj4j version 1.1.5 dependency:
```
[INFO] org.example:calcite-proj4j:jar:1.0-SNAPSHOT
[INFO] \- org.apache.calcite:calcite-core:jar:1.33.0-SNAPSHOT:compile
[INFO] +- org.apache.calcite:calcite-linq4j:jar:1.33.0-SNAPSHOT:compile
[INFO] +- com.fasterxml.jackson.core:jackson-annotations:jar:2.14.0:compile
[INFO] +- com.google.errorprone:error_prone_annotations:jar:2.11.0:compile
[INFO] +- com.google.guava:guava:jar:31.1-jre:compile
[INFO] | +- com.google.guava:failureaccess:jar:1.0.1:compile
[INFO] | +- com.google.guava:listenablefuture:jar:9999.0-empty-to-avoid-conflict-with-guava:compile
[INFO] | +- com.google.code.findbugs:jsr305:jar:3.0.2:compile
[INFO] | \- com.google.j2objc:j2objc-annotations:jar:1.3:compile
[INFO] +- org.apache.calcite.avatica:avatica-core:jar:1.22.0:compile
[INFO] | +- org.apache.calcite.avatica:avatica-metrics:jar:1.22.0:compile
[INFO] | +- com.google.protobuf:protobuf-java:jar:3.17.1:compile
[INFO] | +- org.apache.httpcomponents.client5:httpclient5:jar:5.1.3:runtime
[INFO] | | \- org.apache.httpcomponents.core5:httpcore5-h2:jar:5.1.3:runtime
[INFO] | \- org.apache.httpcomponents.core5:httpcore5:jar:5.1.3:runtime
[INFO] +- org.apiguardian:apiguardian-api:jar:1.1.2:compile
[INFO] +- org.checkerframework:checker-qual:jar:3.12.0:compile
[INFO] +- org.slf4j:slf4j-api:jar:1.7.33:compile
[INFO] +- org.locationtech.jts:jts-core:jar:1.19.0:compile
[INFO] +- org.locationtech.jts.io:jts-io-common:jar:1.19.0:compile
[INFO] | \- com.googlecode.json-simple:json-simple:jar:1.1.1:compile
[INFO] +- com.fasterxml.jackson.core:jackson-core:jar:2.14.0:compile
[INFO] +- com.fasterxml.jackson.core:jackson-databind:jar:2.14.0:compile
[INFO] +- com.fasterxml.jackson.dataformat:jackson-dataformat-yaml:jar:2.14.0:runtime
[INFO] | \- org.yaml:snakeyaml:jar:1.33:runtime
[INFO] +- com.google.uzaygezen:uzaygezen-core:jar:0.2:runtime
[INFO] +- com.jayway.jsonpath:json-path:jar:2.7.0:runtime
[INFO] | \- net.minidev:json-smart:jar:2.4.7:runtime
[INFO] | \- net.minidev:accessors-smart:jar:2.4.7:runtime
[INFO] | \- org.ow2.asm:asm:jar:9.1:runtime
[INFO] +- com.yahoo.datasketches:sketches-core:jar:0.9.0:runtime
[INFO] | \- com.yahoo.datasketches:memory:jar:0.9.0:runtime
[INFO] +- commons-codec:commons-codec:jar:1.13:runtime
[INFO] +- net.hydromatic:aggdesigner-algorithm:jar:6.0:runtime
[INFO] | +- commons-lang:commons-lang:jar:2.4:runtime
[INFO] | \- commons-logging:commons-logging:jar:1.1.3:runtime
[INFO] +- org.apache.commons:commons-dbcp2:jar:2.6.0:runtime
[INFO] | \- org.apache.commons:commons-pool2:jar:2.6.1:runtime
[INFO] +- org.apache.commons:commons-lang3:jar:3.8:runtime
[INFO] +- org.apache.commons:commons-math3:jar:3.6.1:runtime
[INFO] +- commons-io:commons-io:jar:2.11.0:runtime
[INFO] +- org.codehaus.janino:commons-compiler:jar:3.1.8:runtime
[INFO] \- org.codehaus.janino:janino:jar:3.1.8:runtime
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] bchapuis commented on a diff in pull request #2988: [CALCITE-5399] Remove Proj4J from the api dependencies
Posted by GitBox <gi...@apache.org>.
bchapuis commented on code in PR #2988:
URL: https://github.com/apache/calcite/pull/2988#discussion_r1034935144
##########
site/_docs/howto.md:
##########
@@ -841,6 +841,8 @@ your key to the keyservers used by Nexus, see above.
that the `META-INF` directory contains `LICENSE`,
`NOTICE`
* Check PGP, per [this](https://httpd.apache.org/dev/verification.html)
+* Check that Proj4J is not among the api and implementation dependencies,
Review Comment:
Yes, this is a good idea. Would adding something like `if ./gradlew dependencies | grep -q proj4j; then exit 1; else exit 0; fi` in a workflow be sufficient? Should it be a dedicated github actions job?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] bchapuis merged pull request #2988: [CALCITE-5399] Remove Proj4J from the api dependencies
Posted by GitBox <gi...@apache.org>.
bchapuis merged PR #2988:
URL: https://github.com/apache/calcite/pull/2988
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] bchapuis commented on a diff in pull request #2988: [CALCITE-5399] Remove Proj4J from the api dependencies
Posted by GitBox <gi...@apache.org>.
bchapuis commented on code in PR #2988:
URL: https://github.com/apache/calcite/pull/2988#discussion_r1034797722
##########
core/build.gradle.kts:
##########
@@ -57,6 +54,11 @@ dependencies {
api("org.checkerframework:checker-qual")
api("org.slf4j:slf4j-api")
+ api("org.locationtech.jts:jts-core")
+ api("org.locationtech.jts.io:jts-io-common")
+ compileOnly("org.locationtech.proj4j:proj4j")
+ testImplementation("org.locationtech.proj4j:proj4j")
Review Comment:
> Maybe you did this already but if not it would be nice to create a very simple maven project that depends on `calcite-core` and uses the Spatial extension to verify that indeed you get the desired `ClassNotFoundException`.
Good idea, I create a simple project and it confirms that everything behave as expected.
https://github.com/bchapuis/calcite-proj4j
Here is the dependency tree without the proj4j version 1.1.5 dependency:
```
[INFO] org.example:calcite-proj4j:jar:1.0-SNAPSHOT
[INFO] \- org.apache.calcite:calcite-core:jar:1.33.0-SNAPSHOT:compile
[INFO] +- org.apache.calcite:calcite-linq4j:jar:1.33.0-SNAPSHOT:compile
[INFO] +- com.fasterxml.jackson.core:jackson-annotations:jar:2.14.0:compile
[INFO] +- com.google.errorprone:error_prone_annotations:jar:2.11.0:compile
[INFO] +- com.google.guava:guava:jar:31.1-jre:compile
[INFO] | +- com.google.guava:failureaccess:jar:1.0.1:compile
[INFO] | +- com.google.guava:listenablefuture:jar:9999.0-empty-to-avoid-conflict-with-guava:compile
[INFO] | +- com.google.code.findbugs:jsr305:jar:3.0.2:compile
[INFO] | \- com.google.j2objc:j2objc-annotations:jar:1.3:compile
[INFO] +- org.apache.calcite.avatica:avatica-core:jar:1.22.0:compile
[INFO] | +- org.apache.calcite.avatica:avatica-metrics:jar:1.22.0:compile
[INFO] | +- com.google.protobuf:protobuf-java:jar:3.17.1:compile
[INFO] | +- org.apache.httpcomponents.client5:httpclient5:jar:5.1.3:runtime
[INFO] | | \- org.apache.httpcomponents.core5:httpcore5-h2:jar:5.1.3:runtime
[INFO] | \- org.apache.httpcomponents.core5:httpcore5:jar:5.1.3:runtime
[INFO] +- org.apiguardian:apiguardian-api:jar:1.1.2:compile
[INFO] +- org.checkerframework:checker-qual:jar:3.12.0:compile
[INFO] +- org.slf4j:slf4j-api:jar:1.7.33:compile
[INFO] +- org.locationtech.jts:jts-core:jar:1.19.0:compile
[INFO] +- org.locationtech.jts.io:jts-io-common:jar:1.19.0:compile
[INFO] | \- com.googlecode.json-simple:json-simple:jar:1.1.1:compile
[INFO] +- com.fasterxml.jackson.core:jackson-core:jar:2.14.0:compile
[INFO] +- com.fasterxml.jackson.core:jackson-databind:jar:2.14.0:compile
[INFO] +- com.fasterxml.jackson.dataformat:jackson-dataformat-yaml:jar:2.14.0:runtime
[INFO] | \- org.yaml:snakeyaml:jar:1.33:runtime
[INFO] +- com.google.uzaygezen:uzaygezen-core:jar:0.2:runtime
[INFO] +- com.jayway.jsonpath:json-path:jar:2.7.0:runtime
[INFO] | \- net.minidev:json-smart:jar:2.4.7:runtime
[INFO] | \- net.minidev:accessors-smart:jar:2.4.7:runtime
[INFO] | \- org.ow2.asm:asm:jar:9.1:runtime
[INFO] +- com.yahoo.datasketches:sketches-core:jar:0.9.0:runtime
[INFO] | \- com.yahoo.datasketches:memory:jar:0.9.0:runtime
[INFO] +- commons-codec:commons-codec:jar:1.13:runtime
[INFO] +- net.hydromatic:aggdesigner-algorithm:jar:6.0:runtime
[INFO] | +- commons-lang:commons-lang:jar:2.4:runtime
[INFO] | \- commons-logging:commons-logging:jar:1.1.3:runtime
[INFO] +- org.apache.commons:commons-dbcp2:jar:2.6.0:runtime
[INFO] | \- org.apache.commons:commons-pool2:jar:2.6.1:runtime
[INFO] +- org.apache.commons:commons-lang3:jar:3.8:runtime
[INFO] +- org.apache.commons:commons-math3:jar:3.6.1:runtime
[INFO] +- commons-io:commons-io:jar:2.11.0:runtime
[INFO] +- org.codehaus.janino:commons-compiler:jar:3.1.8:runtime
[INFO] \- org.codehaus.janino:janino:jar:3.1.8:runtime
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zabetak commented on a diff in pull request #2988: [CALCITE-5399] Remove Proj4J from the api dependencies
Posted by GitBox <gi...@apache.org>.
zabetak commented on code in PR #2988:
URL: https://github.com/apache/calcite/pull/2988#discussion_r1034587984
##########
site/_docs/howto.md:
##########
@@ -841,6 +841,8 @@ your key to the keyservers used by Nexus, see above.
that the `META-INF` directory contains `LICENSE`,
`NOTICE`
* Check PGP, per [this](https://httpd.apache.org/dev/verification.html)
+* Check that Proj4J is not among the api and implementation dependencies,
Review Comment:
We could enforce this with a new CI job as well. Not necessary to do it now but maybe worth considering as a follow-up.
##########
core/build.gradle.kts:
##########
@@ -57,6 +54,11 @@ dependencies {
api("org.checkerframework:checker-qual")
api("org.slf4j:slf4j-api")
+ api("org.locationtech.jts:jts-core")
+ api("org.locationtech.jts.io:jts-io-common")
+ compileOnly("org.locationtech.proj4j:proj4j")
+ testImplementation("org.locationtech.proj4j:proj4j")
Review Comment:
`testImplementation` means that we need it in the compile and runtime classpath for tests; is this true? If we can use `testRuntimeOnly` I think it would be better.
##########
core/build.gradle.kts:
##########
@@ -57,6 +54,11 @@ dependencies {
api("org.checkerframework:checker-qual")
api("org.slf4j:slf4j-api")
+ api("org.locationtech.jts:jts-core")
+ api("org.locationtech.jts.io:jts-io-common")
+ compileOnly("org.locationtech.proj4j:proj4j")
+ testImplementation("org.locationtech.proj4j:proj4j")
Review Comment:
Moreover, consider moving it closer to other dependencies in the same group.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a diff in pull request #2988: [CALCITE-5399] Remove Proj4J from the api dependencies
Posted by GitBox <gi...@apache.org>.
rubenada commented on code in PR #2988:
URL: https://github.com/apache/calcite/pull/2988#discussion_r1034434709
##########
core/src/main/java/org/apache/calcite/runtime/ProjectionTransformer.java:
##########
@@ -40,6 +40,14 @@
/**
* Transforms the projection of a geometry.
+ *
+ * This class uses Proj4J to transform the projection of a geometry
+ * and should not be used beyond the scope of the Spatial Type Extensions.
+ * Proj4J is released under the Apache License 2.0, however, it also uses the EPSG dataset,
+ * which has restricting <a href="https://epsg.org/terms-of-use.html">terms of use</a>.
+ * As a result, Proj4J is not suitable for inclusion in Apache Calcite
+ * and this class will throw {@code ClassNotFoundException}s
+ * if Proj4J is not added to the classpath by the user.
Review Comment:
Would we need to add a similar warning in the web documentation (`reference.md`) in the spatial section?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] bchapuis commented on a diff in pull request #2988: [CALCITE-5399] Remove Proj4J from the api dependencies
Posted by GitBox <gi...@apache.org>.
bchapuis commented on code in PR #2988:
URL: https://github.com/apache/calcite/pull/2988#discussion_r1034935144
##########
site/_docs/howto.md:
##########
@@ -841,6 +841,8 @@ your key to the keyservers used by Nexus, see above.
that the `META-INF` directory contains `LICENSE`,
`NOTICE`
* Check PGP, per [this](https://httpd.apache.org/dev/verification.html)
+* Check that Proj4J is not among the api and implementation dependencies,
Review Comment:
Yes, this is a good idea. Would adding something like `if ./gradlew dependencies | grep -q proj4j; then exit 1; else exit 0; fi` to the CI be sufficient? Should it be a dedicated github actions job?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] bchapuis commented on a diff in pull request #2988: [CALCITE-5399] Remove Proj4J from the api dependencies
Posted by GitBox <gi...@apache.org>.
bchapuis commented on code in PR #2988:
URL: https://github.com/apache/calcite/pull/2988#discussion_r1034737271
##########
core/build.gradle.kts:
##########
@@ -57,6 +54,11 @@ dependencies {
api("org.checkerframework:checker-qual")
api("org.slf4j:slf4j-api")
+ api("org.locationtech.jts:jts-core")
+ api("org.locationtech.jts.io:jts-io-common")
+ compileOnly("org.locationtech.proj4j:proj4j")
+ testImplementation("org.locationtech.proj4j:proj4j")
Review Comment:
I replaced `testImplementation` by `testRuntimeOnly`, however, builds fail without `compileOnly`. I also tried to improve grouping but failed at identifying a clear pattern (alphabetical order, etc.). I simply moved the spatial related dependencies at the end of each group.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] bchapuis commented on pull request #2988: [CALCITE-5399] Remove Proj4J from the api dependencies
Posted by GitBox <gi...@apache.org>.
bchapuis commented on PR #2988:
URL: https://github.com/apache/calcite/pull/2988#issuecomment-1332752664
@zabetak Thanks a lot for your help.
I just reread the [develop documentation](https://calcite.apache.org/develop/) in prevision of merging the PR. To keep the history clean, I rebased the branch, squashed the commits, and force pushed the result. As a new (and intimidated ;) committer I should now be able to click "rebase and merge" to close this PR. Is that it? Or am I missing something?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zabetak commented on pull request #2988: [CALCITE-5399] Remove Proj4J from the api dependencies
Posted by GitBox <gi...@apache.org>.
zabetak commented on PR #2988:
URL: https://github.com/apache/calcite/pull/2988#issuecomment-1333504823
@bchapuis Thanks for pushing this forward! You are correct; either you click "Rebase and merge" button or you do the same thing via command line. Sharing also another link that you may find useful in the future: https://calcite.apache.org/docs/howto.html#merging-pull-requests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org