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

Build script version detection assumes you're not cross compiling #376

Closed
sfackler opened this issue Mar 3, 2023 · 11 comments
Closed

Build script version detection assumes you're not cross compiling #376

sfackler opened this issue Mar 3, 2023 · 11 comments

Comments

@sfackler
Copy link

sfackler commented Mar 3, 2023

The gdal crate declares gdal-sys as both a runtime and build-time dependency, and calls the gdal_version_docs_rs_wrapper function to determine the gdal version.

This logic is broken when cross compiling, since the host system's gdal won't be the same as the target system's gdal. A more robust approach would be to have gdal-sys output the detected version in its build script which can then be picked up by gdal's build script:

https://doc.rust-lang.org/cargo/reference/build-scripts.html#the-links-manifest-key
https://doc.rust-lang.org/cargo/reference/build-script-examples.html#using-another-sys-crate

@rmanoka
Copy link
Contributor

rmanoka commented Mar 3, 2023

Indeed, this was done to get it working on docs.rs which does a cross-compilation, and we wanted to be able to not rely on the host (docs.rs) having gdal installed. Ref. PR that introduced it for discussion. I believe the standard DOCS_RS flag approach was causing issues for the docs.rs builds of down-stream libraries.

Do you have a MWIP repo to test? I'm not an expert on cross comp.

@sfackler
Copy link
Author

sfackler commented Mar 3, 2023

The issue isn't related to the docs.rs stuff, just to the fact that this dependency exists at all.

@jdroenner
Copy link
Member

jdroenner commented Mar 3, 2023

Well, if there is a better way, let us use that.

@lnicola
Copy link
Member

lnicola commented Mar 3, 2023

@sfackler how can gdal know the version that gdal-sys detected without depending on it?

EDIT: looks like https://doc.rust-lang.org/cargo/reference/build-script-examples.html#conditional-compilation is exactly about this.

@sfackler
Copy link
Author

sfackler commented Mar 3, 2023

It currently depends on gdal-sys twice. This is the dependency that should stay: https://github.com/georust/gdal/blob/master/Cargo.toml#L23

@lnicola
Copy link
Member

lnicola commented Mar 3, 2023

So if I got this right:

  • gdal-sys needs a links = "gdal" clause and can output a cargo:version_number declaration
  • cargo makes sure that the gdal build script gets a DEP_GDAL_VERSION_NUMBER env variable with the value set by gdal-sys

That's quite nifty, I didn't know it was a thing.

@sfackler
Copy link
Author

sfackler commented Mar 3, 2023

Yep!

@rmanoka
Copy link
Contributor

rmanoka commented Mar 4, 2023

So this means we can also test the docs_rs variable somewhere and output the appropriate env. variables?

@jdroenner
Copy link
Member

i guess it should work like this: #377

@lnicola
Copy link
Member

lnicola commented Mar 7, 2023

Fixed in #377 🎉

@lnicola lnicola closed this as completed Mar 7, 2023
@sfackler
Copy link
Author

sfackler commented Mar 8, 2023

Thanks for the quick fix!

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

No branches or pull requests

4 participants