search

georust / gdal

Rust bindings for GDAL
https://crates.io/crates/gdal
MIT License
359 stars 94 forks source link

.github/workflows/ci.yml: run 'cargo valgrind test --lib' #472

Closed rouault closed 10 months ago

rouault commented 10 months ago
lnicola commented 10 months ago

I expected this to be slower, but 30 seconds isn't bad at all.

lnicola commented 10 months ago

I'm thinking it might be a good idea to run this in the 3.8 container, since LTS might not have some of the APIs (like MD) we've implemented.

rouault commented 10 months ago

I'm thinking it might be a good idea to run this in the 3.8 container, since LTS might not have some of the APIs (like MD) we've implemented.

I've added a commit for that. Ok, it detects a leak in gdal::raster::mdarray::MDArray::read_into_slice (). I need to figure out how to build georust/gdal against my custom GDAL build rather than the 3.0.4 that comes with my Ubuntu version

lnicola commented 10 months ago

Great, we can disable it for now.

https://github.com/georust/gdal/tree/master/gdal-sys#build might help with linking.

lnicola commented 10 months ago

Not sure if that, but we might be leaking the data type in the error case at https://github.com/georust/gdal/blob/master/src/raster/mdarray.rs#L166.

rouault commented 10 months ago

ok, the leak in read_into_slice() is fixed now, but there's a "system leak" related to threads & TLS, that is IMHO a false positive. But I can't find an option to specify a Valgrind suppression file to cargo valgrind. I guess we need to skip 3.8.1 testing for now

lnicola commented 10 months ago

Yeah, let's disable it. I'll merge this in the morning.

rouault commented 10 months ago

Yeah, let's disable it. I'll merge this in the morning.

I've dropped the 3.8.1 related commit

lnicola commented 10 months ago

Funny, I don't get the TLS leak on my system (Gedora 39, GDAL 3.7.3).

I think we can pass in a suppressions file with VALGRINDFLAGS="--suppressions=gdal.supp" cargo valgrind test.