locationtech / proj4j

Java port of the Proj.4 library for coordinate reprojection
Other
181 stars 71 forks source link

In release 1.2.0, the pom file contains incorrect version '1.2.0-SNAPSHOT' #94

Closed julianhyde closed 1 year ago

julianhyde commented 1 year ago

In release 1.2.0, the pom file contains incorrect version '1.2.0-SNAPSHOT'. It should be '1.2.0'.

See https://search.maven.org/remotecontent?filepath=org/locationtech/proj4j/proj4j/1.2.0/proj4j-1.2.0.pom and also https://central.sonatype.dev/artifact/org.locationtech.proj4j/proj4j/1.2.0-SNAPSHOT/versions

This caused problems when I tried to upgrade Calcite to use it. See https://github.com/apache/calcite/pull/3001/checks.

pomadchin commented 1 year ago

Thanks for catching it; I’ll check what’s going on and cut a fresh release; I think I know where the error is coming from

pomadchin commented 1 year ago

Hey! Could not make it yesterday, but should be good now: https://repo1.maven.org/maven2/org/locationtech/proj4j/proj4j/1.2.1/proj4j-1.2.1.pom :+1:

bchapuis commented 1 year ago

@pomadchin Thanks a lot, the checks are now passing. However, I expected some tests to fail as we use EPSG:26986 (Massachusetts state plane meters). After looking at the sources, it looks like the epsg file is still in the proj4j core submodule. Shouldn't it be moved to the epsg submodule?

pomadchin commented 1 year ago

@bchapuis I think this file is not a part of the EPSG db; I tried to find the history of this file and it looks like it's been collected somehow for the proj4 project specifically, long time ago they didn't have a sqlite db but used plain files for the proj4string <=> epsg code conversion; it is a list of proj4 strings.

I am not sure this file is under the EPSG license; but if you're sure it is I can work on it as well; also see https://github.com/locationtech/proj4j/issues/90#issuecomment-1332180608

Mb you're referring to a very similar by name https://github.com/locationtech/proj4j/blob/master/epsg/src/main/resources/proj4/wkt/epsg.properties which is really a part of the EPSG DB.

pomadchin commented 1 year ago

You know what, I think you're right; I'll move all the resources into that epsg module; so none of projections going to work with the core only. I'll make a follow-up issue.

I think I had to just move everything,

pomadchin commented 1 year ago

I'll cut a release later today. :+1: 🤦

bchapuis commented 1 year ago

Thanks a lot for your help.

I was just experimenting with this on my side and made a pull request. Here, the epsg submodule is included as a test dependency in the core submodule.

We still have some epsg files in the test resources, but I think that they will not be included in the final jar. I tried to move them as well, but this would require to modify the tests.

I hope this helps a bit.

pomadchin commented 1 year ago

Yes, test resources should be fine! They are not shipped. Thanks for your help!

bchapuis commented 1 year ago

I am not sure this file is under the EPSG license; but if you're sure it is I can work on it as well; also see #90 (comment)

I'm not completly sure either. As the file contains a lot of epsg codes, I guess that it is under the same terms of use. Maybe @desruisseaux can crosscheck.

pomadchin commented 1 year ago

I feel like it makes sense to move all resources out just in case; you asked a question => definitely some one else may ask it; thanks for doing it; it is good that you noticed it and asked.

sorry I’m not by the laptop at the moment, will work on merging your pr ASAP!

desruisseaux commented 1 year ago

If we are talking about the epsg.properties file, indeed this is EPSG data encoded in a different way (as WKT definitions instead of CSV tables). Thanks for moving it.

About testing, as said above it is okay (to my knowledge) to use EPSG data during tests. What matter is what is included in the distribution. But at Apache (I don't know what is Eclipse policy), the fact that EPSG data are used for tests would be declared in the NOTICE file.

bchapuis commented 1 year ago

The new release now throws an exception when the epsg module is not included as a dependency, which is the expected behavior. From a license perspective, the proj4/nad/epsg file was the one causing concerns.

> java.lang.IllegalStateException: Unable to access CRS file: proj4/nad/epsg
    >   at org.locationtech.proj4j.io.Proj4FileReader.readParametersFromFile(Proj4FileReader.java:44)
    >   at org.locationtech.proj4j.io.Proj4FileReader.getParameters(Proj4FileReader.java:119)
    >   at org.locationtech.proj4j.CRSFactory.createFromName(CRSFactory.java:83)
    >   at org.apache.calcite.runtime.ProjectionTransformer.<init>(ProjectionTransformer.java:57)
    >   at org.apache.calcite.runtime.SpatialTypeFunctions.ST_Transform(SpatialTypeFunctions.java:[1196]

@pomadchin should we improve the exception message and or the documentation?

@desruisseaux If I remember correclty, you mentionned that small extracts of the epsg database may fall under a fair use clause. Do you think we may include a couple of popular EPSGs in the core (e.g. 4326, 3857). If so, which EPSGs (apart from these two) would it make sense to include?

desruisseaux commented 1 year ago

The definition of "fair use" is subject to interpretation. My approach is to pick CRS that are collected in other well-known sources. For example OGC Web Map Server (WMS) defines a few CRS in the CRS:xxxx namespace with only a different axis order. I use the numerical data (e.g. ellipsoid axis length) but I exclude all EPSG metadata (scope, remarks, area of use, etc.) except the EPSG code, which I interpret as meaning "See EPSG:xxxx for more complete definition".

There is a list of CRS defined by WMS specification there. The sources that I use implies [this list of EPSG codes](https://sis.apache.org/apidocs/org/apache/sis/referencing/CommonCRS.html#geographic()) of geographic CRS. For projected CRS, I guess that "World Mercator" (EPSG::3395) and Google Mercator are fair use.

If Proj4J has a field for remarks in CRS instances, it may be worth to put a remark like below (I attach this text to the CRS objects defined without EPSG database):

Definitions from public sources. When a definition corresponds to an EPSG object (ignoring metadata), the EPSG code is provided as a reference where to find the complete definition.

pomadchin commented 1 year ago

@bchapuis @desruisseaux thank you both!

Yes @bchapuis we definitely need to improve the error message / docs (https://github.com/locationtech/proj4j/issues/96);

If any of these files / portions of the files can be moved back that will require some code changes as well 💭 but doable. With this one I will definitely ask for some help to understand which codes are safe to move.

But really I think that it's kind of 'okay' to expect that all of these resources can be considered under the EPSG License due to its source.