Depend on `libsqlite3-sys` for bundled builds

weiznich commented 5 months ago

This commit adds an optional dependency to libsqlite3-sys to provide a bundled version of libsqlite3 as well instead of relying on a system provided version.

weiznich commented 5 months ago

What's required to move this forward?

urschrei commented 5 months ago

Could you address the CI failures?

weiznich commented 5 months ago

That's an interesting failure mode. It seems like the build script can decide at runtime to fall back building the libproj. That's problematic because we would need to know beforehand to whether to depend on libsqlite-sys or not. I see three solutions:

Remove the fallback and require setting a feature flag for bundled builds
Always depend and libsqlite-sys and always build the bundled variant of that library. I think we can say that we don't want to link libsqlite to the final binary, but the build overhead will always be there
Only use that approach when gathering explicit feature flag is set, for the fallback builds conserve the old behaviour (relying on a system provided libsqlite3)

I'm not sure which way would be preferred.

weiznich commented 5 months ago

I've pushed an update that should pass CI.

urschrei commented 5 months ago

Great, thank you! I had no idea link-cplusplus existed. I don't see any obvious downsides to this (and it may also get us a bit closer to wasm builds?), so I'm happy to merge once at least one of the other two requested reviewers has a look.

weiznich commented 4 months ago

Is there any interest in moving this PR forward from the relevant maintainers?

urschrei commented 3 months ago

@michaelkirk @lnicola Given the changes in https://github.com/georust/proj/commit/44a503869709ea793a5f8452bfb9e2a99b7160fc, what do you think? It feels like there's progress being made on https://github.com/georust/gdal/pull/517, so we're now in a chicken-and-egg situation in which the GDAL crate work requires this PR to merge, but I'm hesitant to merge unless it's at least likely that it'll land.

lnicola commented 3 months ago

The interaction between this and libsqlite3-sys seems quite subtle, but probably the best solution we can find. It might be worth adding a comment about it?

but I'm hesitant to merge unless it's at least likely that it'll land

I hope the GDAL PR lands. It doesn't have the drivers I use most, but it can be expanded by making -src crates for those in the future. And it's really useful today, especially for vector processing and users who are fine with the basic GeoTIFF support.

michaelkirk commented 3 months ago

I think it's weird to build sqlite3 even though we might not use it. It's not a huge problem though, and apparently this is unavoidable. I'm neutral on this PR. If someone wants to advocate for merging it that's fine with me.

urschrei commented 3 months ago

I think it's weird to build sqlite3 even though we might not use it. It's not a huge problem though, and apparently this is unavoidable. I'm neutral on this PR. If someone wants to advocate for merging it that's fine with me.

I think that we now don't build it if it's not required:

https://github.com/georust/proj/pull/190#discussion_r1547636023

weiznich commented 3 months ago

I think it's weird to build sqlite3 even though we might not use it. It's not a huge problem though, and apparently this is unavoidable. I'm neutral on this PR. If someone wants to advocate for merging it that's fine with me.

I think that we now don't build it if it's not required:

It's a bit confusing but it's essentially as follows:

We always build the rust crate libsqlite3-sys
Downstream users might configure libsqlite3-sys to build the c library libsqlite3, in which case we link that library

michaelkirk commented 3 months ago

It's a bit confusing but it's essentially as follows:

We always build the rust crate libsqlite3-sys Downstream users might configure libsqlite3-sys to build the c library libsqlite3, in which case we link that library

Ah, actually that resolves my concerns. Thanks for your patience @weiznich!

Some small typos and a question about the use of extern, but otherwise LGTM

weiznich commented 2 months ago

@michaelkirk Is there anything that I can do to move this PR forward?

urschrei commented 2 months ago

I'm happy to merge!

michaelkirk commented 2 months ago

The CI failure seems spurious - the floating point on macos are off by a very small amount. It might be related to GH switching to arm64 (macos-14-arm64). I'll open a PR momentarily to address the tests and then we can re-run this one.

update: Ah yes, they seems to have switched the default runner to arm64 in the last few weeks.

Last successful build 3 weeks ago https://github.com/georust/proj/actions/runs/8562584782/job/23466083178:

Runner Image
  Image: macos-12

vs. today:

Runner Image
  Image: macos-14-arm64

georust / proj

Depend on `libsqlite3-sys` for bundled builds #190