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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

gdal: add java bindings #280279

Merged
merged 2 commits into from Feb 6, 2024
Merged

gdal: add java bindings #280279

merged 2 commits into from Feb 6, 2024

Conversation

rollf
Copy link
Contributor

@rollf rollf commented Jan 11, 2024

Description of changes

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 馃憤 reaction to pull requests you find important.

@imincik
Copy link
Contributor

imincik commented Jan 11, 2024

Thank you for contribution. This PR is failing on Darwin now.

@imincik
Copy link
Contributor

imincik commented Jan 11, 2024

Maybe we should also consider building with java as optional feature using withJava ? false derivation parameter.

@imincik
Copy link
Contributor

imincik commented Jan 11, 2024

Result of nixpkgs-review pr 280279 run on x86_64-linux 1

7 packages marked as broken and skipped:
  • python310Packages.worldengine
  • python310Packages.worldengine.dist
  • python311Packages.worldengine
  • python311Packages.worldengine.dist
  • t-rex
  • worldengine-cli
  • worldengine-cli.dist
24 packages failed to build:
  • apacheHttpdPackages.mod_tile (apacheHttpdPackages_2_4.mod_tile)
  • python310Packages.bsuite
  • python310Packages.bsuite.dist
  • python310Packages.cartopy
  • python310Packages.cartopy.dist
  • python310Packages.django-bootstrap4
  • python310Packages.django-bootstrap4.dist
  • python310Packages.fiona
  • python310Packages.fiona.dist
  • python310Packages.folium
  • python310Packages.folium.dist
  • python310Packages.gdal
  • python310Packages.geopandas
  • python310Packages.geopandas.dist
  • python310Packages.osmnx
  • python310Packages.osmnx.dist
  • python310Packages.plotnine
  • python310Packages.plotnine.dist
  • python310Packages.rasterio
  • python310Packages.rasterio.dist
  • python310Packages.wktutils
  • python310Packages.wktutils.dist
  • pytrainer
  • pytrainer.dist
79 packages built:
  • cloudcompare
  • entwine
  • gdal (python311Packages.gdal)
  • gdalMinimal
  • gmt
  • gplates
  • grass
  • haskellPackages.hgdal
  • haskellPackages.hgdal.doc
  • mapcache
  • mapnik
  • mapproxy
  • mapproxy.dist
  • mapserver
  • merkaartor
  • mysql-workbench
  • octavePackages.mapping
  • openorienteering-mapper
  • paraview
  • pdal
  • perl536Packages.Tirex
  • perl536Packages.Tirex.devdoc
  • perl538Packages.Tirex
  • perl538Packages.Tirex.devdoc
  • postgresql12JitPackages.postgis
  • postgresql12JitPackages.postgis.doc
  • postgresql12Packages.postgis
  • postgresql12Packages.postgis.doc
  • postgresql13JitPackages.postgis
  • postgresql13JitPackages.postgis.doc
  • postgresql13Packages.postgis
  • postgresql13Packages.postgis.doc
  • postgresql14JitPackages.postgis
  • postgresql14JitPackages.postgis.doc
  • postgresql14Packages.postgis
  • postgresql14Packages.postgis.doc
  • postgresqlJitPackages.postgis (postgresql15JitPackages.postgis)
  • postgresqlJitPackages.postgis.doc (postgresql15JitPackages.postgis.doc)
  • postgresqlPackages.postgis (postgresql15Packages.postgis)
  • postgresqlPackages.postgis.doc (postgresql15Packages.postgis.doc)
  • postgresql16JitPackages.postgis
  • postgresql16JitPackages.postgis.doc
  • postgresql16Packages.postgis
  • postgresql16Packages.postgis.doc
  • python310Packages.pygmt
  • python310Packages.pygmt.dist
  • python310Packages.python-mapnik
  • python310Packages.python-mapnik.dist
  • python311Packages.bsuite
  • python311Packages.bsuite.dist
  • python311Packages.cartopy
  • python311Packages.cartopy.dist
  • python311Packages.django-bootstrap4
  • python311Packages.django-bootstrap4.dist
  • python311Packages.fiona
  • python311Packages.fiona.dist
  • python311Packages.folium
  • python311Packages.folium.dist
  • python311Packages.geopandas
  • python311Packages.geopandas.dist
  • python311Packages.osmnx
  • python311Packages.osmnx.dist
  • python311Packages.plotnine
  • python311Packages.plotnine.dist
  • python311Packages.pygmt
  • python311Packages.pygmt.dist
  • python311Packages.python-mapnik
  • python311Packages.python-mapnik.dist
  • python311Packages.rasterio
  • python311Packages.rasterio.dist
  • python311Packages.wktutils
  • python311Packages.wktutils.dist
  • qgis
  • qgis-ltr
  • qmapshack
  • saga
  • sumo
  • udig
  • vpv

