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

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

Closed
julianhyde opened this issue Dec 11, 2022 · 14 comments
Closed

Comments

@julianhyde
Copy link

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
Copy link
Member

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
Copy link
Member

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 👍

@bchapuis
Copy link
Contributor

bchapuis commented Dec 12, 2022

@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
Copy link
Member

pomadchin commented Dec 12, 2022

@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 #90 (comment)

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
Copy link
Member

pomadchin commented Dec 12, 2022

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
Copy link
Member

pomadchin commented Dec 12, 2022

I'll cut a release later today. 👍 🤦

@bchapuis
Copy link
Contributor

bchapuis commented Dec 12, 2022

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
Copy link
Member

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

@bchapuis
Copy link
Contributor

bchapuis commented Dec 12, 2022

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
Copy link
Member

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
Copy link

desruisseaux commented Dec 13, 2022

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
Copy link
Contributor

bchapuis commented Dec 13, 2022

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
Copy link

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 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
Copy link
Member

@bchapuis @desruisseaux thank you both!

Yes @bchapuis we definitely need to improve the error message / docs (#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.

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

4 participants