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

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

Merged
merged 2 commits into from
Dec 1, 2023

Conversation

rouault
Copy link
Contributor

@rouault rouault commented Nov 30, 2023

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

@lnicola
Copy link
Member

lnicola commented Nov 30, 2023

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

@lnicola
Copy link
Member

lnicola commented Nov 30, 2023

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 rouault force-pushed the ci_cargo_valgrind branch 2 times, most recently from 09814dc to 53c27a5 Compare November 30, 2023 22:27
@rouault
Copy link
Contributor Author

rouault commented Nov 30, 2023

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
Copy link
Member

lnicola commented Nov 30, 2023

Great, we can disable it for now.

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

@lnicola
Copy link
Member

lnicola commented Nov 30, 2023

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
Copy link
Contributor Author

rouault commented Nov 30, 2023

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
Copy link
Member

lnicola commented Nov 30, 2023

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

@rouault
Copy link
Contributor Author

rouault commented Nov 30, 2023

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

I've dropped the 3.8.1 related commit

@lnicola lnicola merged commit 9524e82 into georust:master Dec 1, 2023
9 checks passed
@lnicola
Copy link
Member

lnicola commented Dec 1, 2023

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.

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.

2 participants