Feature gate versions #81

jdroenner · 2020-07-17T08:32:54Z

This PR restructures the gdal crate into folders related to GDAL versions.
The idea is to keep common (GDAL ~2.0) functionality in the gdal_common folder and add new functionality in the version folders.
To enable this approach the base types (pointer wrapping structs) are separated from the methods which are implemented in traits.
One example is in the spatial_ref folder: The SpatialRef struct is in gdal_common::spatial_ref and methods are implemented in the SpatialRefCommon trait. Additionally, new methods (GDAL 3.0) are added in gdal_3_0::spatial_ref with the SpatialRef_3_0 trait.

I think this approach works well and enables to add (and remove) methods for GDAL 2 and 3.
One open question is how to handle enums with different fields: a) add #[cfg()} in the enum or b) put the enums into the versioned folders and use the same name.

Other changes:

moved all exports to the lib level
unified Dataset and Driver (raster and vector datasets are the same in GDAL >= 2.0)
** vector datasets now also return Layer not &Layer -> Layer now keeps a ref to Dataset
renamed Buffer to RasterBuffer
removed deprecated versions
changed Dataset::open to use GDALOpenEx

ChristianBeilschmidt · 2020-07-18T12:41:18Z

src/lib.rs

+#[cfg(any(feature = "gdal_2_2", feature = "gdal_2_3", feature = "gdal_2_4"))]
+pub mod gdal_2_2;
+#[cfg(any(feature = "gdal_2_2", feature = "gdal_2_3", feature = "gdal_2_4"))]
+pub use gdal_2_2::*;
+
+#[cfg(any(feature = "gdal_2_3", feature = "gdal_2_4"))]
+pub mod gdal_2_3;
+#[cfg(any(feature = "gdal_2_3", feature = "gdal_2_4"))]
+pub use gdal_2_3::*;


I am confused about this part. So why do you import gdal 2.3 stuff for gdal 2.4?
I though all common implementation is done in common.

There is functionality which is exclusive to the 2.x or 3.x branches, so its not common. However, i'm not aware of functionality in gdal 2.x not available in gdal 2.y where y > x.

michaelkirk · 2020-09-01T16:35:01Z

Just a note for what other folks have done.

The openssl crate went down this road of per-version feature flags for a while, but ultimately landed on an approach utilizing build.rs to pass in configuration.

see: sfackler/rust-openssl#852
resolved here: https://github.com/sfackler/rust-openssl/pull/879/files

A snippet from their build.rs:

 if let Ok(version) = env::var("DEP_OPENSSL_VERSION_NUMBER") {
        let version = u64::from_str_radix(&version, 16).unwrap();

        if version >= 0x1_00_01_00_0 {
            println!("cargo:rustc-cfg=ossl101");
        }
        if version >= 0x1_00_02_00_0 {
            println!("cargo:rustc-cfg=ossl102");
        }
        if version >= 0x1_01_00_00_0 {
            println!("cargo:rustc-cfg=ossl110");
        }
        if version >= 0x1_01_00_07_0 {
            println!("cargo:rustc-cfg=ossl110g");
        }
        if version >= 0x1_01_01_00_0 {
            println!("cargo:rustc-cfg=ossl111");
        }
    }

Note that their configs are cumulative - so cfg!(ossl101) applies to version 1.0.1 or newer. To do a "less then" check, presumably you'd need to do something like cfg!(not(ossl111))

I think part of why the feature flag approach failed for them is because they had multiple incompatible providers (libreopenssl vs openssl), which thankfully is a problem we don't have to contend with, so maybe it doesn't directly apply.

I do think there's something to be said for having the configuration generated automatically by the build script rather than expecting the user to know they need to specify them.

michaelkirk · 2020-09-01T16:36:25Z

Maybe this is already known, but just a note to say this isn't currently building for me with the gdal_3_0 feature:

output from cargo test --features gdal_3_0

   Compiling gdal v   Compiling gdal v0.6.0 (/Users/mkirk/src/georust/gdal)
0.6.0 (/Users/mkirk/src/georust/gdal)
warning: unused variable: `allowed_drivers`
  --> src/gdal_common/dataset.rs:50:54
   |
