Skip to content

Depend on libsqlite3-sys for bundled builds #190

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.

Sign up for GitHub

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

Merged
michaelkirk merged 5 commits into from
Apr 24, 2024
Merged

Depend on libsqlite3-sys for bundled builds #190

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9a6b47d
Depend on `libsqlite3-sys` for bundled builds
weiznich Jan 25, 2024
44a5038
Do not enforce building libsqlite3 from source
weiznich Feb 26, 2024
afe9aed
Merge branch 'main' into feature/static_sqlite
weiznich Apr 2, 2024
37e73c9
Add a comment about the libsqlite3 handling
weiznich Apr 2, 2024
7ef1021
Fix typos
weiznich Apr 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions proj-sys/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ links = "proj"
rust-version = "1.70"

[dependencies]
libsqlite3-sys = { version = "0.28", features = ["bundled"] }
Copy link
Member

@michaelkirk michaelkirk Feb 12, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@weiznich weiznich Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

@weiznich weiznich Feb 22, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member

@michaelkirk michaelkirk Feb 22, 2024

Choose a reason for hiding this comment

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

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)?

Copy link
Contributor Author

@weiznich weiznich Feb 22, 2024

Choose a reason for hiding this comment

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

@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.

Copy link
Member

@urschrei urschrei Apr 2, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@weiznich weiznich Apr 2, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

@urschrei urschrei Apr 2, 2024

Choose a reason for hiding this comment

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

uses the statically build

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

Copy link
Contributor Author

@weiznich weiznich Apr 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@urschrei urschrei Apr 2, 2024

Choose a reason for hiding this comment

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

Great, thanks for clarifying!

link-cplusplus = "1.0"

[build-dependencies]
bindgen = "0.68.1"
Expand Down
21 changes: 9 additions & 12 deletions proj-sys/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
// returns the path of "include" for the built proj
fn build_from_source() -> Result<std::path::PathBuf, Box<dyn std::error::Error>> {
eprintln!("building libproj from source");
println!("cargo:rustc-cfg=bundled_build");
if let Ok(val) = &env::var("_PROJ_SYS_TEST_EXPECT_BUILD_FROM_SRC") {
if val == "0" {
panic!(
Expand All @@ -80,7 +81,6 @@ fn build_from_source() -> Result<std::path::PathBuf, Box<dyn std::error::Error>>
}
}

// NOTE: The PROJ build expects Sqlite3 to be present on the system.
let path = "PROJSRC/proj-9.3.1.tar.gz";
let out_path = PathBuf::from(env::var("OUT_DIR").unwrap());
let tar_gz = File::open(path)?;
Expand All @@ -98,6 +98,14 @@ fn build_from_source() -> Result<std::path::PathBuf, Box<dyn std::error::Error>>
config.define("BUILD_PROJINFO", "OFF");
config.define("BUILD_PROJSYNC", "OFF");
config.define("ENABLE_CURL", "OFF");
config.define(
"SQLITE3_INCLUDE_DIR",
std::env::var("DEP_SQLITE3_INCLUDE").expect("This is set by libsqlite3-sys"),
);
config.define(
"SQLITE3_LIBRARY",
format!("{}/libsqlite3.a", std::env::var("DEP_SQLITE3_LIB_DIR").unwrap()),
);

if cfg!(feature = "tiff") {
eprintln!("enabling tiff support");
Expand Down Expand Up @@ -132,9 +140,6 @@ fn build_from_source() -> Result<std::path::PathBuf, Box<dyn std::error::Error>>
&out_path.join("build/lib").display()
);

// The PROJ library needs SQLite and the C++ standard library.
println!("cargo:rustc-link-lib=dylib=sqlite3");

if cfg!(feature = "tiff") {
// On platforms like apples aarch64, users are likely to have installed libtiff with homebrew,
// which isn't in the default search path, so try to determine path from pkg-config
Expand All @@ -159,13 +164,5 @@ fn build_from_source() -> Result<std::path::PathBuf, Box<dyn std::error::Error>>
println!("cargo:rustc-link-lib=dylib=tiff");
}

if cfg!(target_os = "linux") {
println!("cargo:rustc-link-lib=dylib=stdc++");
} else if cfg!(target_os = "macos") {
println!("cargo:rustc-link-lib=dylib=c++");
} else {
println!("cargo:warning=proj-sys: Not configuring an explicit C++ standard library on this target.");
}

Ok(proj.join("include"))
}
5 changes: 5 additions & 0 deletions proj-sys/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

@michaelkirk michaelkirk Apr 2, 2024

Choose a reason for hiding this comment

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

Why do we need these extern crate declarations?

Copy link
Contributor Author

@weiznich weiznich Apr 2, 2024

Choose a reason for hiding this comment

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

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.

#[cfg(bundled_build)]
extern crate link_cplusplus;

#[cfg(not(feature = "nobuild"))]
include!(concat!(env!("OUT_DIR"), "/bindings.rs"));

Expand Down