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

weiznich · 2024-01-26T07:19:59Z

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.

Cargo.toml

weiznich · 2024-02-09T10:00:00Z

What's required to move this forward?

urschrei · 2024-02-09T17:30:38Z

Could you address the CI failures?

weiznich · 2024-02-09T19:11:50Z

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.

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. Also use the `link_cplusplus` crate to handling linking libc++ for us.

weiznich · 2024-02-12T14:22:21Z

I've pushed an update that should pass CI.

urschrei · 2024-02-12T14:58:14Z

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.

michaelkirk · 2024-02-12T19:57:33Z

proj-sys/Cargo.toml

@@ -12,6 +12,8 @@ links = "proj"
 rust-version = "1.70"

 [dependencies]
+libsqlite3-sys = { version = "0.28", features = ["bundled"] }


Based on your description, I was expecting this to be an optional dependency. What am I misunderstanding?

It's unfortunately not possible to keep that as optional dependency as the build script can decide at runtime to build libproj from source. See here for more details.

(It's currently configured that way that it only links libsqlite3 if we build from source, that's why the extern crate libsqlite3-sys line is behind a cfg flag).

@michaelkirk Does this answer your question? Or would you like something to be changed here?

Well ideally we wouldn't be compiling a thing that we don't need. I understand that it's complicated though.

Will libsqlite3-sys build sqlite3 even if it's already been installed on the system (e.g. via a package manager)?

@michaelkirk Yes with the bundled feature flag enabled libsqlite3-sys will always build libsqlite3 from source and statically link it. They do not provide a option that performs runtime detection whether the crate is installed or not and that falls back to compiling if the crate is not found. That means with the current version we will always build libsqlite3 from source, but we will only link that version if we also build libproj from source.

That written: I had another idea how we could maybe solve that problem in a better way. We could not enable the bundled feature flag in proj-sys at all. We can then use the DEP_SQLITE3_* variables to check at runtime whether or not the libsqlite3-sys crate build a libsqlite3 version from source by checking if these environment variables exist. That would allow users to control how libsqlite3 should be linked, as they then can control the bundled flag there on their own as feature flags are additive. If these variables are not set we would fall back to trying to get a system version of libsqlite3 for the build, which is the current behavior. If that sounds more acceptable I would change the PR to use that approach instead.

@weiznich Just so I'm clear: there's now no unneeded sqlite build unless the bundled feature is enabled?

Yes, I changed it that way that it only uses the statically build libsqlite3 version if the user specified somewhere else in their dependency tree that libsqlite3 should be bundled.

uses the statically build

So it still builds sqlite every time, but discards the static build if it's not needed?

No, it doesn't configure the bundled build for sqlite at all.

If we reach the stage where we build libproj from source is just checks if someone else has configured that libsqlite3-sys should build libsqlite3 from source and if that's the case it will use these build artifacts. That check is done by these two DEP_* variables, which are set by libsqlite3-sys for the bundled build. For that to work proj-sys needs to depend on libsqlite3-sys (otherwise we won't see the variables), but that shouldn't be a problem as that only runs the build script of that crate, which by default will only handle that libsqlite3 is linked to the binary, not that it is build from source.

So we likely do the following unnecessary steps of work now for proj-sys for the not bundled case:

Building the build script of libsqlite3-sys (not libsqlite3 itself, just the rust side build.rs file)

Informing the linker that we want to link libsqlite3, although that might be unnecessary for cases where we dynamically link libproj.

Great, thanks for clarifying!

Instead we now just check if it was build from source (by checking the relevant environment variables) and if that was the case we link that version statically.

weiznich · 2024-03-15T10:05:23Z

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

urschrei · 2024-04-02T11:11:20Z

@michaelkirk @lnicola Given the changes in 44a5038, what do you think? It feels like there's progress being made on georust/gdal#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 · 2024-04-02T11:41:56Z

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 · 2024-04-02T13:57:35Z

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 · 2024-04-02T14:00:15Z

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:

#190 (comment)

weiznich · 2024-04-02T14:17:32Z

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

proj-sys/build.rs

michaelkirk · 2024-04-02T14:35:47Z

proj-sys/src/lib.rs

@@ -28,6 +28,11 @@
 //! implement your own set of callbacks if you wish to make use of them (see the
 //! [`proj`](https://crates.io/crates/proj) crate for an example).

+#[cfg(bundled_build)]
+extern crate libsqlite3_sys;


Why do we need these extern crate declarations?

Rustc only links crates that are explictly used somewhere, either by actually use items from that crate or by having an explicit extern crate … statement somewhere. So this essentially ensures that we actually link libsqlite3 (the c library) for the bundled build case. We set the cfg flag from the build script if we actually build libproj from source and statically link it. Only then we need to also link libsqlite3.