50 | ...flags: Option, allowed_drivers: Option<&str>, open_options: Option<&str>, sibling_files: Option<&str>) -> Result {
   |                        ^^^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_allowed_drivers`
   |
   = note: `#[warn(unused_variables)]` on by default
warning: unused variable: allowed_drivers

--> src/gdal_common/dataset.rs:50:54

|

50 | ...flags: Option, allowed_drivers: Option<&str>, open_options: Option<&str>, sibling_files: Option<&str>) -> Result {

|                        ^^^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: _allowed_drivers

|

= note: #[warn(unused_variables)] on by default
warning: unused variable: open_options

--> src/gdal_common/dataset.rs:50:85

|

50 | ...drivers: Option<&str>, open_options: Option<&str>, sibling_files: Option<&str>) -> Result {

|                           ^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: _open_options
warning: unused variable: open_options

--> src/gdal_common/dataset.rs:50:85

|

50 | ...drivers: Option<&str>, open_options: Option<&str>, sibling_files: Option<&str>) -> Result {

|                           ^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: _open_options
warning: unused variable: sibling_files

--> src/gdal_common/dataset.rs:50:113

|

50 | ...ptions: Option<&str>, sibling_files: Option<&str>) -> Result {

|                          ^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: _sibling_files
warning: 3 warnings emitted
warning: unused variable: sibling_files

--> src/gdal_common/dataset.rs:50:113

|

50 | ...ptions: Option<&str>, sibling_files: Option<&str>) -> Result {

|                          ^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: _sibling_files
warning: 3 warnings emitted
error[E0432]: unresolved import crate::vector

--> src/gdal_3_0/spatial_ref/tests.rs:4:29

|

4 | use crate::{SpatialRef_3_0, vector::Geometry, assert_almost_eq};

|                             ^^^^^^

|                             |

|                             unresolved import

|                             help: a similar path exists: crate::gdal_3_0::vector
error[E0432]: unresolved import crate::vector

--> src/gdal_3_0/spatial_ref/tests.rs:4:29

|

4 | use crate::{SpatialRef_3_0, vector::Geometry, assert_almost_eq};

|                             ^^^^^^

|                             |

|                             unresolved import

|                             help: a similar path exists: crate::gdal_3_0::vector
error[E0432]: unresolved imports crate::vector, crate::vector

--> src/gdal_3_0/vector/tests.rs:1:37

|

1 | use crate::{DatasetCommon, Dataset, vector::VectorDatasetCommon, SpatialRef, SpatialRefCommon, SpatialRef_3_0, vector::OGRwkbGeometryType};

|                                     ^^^^^^ unresolved import                                                   ^^^^^^ unresolved import

|

help: a similar path exists

|

1 | use crate::{DatasetCommon, Dataset, crate::gdal_3_0::vector::VectorDatasetCommon, SpatialRef, SpatialRefCommon, SpatialRef_3_0, vector::OGRwkbGeometryType};

|                                     ^^^^^^^^^^^^^^^^^^^^^^^

help: a similar path exists

|

1 | use crate::{DatasetCommon, Dataset, vector::VectorDatasetCommon, SpatialRef, SpatialRefCommon, SpatialRef_3_0, crate::gdal_3_0::vector::OGRwkbGeometryType};

|                                                                                                                ^^^^^^^^^^^^^^^^^^^^^^^
error[E0432]: unresolved imports crate::vector, crate::vector

--> src/gdal_3_0/vector/tests.rs:1:37

|

1 | use crate::{DatasetCommon, Dataset, vector::VectorDatasetCommon, SpatialRef, SpatialRefCommon, SpatialRef_3_0, vector::OGRwkbGeometryType};

|                                     ^^^^^^ unresolved import                                                   ^^^^^^ unresolved import

|

help: a similar path exists

|

1 | use crate::{DatasetCommon, Dataset, crate::gdal_3_0::vector::VectorDatasetCommon, SpatialRef, SpatialRefCommon, SpatialRef_3_0, vector::OGRwkbGeometryType};

|                                     ^^^^^^^^^^^^^^^^^^^^^^^

help: a similar path exists

|

1 | use crate::{DatasetCommon, Dataset, vector::VectorDatasetCommon, SpatialRef, SpatialRefCommon, SpatialRef_3_0, crate::gdal_3_0::vector::OGRwkbGeometryType};

|                                                                                                                ^^^^^^^^^^^^^^^^^^^^^^^
error[E0599]: no method named layer found for struct gdal_common::dataset::Dataset in the current scope

--> src/gdal_3_0/vector/tests.rs:25:20

|

25 |     let layer = ds.layer(0).unwrap();

|                    ^^^^^ method not found in gdal_common::dataset::Dataset

|

::: src/gdal_common/dataset.rs:8:1

|

8  | pub struct Dataset {

| ------------------ method layer not found for this

|

= help: items from traits can only be used if the trait is in scope

= note: the following trait is implemented but not in scope; perhaps add a use for it:

use crate::gdal_common::vector::dataset::VectorDatasetCommon;
error[E0599]: no method named layer found for struct gdal_common::dataset::Dataset in the current scope

--> src/gdal_3_0/vector/tests.rs:25:20

|

25 |     let layer = ds.layer(0).unwrap();

|                    ^^^^^ method not found in gdal_common::dataset::Dataset

|

::: src/gdal_common/dataset.rs:8:1

|

8  | pub struct Dataset {

| ------------------ method layer not found for this

|

= help: items from traits can only be used if the trait is in scope

= note: the following trait is implemented but not in scope; perhaps add a use for it:

use crate::gdal_common::vector::dataset::VectorDatasetCommon;
error[E0609]: no field 0 on type &_

--> src/gdal_3_0/vector/tests.rs:33:21

|

33 |         .map(|s| (s.0.to_string(), s.1.clone()))

|                     ^
error[E0609]: no field 0 on type &_

--> src/gdal_3_0/vector/tests.rs:33:21

|

33 |         .map(|s| (s.0.to_string(), s.1.clone()))

|                     ^
error: aborting due to 4 previous errors
error: aborting due to 4 previous errors
Some errors have detailed explanations: E0432, E0599, E0609.

For more information about an error, try rustc --explain E0432.Some errors have detailed explanations: E0432, E0599, E0609.

For more information about an error, try rustc --explain E0432.
error: could not compile gdal.
To learn more, run the command again with --verbose.

error: could not compile gdal.
To learn more, run the command again with --verbose.

michaelkirk · 2020-09-01T16:37:14Z

src/gdal_2_2/mod.rs

@@ -0,0 +1,2 @@
+mod spatial_ref;
+mod vector;


might want to cargo fmt before merging

jdroenner · 2020-09-01T18:54:07Z

Thank you for the great feedback! The build.rs approach is very nice. We could query gdal-config for the version.
I will fix and format the issues when im back from vacation:)

michaelkirk · 2020-09-01T18:58:16Z

Thank you for the great feedback! The build.rs approach is very nice. We could query gdal-config for the version.
I will fix and format the issues when im back from vacation:)

lol yes, sorry to keep bugging you. It's not urgent for me. Enjoy your vacation. =)

rmanoka · 2020-09-04T03:43:50Z

Just a minor note, adding to @michaelkirk 's points above. Cargo features are additive, so two crates with gdal as dependency might end up enabling contradicting feature gates. If that is a possibility, we could proactively add compile_error! for feature combinations that are not allowed. This, and ensuring via build.rs that we're indeed linking to the expected gdal version should be very cool!

jdroenner · 2020-09-24T14:45:29Z

I thought about this PR for some time. I think we will discard the trait based approach and generate cfg flags for versions using build.rs. Will create a new PR for this soon.

jdroenner · 2020-09-25T14:20:47Z

I created a PR where the version is selected at build time based on the installed GDAL version: #92 I think thats the way to go.

jdroenner added 5 commits July 10, 2020 21:02

move everything to gdal_common, fn to traits 1

cf66c90

unify dataset and driver

bf76a61

adapt ogr part and ad version traits

67812b5

move all exporets to lib level

011ab46

fix doctests

86b51e4

jdroenner mentioned this pull request Jul 17, 2020

Add support for Integer64 #80

Merged

ChristianBeilschmidt reviewed Jul 18, 2020

View reviewed changes

michaelkirk reviewed Sep 1, 2020

View reviewed changes

src/gdal_2_2/mod.rs Outdated

@@ -0,0 +1,2 @@

mod spatial_ref;

mod vector;

Copy link

Member

michaelkirk Sep 1, 2020

Choose a reason for hiding this comment

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

might want to cargo fmt before merging

rmanoka mentioned this pull request Sep 8, 2020

Cargo features must be additive georust/proj#40

Closed

jdroenner added 2 commits September 18, 2020 15:31

fmt

1190e16

fmt + clippy + tests

6650b37

jdroenner marked this pull request as draft September 24, 2020 13:52

jdroenner closed this Sep 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature gate versions #81

Feature gate versions #81

jdroenner commented Jul 17, 2020 •

edited

Loading

ChristianBeilschmidt Jul 18, 2020

jdroenner Jul 18, 2020

michaelkirk commented Sep 1, 2020

michaelkirk commented Sep 1, 2020 •

edited

Loading

michaelkirk Sep 1, 2020

jdroenner commented Sep 1, 2020

michaelkirk commented Sep 1, 2020

rmanoka commented Sep 4, 2020 •

edited

Loading

jdroenner commented Sep 24, 2020

jdroenner commented Sep 25, 2020

Feature gate versions #81

Feature gate versions #81

Conversation

jdroenner commented Jul 17, 2020 • edited Loading

ChristianBeilschmidt Jul 18, 2020

Choose a reason for hiding this comment

jdroenner Jul 18, 2020

Choose a reason for hiding this comment

michaelkirk commented Sep 1, 2020

michaelkirk commented Sep 1, 2020 • edited Loading

michaelkirk Sep 1, 2020

Choose a reason for hiding this comment

jdroenner commented Sep 1, 2020

michaelkirk commented Sep 1, 2020

rmanoka commented Sep 4, 2020 • edited Loading

jdroenner commented Sep 24, 2020

jdroenner commented Sep 25, 2020

jdroenner commented Jul 17, 2020 •

edited

Loading

michaelkirk commented Sep 1, 2020 •

edited

Loading

rmanoka commented Sep 4, 2020 •

edited

Loading