pkgs/development/libraries/gdal/tests.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/gdal/default.nix Show resolved Hide resolved
pkgs/development/libraries/gdal/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/gdal/tests.nix Outdated Show resolved Hide resolved
@rollf
Copy link
Contributor Author

rollf commented Jan 12, 2024

Maybe we should also consider building with java as optional feature using withJava ? false derivation parameter.

@imincik Thanks for your preliminary review. I will implement this.

@ofborg ofborg bot requested a review from imincik January 12, 2024 10:43
@r-burns
Copy link
Contributor

r-burns commented Jan 12, 2024

Maybe we should also consider building with java as optional feature using withJava ? false derivation parameter.

Alternatively, if it's possible to put the java bindings in a separate output, we could always build the java bindings and not worry about closure bloat in environments containing gdal with+without java support, as they'd both use the same underlying C++ library. But if it's difficult to split out a -java output then I think withJava is a good approach.

@rollf
Copy link
Contributor Author

rollf commented Jan 15, 2024

The GDAL teams defaults to building it Java (if ant & jdk are available) and we already have a withX approach for other optional features so I tend to go with withJava to keep things consistent.

That being said, I'm not the maintainer.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/week-in-geospatial-team/37035/3

@rollf
Copy link
Contributor Author

rollf commented Jan 16, 2024

Looks like this doesn't build on darwin. From the ofBorg log:

       > Center      (  400.0,  300.0)
       > Band 1 Block=800x5 Type=UInt16, ColorInterp=Gray
       >   NoData Value=255
       > Native library load failed.
       > java.lang.UnsatisfiedLinkError: no gdalalljni in java.library.path: /nix/store/iyysy6nv6w55cy2lkignza88hdascpa9-gdal-3.8.2/lib/
       > Exception in thread "main" java.lang.UnsatisfiedLinkError: 'void org.gdal.gdal.gdalJNI.AllRegister()'
       >    at org.gdal.gdal.gdalJNI.AllRegister(Native Method)
       >    at org.gdal.gdal.gdal.AllRegister(gdal.java:712)
       >       at Main.main(main.java:4)
       For full logs, run 'nix log /nix/store/43ryfz175vlkmg60i43h79nraifkxml6-gdal-tests.drv'.

Seems like ti .so file could not be built.

There are several errors in the full log, like:

/tmp/nix-build-gdal-3.8.2.drv-1/source/build/swig/java/org_patched/org/gdal/ogr/ogr.java:17: error: invalid uri: "#Open(java.lang.String, int)"
OR
/tmp/nix-build-gdal-3.8.2.drv-1/source/build/swig/java/org_patched/org/gdal/gdal/Band.java:514: error: invalid uri: "#ReadBlock_Direct(int, int, java.nio.ByteBuffer)"

I have no way to inspect this on a Mac.

@imincik
Copy link
Contributor

imincik commented Jan 16, 2024

Sorry, I can't help with Darwin either.

@rollf
Copy link
Contributor Author

rollf commented Jan 16, 2024

Ah, now that I think about this. I guess it worked just fine because of the log line

Installing: /nix/store/iyysy6nv6w55cy2lkignza88hdascpa9-gdal-3.8.2/lib/jni/libgdalalljni.dylib

but the test fails. I don't have experience with Mac.

@rollf
Copy link
Contributor Author

rollf commented Jan 18, 2024

Okay, I will see what I can do. @imincik FYI, I originally worked on this PR. It is not prepared yet, I just opened a draft PR to be able to point to it from here. Working on this caused me to add the Java bindings to gdal (hence this PR here)

@rollf
Copy link
Contributor Author

rollf commented Jan 20, 2024

