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

Fix source build on aarch64/homebrew #95

Merged
merged 2 commits into from Nov 29, 2021
Merged

Conversation

michaelkirk
Copy link
Member

Slightly improves the situation at #57

On apple platforms, libtiff is not installed by the operating system. If
the user has it installed, likely it was installed by homebrew.

Previously, on x86, homebrew installed libs into /usr/lib which is in
the default search path, but since aarch64, homebrew has moved libs to
/opt/homebrew/lib.

So now we use pkg-config to find the library.

Potential problems:

  • Not everyone has pkg-config (e.g windows users, a few mac users)
  • Not everyone actually needs libtiff, only the network geotiff users,
    but we assume everyone needs it since most system installations
    include it.

Note that proj enables tiff support by defeault, provided libtiff is
found (using pkg-config? or something else?)

This is a draft because some tests are currently failing for me on aarch64. I'm looking into it now...

failing test output
geos: Invalid latitude
geos: Invalid latitude
"proj=pipeline step proj=unitconvert xy_in=us-ft xy_out=m step inv proj=lcc lat_0=32.1666666666667 lon_0=-116.25 lat_1=33.8833333333333 lat_2=32.7833333333333 x_0=2000000.0001016 y_0=500000.0001016 ellps=GRS80 step proj=lcc lat_0=32.1666666666667 lon_0=-116.25 lat_1=33.8833333333333 lat_2=32.7833333333333 x_0=2000000 y_0=500000 ellps=GRS80"
proj_create: unrecognized format / unknown name

assert_relative_eq!(t.x(), 0.43633200013698786)

left  = NaN
right = 0.43633200013698786

thread 'proj::test::test_inverse_projection' panicked at 'assert_relative_eq!(t.x(), 0.43633200013698786)

left  = NaN
right = 0.43633200013698786

', src/proj.rs:1069:9
stack backtrace:
0: rust_begin_unwind
at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/std/src/panicking.rs:517:5
1: std::panicking::begin_panic_fmt
at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/std/src/panicking.rs:460:5
2: proj::proj::test::test_inverse_projection
at ./src/proj.rs:1069:9
3: proj::proj::test::test_inverse_projection::{{closure}}
at ./src/proj.rs:1059:5
4: core::ops::function::FnOnce::call_once
at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/core/src/ops/function.rs:227:5
5: core::ops::function::FnOnce::call_once
at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with RUST_BACKTRACE=full for a verbose backtrace.

assert_relative_eq!(t.x(), 0.0023755864830313977)

left  = NaN
right = 0.0023755864830313977

thread 'proj::test::test_london_inverse' panicked at 'assert_relative_eq!(t.x(), 0.0023755864830313977)

left  = NaN
right = 0.0023755864830313977

', src/proj.rs:1086:9
stack backtrace:
0: rust_begin_unwind
at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/std/src/panicking.rs:517:5
1: std::panicking::begin_panic_fmt
at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/std/src/panicking.rs:460:5
2: proj::proj::test::test_london_inverse
at ./src/proj.rs:1086:9
3: proj::proj::test::test_london_inverse::{{closure}}
at ./src/proj.rs:1074:5
4: core::ops::function::FnOnce::call_once
at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/core/src/ops/function.rs:227:5
5: core::ops::function::FnOnce::call_once
at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with RUST_BACKTRACE=full for a verbose backtrace.

assert_relative_eq!(t.x(), 500119.7035366755, epsilon = 1e-5)

left  = NaN
right = 500119.7035366755

thread 'proj::test::test_projection' panicked at 'assert_relative_eq!(t.x(), 500119.7035366755, epsilon = 1e-5)

left  = NaN
right = 500119.7035366755

', src/proj.rs:1054:9
stack backtrace:
0: rust_begin_unwind
at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/std/src/panicking.rs:517:5
1: std::panicking::begin_panic_fmt
at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/std/src/panicking.rs:460:5
2: proj::proj::test::test_projection
at ./src/proj.rs:1054:9
3: proj::proj::test::test_projection::{{closure}}
at ./src/proj.rs:1044:5
4: core::ops::function::FnOnce::call_once
at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/core/src/ops/function.rs:227:5
5: core::ops::function::FnOnce::call_once
at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with RUST_BACKTRACE=full for a verbose backtrace.

