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

Resolve MacOS dylib loading issues for gdal-warp-bindings #99

Merged
merged 9 commits into from
Nov 13, 2020

Conversation

CloudNiner
Copy link
Contributor

@CloudNiner CloudNiner commented Nov 12, 2020

Overview

We were compiling macos gdal-warp-bindings with homebrew gdal leading to the errors in #98 and #73. This PR fixes those issues with a two-pronged approach:

  1. Compile bindings with the conda gdal binary package: This updates the linker path for libgdal.dylib to @rpath/libgdal.dylib, removing the HOMEBREW_PREFIX.
  2. Add ld flag -rpath=/usr/local/lib so that a common dir is in the rpath of our libgdalwarp_bindings.dylib and the linker can find the gdal dylib at a common location

This is still a bit of a mess. We currently build gdal 3.1.2 but brew install gdal is currently 3.2.0, which switches to libgdal.28.dylib(3.2) from libgdal.27.dylib (3.1.2). So, in the simplest case, if you install gdal with brew, and don't already have 3.1.2 installed, you'll still get a linker error. HOWEVER... now you can install gdal into a conda env and symlink the appropriate dylib to /usr/local/lib and these bindings will work:

conda create -n gdal-3.1.2
conda activate gdal-3.1.2
conda install -c conda-forge gdal==3.1.2
sudo ln -s <path/to/conda/install>/envs/gdal-3.1.2/lib/libgdal.27.dylib /usr/local/lib/libgdal.27.dylib
conda deactivate
cd path/to/my/project
./sbt run
[info] welcome to sbt 1.4.0 (AdoptOpenJDK Java 1.8.0_252)
[info] loading project definition from /Users/azavea/src/gdal-rs-test/project
[info] loading settings for project gdal-rs-test from build.sbt ...
[info] set current project to gdal-bug (in build file:/Users/azavea/src/gdal-rs-test/)
[info] running Main 
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
Extent(0.0, 0.0, 806.0, 568.0)
[success] Total time: 3 s, completed Nov 12, 2020 4:08:59 PM

Unfortunately this doesn't address #91, although at this point I'm not sure we need to since we can symlink the appropriate gdal dylib installed anywhere to /usr/local/lib and it will work.

Demo

Here's a zipped sbt project you can download and run to verify that this build fixes the issue. Ensure you have a libgdal.27.dylib in /usr/local/lib, either because you have a brew install of gdal 3.1.2 there, or you installed it via conda and manually linked it there, as described above:

gdal-warp-bindings-macos-test.zip

Closes #98
Closes #73

cc @metasim

@CloudNiner
Copy link
Contributor Author

CloudNiner commented Nov 12, 2020

Remaining TODOs:

  • Revert change to image name in PR diff
  • How do we publish this fix?
  • Document steps for MacOS users to symlink the correct dylib to /usr/local/lib
  • Upgrade to gdal 3.2.0 so brew installs work again? Update to GDAL 3.7.x #100

Copy link
Member

@pomadchin pomadchin left a comment

Choose a reason for hiding this comment

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

In comments I covered 1. the state of the docker image naming conventions that are established right now in the repo and what should be changed in CircleCI to make it happy later! Hope it would help you :D

  1. The release is done through the CircleCI (the detailed explanation can be found here CI / Auto Publish #87); but basically you'd need to run git tag -a v1.1.1 -m "v1.1.1" && git push --tags and after that to check jars manually on Sonatype using company credentials and if everything is fine do a manual release. (this is a manual process just for sainity now)

  2. Yes, I agree that we need to document GDAL compatible versions description and to mention explicitly that these bindings would use /usr/local/lib by default as a place to search for dylibs. For the context and for other readers: PDAL also has a default rpath hardcoded to /usr/local/lib

  3. How do you feel about putting the upgrade up to GDAL 3.2.0 into a separate issue here?

Looks good to me in general and fixes the issue!

scripts/publish-local.sh Show resolved Hide resolved
scripts/publish-local.sh Outdated Show resolved Hide resolved
@CloudNiner CloudNiner mentioned this pull request Nov 12, 2020
@CloudNiner
Copy link
Contributor Author

How do you feel about putting the upgrade up to GDAL 3.2.0 into a separate issue here?

Opened #100

Yes, I agree that we need to document GDAL compatible versions description and to mention explicitly that these bindings would use /usr/local/lib

Do you have a preference for where to put that information? Add it to this project's README? Also add it to GeoTrellis documentation somewhere?

Release stuff noted, I'll work on that in a bit, I have a request out to @geotrellis/operations to see if we can't get org-wide credentials for this container so we don't have to publish it to our personal repos.

@pomadchin
Copy link
Member

pomadchin commented Nov 12, 2020

I think README.md; to have section similar to https://github.com/geotrellis/gdal-warp-bindings#macintosh
I'm also fine with releasing the updated docker image for now; I'll let you know once it would be done.

@pomadchin
Copy link
Member

pomadchin commented Nov 12, 2020

@CloudNiner I published daunnc/gdal-build-environment:7 so we can start upgrading CircleCI and move towards the release process. If the situation with orgs would be resolved that is good and by that time we would have established and tested builds.

Copy link
Member

@jamesmcclain jamesmcclain left a comment

Choose a reason for hiding this comment

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

If this solves it, then 👍

@CloudNiner
Copy link
Contributor Author

CloudNiner commented Nov 13, 2020

@pomadchin updated container name, build settings, and there's a working jar published to "com.azavea.geotrellis" % "gdal-warp-bindings" % "1.1.1-feature_awf_macos_conda_lib-SNAPSHOT" for testing. Updated the PR description with a demo project.

Docs update coming soon.

@metasim
Copy link

metasim commented Nov 13, 2020

Awesome, exciting work guys! Thanks for CC'ing me!

@CloudNiner
Copy link
Contributor Author

@pomadchin updated with gt quay image and some docs + changelog. Should be ready to go pending final review. Will create a tag and publish after merge.

Copy link
Member

@pomadchin pomadchin left a comment

Choose a reason for hiding this comment

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

Looks very good to me; thanks Andrew!

One minor issue - a broken MD table and probably we need to add OS into this table.

After that we're good to squash and merge!

README.md Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
@CloudNiner
Copy link
Contributor Author

Docs are fixed and build is green. Merging!

@CloudNiner CloudNiner merged commit 500eaab into master Nov 13, 2020
@CloudNiner CloudNiner deleted the feature/awf/macos-conda-lib branch November 13, 2020 18:25
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.

MacOS Catalina fails to load libgdal.27.dylib MacOS build assumes libgdal via Homebrew
4 participants