@imincik I managed to fix the Darwin issue. The problem was that I symlinked a *.so file although on Darwin, these are called *.dylib. I now use a simple shell expansion. All checks of this PR pass now (at least for dd8dcd0; currently 35353ca gets evaluated but I'm confident it will pass, too). Do you mind having another look? Once this is merged, I'd like to polish #281739.

@rollf
Copy link
Contributor Author

rollf commented Jan 20, 2024

Phew ... now that I found some debug leftovers, should I squash this whole PR?

EDIT: Done.

@rollf
Copy link
Contributor Author

rollf commented Jan 22, 2024

@imincik All checks are passing now. This PR is ready from my point of view. Do you mind having another look into this?

Copy link
Contributor

@imincik imincik left a comment

Choose a reason for hiding this comment

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

Thank you very much for your effort @rollf .

I requested some changes. My biggest concern is whether we should build GDAL with java by default or not.

Also, please remove trailing dot (.) from the commit message and don't use capital letters in change description if not needed (suggested commit message should be something like gdal: add java bindings).

@thoughtpolice , as the main author of gdalMinimal would you mind to review this PR ?

pkgs/development/libraries/gdal/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/gdal/default.nix Show resolved Hide resolved
pkgs/development/libraries/gdal/default.nix Show resolved Hide resolved
pkgs/development/libraries/gdal/tests.nix Show resolved Hide resolved
@imincik
Copy link
Contributor

imincik commented Jan 23, 2024

@rollf , please remove trailing dot (.) from the commit message and don't use capital letters in change description if not needed (suggested commit message should be something like gdal: add java bindings). Thank you.

@rollf rollf changed the title gdal: Add java bindings. gdal: add java bindings Jan 25, 2024
@rollf
Copy link
Contributor Author

rollf commented Jan 25, 2024

@imincik Thank you for the review. I addressed your change requests except for withJava ? false. I currently wait for the ofBorg results and fix issues if any.. I appreciate the attention paid to the closure size. However, I think I have convincing arguments to build with Java support by default:

  • GDAL ships with a lot of possible integraitons; from what I can see, they are mostly build automatically when the deps are found. For example, the nix package definition adds cfitsio, brunsli, cryptopp, geos, etc. as dependencies which in turn enables the usage of these libraries. Python bindings are built by default if python is found and python3 is yet another dependency provided in gdal's package definition. Why treat Java differently? Just because it came late to the party? I feel like useJava ? (!useMinimalFeatures) would be a good compromise here.
  • The first package merged into nixpkgs that uses the GDAL java bindings will have to use gdal.override { withJava = true; } which will actually render the size argument obsolute since it will essentially force the existence 3 gdal packages:
    • "full gdal (with Java, not build by default)",
    • "full gdal (without Java, build by default)",
    • and "minimal gdal (build by default)".
  • Any packages that uses the non-default-gdal-with-Java derivation needs to build gdal (which is actually quite time-consuming.
  • The test code I added will fail by default, so
    • either I remove the test code which means I'd contribute non-tested code. Future maintainers will have to make sure the building of the Java bindings works
    • or I simply use gdal.override { withJava = true; } which creates a new package solely for the test (see my 2nd argument).

Please consider this carefully. As I mentioned above, I plan to use gdal-with-Java-bindings rather soon.

@ofborg ofborg bot requested a review from imincik January 25, 2024 12:06
@imincik
Copy link
Contributor

imincik commented Jan 25, 2024

I addressed your change requests except for withJava ? false

I must say I like your arguments. Personally, I am happy to take it, but I would be happier if more people will say yes.

@das-g
Copy link
Member

das-g commented Jan 25, 2024

I'm no expert on closure size considerations, but @rollf's arguments convince me. Strongly.

If still in doubt, I'd opt for the useJava ? (!useMinimalFeatures) compromise he suggests.

@rollf
Copy link
Contributor Author

rollf commented Jan 29, 2024

I must say I like your arguments. Personally, I am happy to take it, but I would be happier if more people will say yes.

I'm no expert on closure size considerations, but @rollf's arguments convince me. Strongly.

If still in doubt, I'd opt for the useJava ? (!useMinimalFeatures) compromise he suggests.

Okay, thank you. I believe I have addressed all other change requests. From my point of view, this PR is done and could be merged.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/week-in-geospatial-team/37035/6

@imincik
Copy link
Contributor

imincik commented Jan 31, 2024

Current default Java version is EOL - #280901 . Fix in #273811

@imincik
Copy link
Contributor

imincik commented Jan 31, 2024

Result of nixpkgs-review pr 280279 run on x86_64-linux 1

13 packages marked as broken and skipped:
  • python311Packages.worldengine
  • python311Packages.worldengine.dist
  • python312Packages.fiona
  • python312Packages.fiona.dist
  • python312Packages.geopandas
  • python312Packages.geopandas.dist
  • python312Packages.plotnine
  • python312Packages.plotnine.dist
  • python312Packages.worldengine
  • python312Packages.worldengine.dist
  • t-rex
  • worldengine-cli
  • worldengine-cli.dist
7 packages failed to build:
  • apacheHttpdPackages.mod_tile (apacheHttpdPackages_2_4.mod_tile)
  • cloudcompare
  • python311Packages.osmnx
  • python311Packages.osmnx.dist
  • python311Packages.rasterio
  • python311Packages.rasterio.dist
  • qgis-ltr
75 packages built:
  • entwine
  • flatcam
  • gdal (python311Packages.gdal)
  • gdalMinimal
  • gmt
  • gplates
  • grass
  • haskellPackages.hgdal
  • haskellPackages.hgdal.doc
  • mapcache
  • mapnik
  • mapproxy
  • mapproxy.dist
  • mapserver
  • merkaartor
  • mysql-workbench
  • octavePackages.mapping
  • openorienteering-mapper
  • paraview
  • pdal
  • perl536Packages.Tirex
  • perl536Packages.Tirex.devdoc
  • perl538Packages.Tirex
  • perl538Packages.Tirex.devdoc
  • postgresql12JitPackages.postgis
  • postgresql12JitPackages.postgis.doc
  • postgresql12Packages.postgis
  • postgresql12Packages.postgis.doc
  • postgresql13JitPackages.postgis
  • postgresql13JitPackages.postgis.doc
  • postgresql13Packages.postgis
  • postgresql13Packages.postgis.doc
  • postgresql14JitPackages.postgis
  • postgresql14JitPackages.postgis.doc
  • postgresql14Packages.postgis
  • postgresql14Packages.postgis.doc
  • postgresqlJitPackages.postgis (postgresql15JitPackages.postgis)
  • postgresqlJitPackages.postgis.doc (postgresql15JitPackages.postgis.doc)
  • postgresqlPackages.postgis (postgresql15Packages.postgis)
  • postgresqlPackages.postgis.doc (postgresql15Packages.postgis.doc)
  • postgresql16JitPackages.postgis
  • postgresql16JitPackages.postgis.doc
  • postgresql16Packages.postgis
  • postgresql16Packages.postgis.doc
  • python311Packages.bsuite
  • python311Packages.bsuite.dist
  • python311Packages.cartopy
  • python311Packages.cartopy.dist
  • python311Packages.django-bootstrap4
  • python311Packages.django-bootstrap4.dist
  • python311Packages.fiona
  • python311Packages.fiona.dist
  • python311Packages.folium
  • python311Packages.folium.dist
  • python311Packages.geopandas
  • python311Packages.geopandas.dist
  • python311Packages.plotnine
  • python311Packages.plotnine.dist
  • python311Packages.pygmt
  • python311Packages.pygmt.dist
  • python311Packages.python-mapnik
  • python311Packages.python-mapnik.dist
  • python311Packages.wktutils
  • python311Packages.wktutils.dist
  • python312Packages.gdal
  • python312Packages.pygmt
  • python312Packages.pygmt.dist
  • pytrainer
  • pytrainer.dist
  • qgis
  • qmapshack
  • saga
  • sumo
  • udig
  • vpv
E           hypothesis.errors.Flaky: Hypothesis test_outer_boundless_pixel_fidelity(path_rgb_byte_tif='/build/source/tests/data/RGB.byte.tif', col_start=-3, row_start=-53, col_stop=2, row_stop=256) produces unreliable results: Falsified on the first call but did not on a subsequent one
E           Falsifying example: test_outer_boundless_pixel_fidelity(
E               path_rgb_byte_tif='/build/source/tests/data/RGB.byte.tif',
E               col_start=-3,
E               row_start=-53,
E               col_stop=2,
E               row_stop=256,
E           )
E           Unreliable test timings! On an initial run, this test took 597.15ms, which exceeded the deadline of 200.00ms, but on a subsequent run it took 40.38 ms, which did not. If you expect this sort of variability in your test timings, consider turning deadlines off for this test by setting deadline=None.

/nix/store/1yhs34ajg2s4ydqvnlflkc8imhg4pl2g-python3.11-hypothesis-6.91.0/lib/python3.11/site-packages/hypothesis/core.py:892: Flaky

@imincik imincik merged commit 621798f into NixOS:master Feb 6, 2024
23 checks passed
@imincik
Copy link
Contributor

imincik commented Feb 6, 2024

@rollf , thanks for contributing.

imincik added a commit to imincik/geospatial-nix that referenced this pull request Feb 9, 2024
Nixpkgs PR:
NixOS/nixpkgs#280279

Building with Java support is disabled by default
in Geospatial NIX.
imincik added a commit to imincik/geospatial-nix that referenced this pull request Feb 9, 2024
Nixpkgs PR:
NixOS/nixpkgs#280279

Building with Java support is disabled by default
in Geospatial NIX.
imincik added a commit to imincik/geospatial-nix that referenced this pull request Feb 9, 2024
Nixpkgs PR:
NixOS/nixpkgs#280279

Building with Java support is disabled by default
in Geospatial NIX.
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/week-in-geospatial-team/37035/7

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

Successfully merging this pull request may close these issues.

None yet

5 participants