michaelkirk · 2024-04-02T14:36:52Z

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

Co-authored-by: Michael Kirk <michael.code@endoftheworl.de>

weiznich · 2024-04-23T18:10:43Z

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

michaelkirk

Sorry for letting this slip and thanks for the prod @weiznich.

I'm OK to merge if @urschrei is.

proj-sys/build.rs

urschrei · 2024-04-23T19:48:57Z

I'm happy to merge!

michaelkirk · 2024-04-23T20:13:03Z

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

weiznich commented Jan 26, 2024

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

weiznich mentioned this pull request Jan 26, 2024

Provide a bundled flag for gdal-sys georust/gdal#517

Open

2 tasks

weiznich force-pushed the feature/static_sqlite branch 2 times, most recently from 1f714f1 to 6120416 Compare January 30, 2024 08:11

Depend on libsqlite3-sys for bundled builds

9a6b47d

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. Also use the `link_cplusplus` crate to handling linking libc++ for us.

weiznich force-pushed the feature/static_sqlite branch from 6120416 to 9a6b47d Compare February 12, 2024 14:21

urschrei requested review from michaelkirk and lnicola February 12, 2024 14:57

michaelkirk reviewed Feb 12, 2024

View reviewed changes

Do not enforce building libsqlite3 from source

44a5038

Instead we now just check if it was build from source (by checking the relevant environment variables) and if that was the case we link that version statically.

Merge branch 'main' into feature/static_sqlite

afe9aed

lnicola approved these changes Apr 2, 2024

View reviewed changes

Add a comment about the libsqlite3 handling

37e73c9

michaelkirk reviewed Apr 2, 2024

View reviewed changes

Fix typos

7ef1021

Co-authored-by: Michael Kirk <michael.code@endoftheworl.de>

michaelkirk approved these changes Apr 23, 2024

View reviewed changes

proj-sys/build.rs Show resolved Hide resolved

michaelkirk added this pull request to the merge queue Apr 23, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 23, 2024

michaelkirk added this pull request to the merge queue Apr 24, 2024

Merged via the queue into georust:main with commit 4a0e203 Apr 24, 2024
27 checks passed

kkk25641463 mentioned this pull request May 22, 2024

feat(query): implement ST_YMAX/ST_XMIN/ST_YMIN datafuselabs/databend#15594

Open

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

weiznich commented Jan 26, 2024

weiznich commented Feb 9, 2024

urschrei commented Feb 9, 2024

weiznich commented Feb 9, 2024

weiznich commented Feb 12, 2024

urschrei commented Feb 12, 2024

michaelkirk Feb 12, 2024

weiznich Feb 13, 2024 •

edited

weiznich Feb 22, 2024

michaelkirk Feb 22, 2024

weiznich Feb 22, 2024

urschrei Apr 2, 2024

weiznich Apr 2, 2024

urschrei Apr 2, 2024

weiznich Apr 2, 2024 •

edited

urschrei Apr 2, 2024

weiznich commented Mar 15, 2024

urschrei commented Apr 2, 2024

lnicola commented Apr 2, 2024

michaelkirk commented Apr 2, 2024

urschrei commented Apr 2, 2024

weiznich commented Apr 2, 2024

michaelkirk Apr 2, 2024

weiznich Apr 2, 2024

michaelkirk commented Apr 2, 2024

weiznich commented Apr 23, 2024

michaelkirk left a comment

urschrei commented Apr 23, 2024

michaelkirk commented Apr 23, 2024 •

edited

Depend on libsqlite3-sys for bundled builds #190

Depend on libsqlite3-sys for bundled builds #190

Conversation

weiznich commented Jan 26, 2024

weiznich commented Feb 9, 2024

urschrei commented Feb 9, 2024

weiznich commented Feb 9, 2024

weiznich commented Feb 12, 2024

urschrei commented Feb 12, 2024

Choose a reason for hiding this comment

weiznich Feb 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiznich Apr 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiznich commented Mar 15, 2024

urschrei commented Apr 2, 2024

lnicola commented Apr 2, 2024

michaelkirk commented Apr 2, 2024

urschrei commented Apr 2, 2024

weiznich commented Apr 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkirk commented Apr 2, 2024

weiznich commented Apr 23, 2024

michaelkirk left a comment

Choose a reason for hiding this comment

urschrei commented Apr 23, 2024

michaelkirk commented Apr 23, 2024 • edited

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

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

weiznich Feb 13, 2024 •

edited

weiznich Apr 2, 2024 •

edited

michaelkirk commented Apr 23, 2024 •

edited