upgrade ci

[geohardtke]

CI should now build and test against gdal 3.0, 3.1, 3.2, 3.3 and 3.4

Unfortunately some tests are now failing. Any idea?

[geohardtke]

Some more work is needed here, but I don't have time right now. Will come back here later

[geohardtke]

Test pass in gdal 3.0; then in gdal 3.2 and gdal 3.3; test_sql_with_dialect is broken.

test vector::vector_tests::tests::test_features_aliasing_compile_fail ... ok

failures:

---- vector::vector_tests::sql::test_sql_with_dialect stdout ----
thread 'vector::vector_tests::sql::test_sql_with_dialect' panicked at 'called `Result::unwrap()` on an `Err` value: CplError { class: 3, number: 1, msg: "In ExecuteSQL(): sqlite3_prepare_v2(SELECT * FROM roads WHERE highway = 'pedestrian' and NumPoints(GEOMETRY) = 3):\n  no such function: NumPoints" }', src/vector/vector_tests/sql.rs:82:10
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    vector::vector_tests::sql::test_sql_with_dialect

and in gdal 3.4 a second test breaks; auto_identify :

---- spatial_ref::tests::auto_identify stdout ----
thread 'spatial_ref::tests::auto_identify' panicked at 'called `Result::unwrap()` on an `Err` value: OgrError { err: 7, method_name: "OSRAutoIdentifyEPSG" }', src/spatial_ref/tests.rs:207:38

Which shows why it is a good idea to test against as many versions as possible. :)

[geohardtke]

Ok. so the problem with one of the function was probably a driver not available on the minimal ubuntu base, swapping to the full image solved the problem.

[jdroenner]

great work!
i guess the failure in 3.4 is caused by Proj not accepting the SRS identification we have in that test.

[geohardtke]

Proj not accepting the SRS

Any idea how to fix it? I'm not very familiar with that side of the API.

[michaelkirk]

I was able to get the test to pass with these changes:
https://github.com/geohardtke/gdal/compare/upgrade_ci...michaelkirk:mkirk/upgrade-ci?expand=1

I've never used the WKT SRS format before. Is it possible that it changed with the more recent versions?

[michaelkirk]

bors try

[geohardtke]

I was able to get the test to pass with these changes: https://github.com/geohardtke/gdal/compare/upgrade_ci...michaelkirk:mkirk/upgrade-ci?expand=1

Excellent, thanks! We do have all green lights now.

Maybe it would be nice to add a note about this in the release notes when gdal 3.4 bindings are added? Unfortunately I can't find anything in the release notes upstream to explain the change in behaviour. I'm not even sure if it is a change in gdal or proj?

[michaelkirk]

I don't know either. One data point I have is that I was actually seeing this same error locally with gdal 3.3 and proj 8.2 (on apple aarch64).

So that might point to it being related to the underlying proj instance... maybe 🤷

[geohardtke]

yes, I'm inclined to think that is a change in proj. I just checked and the docker image for gdal 3.4 uses proj 8.2 while the 3.3 version uses 8.0.1.
So the actual behaviour will depend on the proj version available in the system and not the gdal version. So probably we don't need to add anything to the release notes.

Thanks again!

[jdroenner]

thanks! this a huge improvement. I also think think it depends on the proj version.

[jdroenner]

bors r+

[bors]

Timed out.

[bors]

try

Timed out.

[bors]

try

Timed out.

[jdroenner]

bors try

[jdroenner]

the ci is not happy with some of the tests :(

[jdroenner]

bors retry

[lnicola]

We might have to merge this manually, but I'd rebase it first.

[jdroenner]

yes. just thought it should try again 😆

[bors]

Timed out.

[jdroenner]

i will rebase and merge it later

[jdroenner]

ok now we have a new issue. Thanks clippy -_-

What about adding a ::zero() method to GdalType or use num_traits ::zero() to initialize generic vectors?

[rmanoka]

@jdroenner The set_len is exactly as suggested in the std::Vec docs, but unfortunately the example link keeps changing. See https://doc.rust-lang.org/std/vec/struct.Vec.html#examples-22 . May be clippy will accept it if we do the set_len after the FFI call? I believe it is still sound even if the FFI errors out as the capacity is already known to Vec and it should drop it correctly.

[jdroenner]

i think we would have to make the FFI call in the same method. we can not use a &mut ref and pass it to the other method.

[jdroenner]

@rmanoka this way it works. Don't know what i like better.

[jdroenner]

bors try

[rmanoka]

@jdroenner I like your approach; it's a bit of copied code, but safer. We used set_len earlier because this could be part of a hot-loop in many raster reading paradigms. That said, safety and easier maintenance is more important, so I don't mind either the copied code or the T::zero() approach. There may be a way using MaybeUninit as clippy suggests, but it's beyond my understanding.

[rmanoka]

Another option, if you would like to not have copied code: move the common GDALRasterIOEx code into a private fn that takes *mut instead of &mut. Then both the method returning Buffer and the read_into_slice can call that. Since it is *mut we can now do the set_len after the call. At this point, it's more for clarity, so pls. feel free to ignore this.

[jdroenner]

lets use it like this then

[jdroenner]

bors merge

[bors]

try

Timed out.

[bors]

Timed out.

[jdroenner]

i guess the CI is just to much for bors