thread 'proj::test::test_searchpath' panicked at 'assertion failed: (left == right)
left: "/Users/mkirk/src/georust/proj/target/debug/build/proj-sys-1e3a03028e94a7d6/out/share/proj",
right: "/foo"', src/proj.rs:1016:9
stack backtrace:
0: rust_begin_unwind
at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/std/src/panicking.rs:517:5
1: core::panicking::panic_fmt
at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/core/src/panicking.rs:101:14
2: core::panicking::assert_failed_inner
3: core::panicking::assert_failed
at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/core/src/panicking.rs:140:5
4: proj::proj::test::test_searchpath
at ./src/proj.rs:1016:9
5: proj::proj::test::test_searchpath::{{closure}}
at ./src/proj.rs:1010:5
6: core::ops::function::FnOnce::call_once
at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/core/src/ops/function.rs:227:5
7: core::ops::function::FnOnce::call_once
at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with RUST_BACKTRACE=full for a verbose backtrace.

On apple platforms, libtiff is not installed by the operating system. If
the user has it installed, likely it was installed by homebrew.

Previously, on x86, homebrew installed libs into /usr/lib which is in
the default search path, but since aarch64, homebrew has moved libs to
/opt/homebrew/lib.

So now we use pkg-config to find the library.

Potential problems:

 - Not everyone has pkg-config (e.g windows users, a few mac users)
 - Not everyone actually needs libtiff, only the network geotiff users,
   but we assume everyone needs it since most system installations
   include it.

Note that proj enables tiff support by defeault, provided libtiff is
found (using pkg-config? or something else?)
// in some default search path.
eprintln!("Failed to find libtiff with pkg-config: {}", err);
}
}
println!("cargo:rustc-link-lib=dylib=tiff");
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm having second thoughts about some of my changes in 9aca22c 😬

Assuming that the system proj has tiff was a nice way to avoid building from src in more circumstances, but we're now also requiring libtiff whenever building from source.

With a little work we might be able to accommodate all these cases...

Copy link
Member Author

Choose a reason for hiding this comment

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

After typing it out, I was motivated to just address this. With b0f4474 we only require tiff when using the network feature, which I've now forwarded from proj to proj/sys.

It's almost like the old bunled_proj_tiff, except that we'll still allow a system proj to satisfy the requirement, assuming that it has tiff support.

We could add some kind of check to build.rs to make sure the linked libproj supports tiff, but I'm inclined to wait and see if it's actually a problem.

If we wanted to require pkg-config, one (not very impressive) approach would be parsing the output of pkg-config --libs --static proj for libtiff.

Copy link
Member

Choose a reason for hiding this comment

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

IIRC, kornel's Rust -sys library guide notes that pkg-config can be quite fussy. Homebrew's proj installs pkg-config but of course we can't make that assumption w/r/t/ Linux, so if we can do without it that's great.

@michaelkirk
Copy link
Member Author

bors try

I'm curious if the failing tests are just an aarch64 thing...

bors bot added a commit that referenced this pull request Nov 29, 2021
@michaelkirk
Copy link
Member Author

Oh yeah! Looks like tests have passed. I'm inclined to propose merging this as is, and leave the 3 failing tests as a separate change. I notice that I get different results when linking against proj 8.2 vs building proj 8.1 from source.

@michaelkirk michaelkirk marked this pull request as ready for review November 29, 2021 20:21
@bors
Copy link
Contributor

bors bot commented Nov 29, 2021

try

Build succeeded:

@urschrei
Copy link
Member

I notice that I get different results when linking against proj 8.2 vs building proj 8.1 from source.

It's reasonable to assume that it's a db update causing NaN values (as has been the case in the past) due to bounding box updates.

@michaelkirk
Copy link
Member Author

michaelkirk commented Nov 29, 2021

IIRC, kornel's Rust -sys library guide notes that pkg-config can be quite fussy. Homebrew's proj installs pkg-config but of course we can't make that assumption w/r/t/ Linux, so if we can do without it that's great.

Oh! For some reason I assumed all the linux distros relied on pkg-config and that it was just the mac and windows people that might be missing it.

Here's my proposal, if you're OK with it:

  • Let's merge as is.
  • If we hear of people getting tif linker errors when using their pre-existing system proj, we can investigate how to improve the situation at that time with real world use cases. All the default proj installations I could find include tiff, so it shouldn't be common.

@urschrei
Copy link
Member

That sounds good to me!

@michaelkirk
Copy link
Member Author

bors r=urschrei

@bors
Copy link
Contributor

bors bot commented Nov 29, 2021

Build succeeded:

@bors bors bot merged commit 5c1594f into master Nov 29, 2021
@frewsxcv frewsxcv deleted the mkirk/homebrew-vs-libtiff branch February 5, 2022 18:21
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.

None yet

2 participants