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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prebuilt bindings for GDAL 3.5 #277

Merged
merged 3 commits into from Jun 23, 2022
Merged

Conversation

MrMuetze
Copy link

Hey 馃憢

Here is my attempt to provide the prebuilt bindings for GDAL 3.5. I had a look at the previous PR for GDAL 3.4 and I hope I did everything in a similar way 馃槄

Please let me know if any changes need to be made.

@rmanoka
Copy link
Contributor

rmanoka commented Jun 19, 2022

This looks great @MrMuetze .

Could you also adjust the CI yml file to use the ubuntu-full-3.5.0 image ( .github/workflows/ci.yml ). We should essentially copy the 3.4 recipe and replace the image and names appropriately. We could remove the CI for the least recent version 3.1.0 instead.

@MrMuetze
Copy link
Author

MrMuetze commented Jun 20, 2022

I will have a look at this 馃憤

Edit: I have (hopefully) pushed the changes you mentioned :)

and removing the job for gdal 3.1
@rmanoka
Copy link
Contributor

rmanoka commented Jun 20, 2022

Hmm apparently, GDAL now uses versions like: GDAL 3.5.0.2, but our build-script fails to parse it (because the semver crate doesn't seem to support it). See error message copied from the CI:

thread 'main' panicked at 'Could not parse gdal version!: Error("unexpected character '.' after patch version number")', build.rs:27:61

@MrMuetze
Copy link
Author

Would it be an option to move away from semver and just parse the version bits on our own? Something like this possibly (I just threw this together, I'm not sure if u8 is ideal for version numbers.

use std::str::FromStr;

fn main() {
    let version_string = "3.5.0.2";
    let version_parts: Vec<u8> = version_string
        .split(".")
        .map(u8::from_str)
        .collect::<Result<Vec<_>, _>>()
        .expect("we convert numbers");
    dbg!(version_parts);
    
    // check for major version
    // version_parts[0] < 2
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=a1d8d8b50021881cc4a1704c665144b6

@rmanoka
Copy link
Contributor

rmanoka commented Jun 20, 2022

Good idea. To make it a bit more comprehensive: running git tag -l on fresh clone of osgeo/gdal suggests we should try to support these strings:

1.1.5
1.1.6
1.1.7
1.1.8
1.1.9
1.10.0
1.10.1
1.11.0
1.11.1
1.11.2
1.11.3
1.11.4
1.11.5
1.2.0
1.2.1
1.2.2
1.2.3
1.2.4
1.2.5
1.2.6
1.3.0
1.3.1
1.3.2
1.4.0
1.4.1
1.4.2
1.4.3
1.4.4
1.4.5
1.5.0
1.5.1
1.5.2
1.5.3
1.5.4
1.6.0
1.6.1
1.6.2
1.6.3
1.7.0
1.7.1
1.7.2
1.7.3
1.8.0
1.8.1
1.9.0
1.9.1
1.9.2
2.0.0
2.0.1
2.0.2
2.0.3
2.1.0
2.1.1
2.1.2
2.1.3
2.1.4
2.2.0
2.2.1
2.2.2
2.2.3
2.2.4
2.3.0
2.3.1
2.3.2
2.3.3
2.4.0
2.4.1
2.4.2
2.4.3
2.4.3RC1
2.4.4
2.4.4RC1
3.0.0
3.0.1
3.0.2
3.0.2RC1
3.0.3
3.0.3RC1
3.0.4
3.0.4RC1
3.1.0
3.1.0RC1
3.1.0RC2
3.1.0RC3
3.1.1
3.1.1RC1
3.1.2
3.1.3
3.1.3RC1
3.1.4
3.1.4RC1
3.1.4RC2
3.2.0
3.2.0RC1
3.2.1
3.2.1RC1
3.2.2
3.2.2.1
3.2.2RC1
3.2.3
3.2.3RC1
3.3.0
3.3.0RC1
3.3.0beta1
3.3.1
3.3.1RC1
3.3.2
3.3.2RC1
3.3.2RC2
3.3.2RC3
3.3.3
3.3.3RC1
3.4.0
3.4.0RC1
3.4.0RC2
3.4.0RC3
3.4.1
3.4.1RC1
3.4.2
3.4.2RC1
3.4.2RC2
3.4.3
3.4.3RC1
3.4.3RC2
3.5.0
3.5.0.1
3.5.0.2
3.5.0RC1
3.5.0RC2
3.5.0RC3
3.5.0RC4

@MrMuetze can you verify your logic can parse all of these? We seem to require a numeric major, minor, patch for the way we version control features.

@MrMuetze
Copy link
Author

MrMuetze commented Jun 20, 2022

My approach would not be able to support this kind of prerelease string (for betas or release candidates), but I don't think the semver crate did that either. 馃 I did a quick check right now and semver would expect something like this: 3.5.0-RC0. It fails with 3.5.0RC0. (Error("unexpected character 'R' after patch version number"))

Do you think we need to be able to parse beta or release candidate versions? If not, my approach would be sufficient and I could incorporate the changes. If not, we have to figure out something else.

Edit: But all the numeric versions should be supported. 馃憤

@rmanoka
Copy link
Contributor

rmanoka commented Jun 20, 2022

Makes sense @MrMuetze ; I'm just seeing we can ensure better robustness. I looked into the osgeo gdal source code. Most apps in the repo seem to use GDAL_VERSION_NUM which is a numeric format of the version. It is computed as follows:

/* GDAL_COMPUTE_VERSION macro introduced in GDAL 1.10 */
/* Must be used ONLY to compare with version numbers for GDAL >= 1.10 */
#ifndef GDAL_COMPUTE_VERSION
#define GDAL_COMPUTE_VERSION(maj,min,rev) ((maj)*1000000+(min)*10000+(rev)*100)
#endif

/* Note: the formula to compute GDAL_VERSION_NUM has changed in GDAL 1.10 */
#ifndef GDAL_VERSION_NUM
#  define GDAL_VERSION_NUM      (GDAL_COMPUTE_VERSION(GDAL_VERSION_MAJOR,GDAL_VERSION_MINOR,GDAL_VERSION_REV)+GDAL_VERSION_BUILD)
#endif

The formula seems to be the same since at least 2.0.0, and the value is available as a string from the API by calling gdal_version_info("VERSION_NUM") in build.rs. I think this would be a better approach. Any thoughts @michaelkirk @lnicola @jdroenner ?

@MrMuetze
Copy link
Author

Yes if we can get something semver compatible from GDAL that would be the way to go! 馃憤

@rmanoka
Copy link
Contributor

rmanoka commented Jun 23, 2022

@MrMuetze let's go ahead the VERSION_NUM approach. Could you modify gdal/build.rs to use this approach?

@MrMuetze
Copy link
Author

MrMuetze commented Jun 23, 2022

I can give it a go 馃憤

Edit: Not sure if my dissection of the version string is the cleanest, but it should work 馃槄

this should be a more robust approach
@rmanoka
Copy link
Contributor

rmanoka commented Jun 23, 2022

Thanks @MrMuetze !

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 23, 2022

Build succeeded:

@bors bors bot merged commit 69e77cc into georust:master Jun 23, 2022
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