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

libtiff: Add LERC support #290556

Merged
merged 1 commit into from
Mar 22, 2024
Merged

libtiff: Add LERC support #290556

merged 1 commit into from
Mar 22, 2024

Conversation

l0b0
Copy link
Contributor

@l0b0 l0b0 commented Feb 22, 2024

Description of changes

Fixes #238882.

Based on Improve build doc when building gdal with LERC lib and the GDAL GeoTIFF docs.

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.

@l0b0 l0b0 requested a review from imincik February 22, 2024 04:04
@l0b0 l0b0 marked this pull request as draft February 22, 2024 05:17
@l0b0 l0b0 changed the base branch from master to staging February 22, 2024 05:18
@l0b0 l0b0 marked this pull request as ready for review February 22, 2024 05:18
@l0b0 l0b0 mentioned this pull request Feb 22, 2024
13 tasks
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.

I confirm this PR is adding LERC support for gdal.

nix-build -A gdal

./result/bin/gdal_create -outsize 10 10 -co COMPRESS=LERC_DEFLATE test.tif

./result/bin/gdalinfo test.tif
Driver: GTiff/GeoTIFF
Files: test.tif
Size is 10, 10
Image Structure Metadata:
  COMPRESSION=LERC_DEFLATE
  INTERLEAVE=BAND
  LERC_VERSION=2.4
Corner Coordinates:
Upper Left  (    0.0,    0.0)
Lower Left  (    0.0,   10.0)
Upper Right (   10.0,    0.0)
Lower Right (   10.0,   10.0)
Center      (    5.0,    5.0)
Band 1 Block=10x10 Type=Byte, ColorInterp=Gray

@@ -63,6 +64,7 @@ stdenv.mkDerivation rec {

# TODO: opengl support (bogus configure detection)
propagatedBuildInputs = [
lerc
Copy link
Member

Choose a reason for hiding this comment

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

Why does it need to be propagated?

Copy link
Contributor

Choose a reason for hiding this comment

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

It wasn't clear to us as well.

From GDAL documentation

"""
LERC and LERC_DEFLATE are available only when using internal libtiff for GDAL < 3.3.0. Since GDAL 3.3.0, LERC compression is also available when building GDAL against external libtiff >= 4.3.0, built itself against https://github.com/esri/lerc
"""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assumed that since all the other libraries were already propagated, this one would have to be. It took a long time to build libtiff.passthru.tests, and it worked, so I didn't try anything else.

As for which libraries need propagating, why would any of them need to be? I can try changing the whole list to buildInputs.

Copy link
Member

@alyssais alyssais Feb 26, 2024

Choose a reason for hiding this comment

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

I'm not sure why the current list is propagated, but we shouldn't add new propagated inputs without having a good reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

True, I understand what you are saying now.

Copy link
Contributor

Choose a reason for hiding this comment

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

With

diff --git a/pkgs/development/libraries/libtiff/default.nix b/pkgs/development/libraries/libtiff/default.nix
index 1614748afcb1..e087e861a06b 100644
--- a/pkgs/development/libraries/libtiff/default.nix
+++ b/pkgs/development/libraries/libtiff/default.nix
@@ -63,7 +63,7 @@ stdenv.mkDerivation rec {
   nativeBuildInputs = [ autoreconfHook pkg-config sphinx ];

   # TODO: opengl support (bogus configure detection)
-  propagatedBuildInputs = [
+  buildInputs = [
     lerc
     libdeflate
     libjpeg

build of libgeotiff fails with

configure: error: cannot find zlib.h, bailing out

Copy link
Contributor

Choose a reason for hiding this comment

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

If I add zlib in to libgeotiff nativeBuildInputs

diff --git a/pkgs/development/libraries/libgeotiff/default.nix b/pkgs/development/libraries/libgeotiff/default.nix
index 9b160e6b29e8..bf2d6a42dbc3 100644
--- a/pkgs/development/libraries/libgeotiff/default.nix
+++ b/pkgs/development/libraries/libgeotiff/default.nix
@@ -32,6 +32,7 @@ stdenv.mkDerivation rec {
   nativeBuildInputs = [
     autoreconfHook
     pkg-config
+    zlib
   ];

   buildInputs = [ libtiff proj ];

build of libgeotiff will finish successfully.

@alyssais Looks like confirmation of negative impact of propagatedBuildInputs in libtiff which we should fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, zlib should go to buildInputs of libgeotiff.

Copy link
Contributor

Choose a reason for hiding this comment

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

Draft PR to fix propagation of libtiff build inputs created #291657

Copy link
Contributor Author

@l0b0 l0b0 Mar 5, 2024

Choose a reason for hiding this comment

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

I've changed lerc to a buildInput. Does it make sense to merge this before #292568, or should they go in together?

@l0b0 l0b0 force-pushed the libtiff-lerc-support branch 2 times, most recently from 767b3d0 to 8a61a55 Compare March 5, 2024 19:42
l0b0 added a commit to linz/topo-imagery that referenced this pull request Mar 6, 2024
l0b0 added a commit to linz/topo-imagery that referenced this pull request Mar 6, 2024
l0b0 added a commit to linz/topo-imagery that referenced this pull request Mar 7, 2024
@l0b0 l0b0 requested review from alyssais and imincik March 9, 2024 21:29
l0b0 added a commit to linz/topo-imagery that referenced this pull request Mar 11, 2024
l0b0 added a commit to linz/topo-imagery that referenced this pull request Mar 13, 2024
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.

I re-tested current state of this PR (with lerc added to buildInputs) and I confirm that it works for adding LERC support for GDAL

nix-build -A gdal

./result/bin/gdal_create -outsize 10 10 -co COMPRESS=LERC_DEFLATE test.tif

./result/bin/gdalinfo test.tif | grep LERC
  COMPRESSION=LERC_DEFLATE
  LERC_VERSION=2.4

I think this PR is good to be merged and is a low risk to all libtiff reverse dependencies.

@imincik imincik merged commit 23c5b0d into NixOS:staging Mar 22, 2024
22 of 23 checks passed
@l0b0 l0b0 deleted the libtiff-lerc-support branch March 22, 2024 21:38
l0b0 added a commit to linz/topo-imagery that referenced this pull request Mar 25, 2024
Remove and bump nixpkgs once
<NixOS/nixpkgs#290556> is at least in master,
ideally in nixos-unstable <https://nixpk.gs/pr-tracker.html?pr=290556>.
l0b0 added a commit to linz/topo-imagery that referenced this pull request Apr 17, 2024
Remove and bump nixpkgs once
<NixOS/nixpkgs#290556> is at least in master,
ideally in nixos-unstable <https://nixpk.gs/pr-tracker.html?pr=290556>.
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.

Compression support missing from GDAL GeoTIFF driver
5 participants