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

wrapper for gdalmdimtranslate #289

Merged
merged 9 commits into from Sep 2, 2022
Merged

wrapper for gdalmdimtranslate #289

merged 9 commits into from Sep 2, 2022

Conversation

ChristianBeilschmidt
Copy link
Contributor

  • 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.

I created a wrapper around GDALMultiDimTranslate. I used part of the logic for the wrapper for the VRT program.

impl TryFrom<Vec<&str>> for MultiDimTranslateOption {
type Error = GdalError;

fn try_from(value: Vec<&str>) -> std::result::Result<Self, Self::Error> {
Copy link
Member

Choose a reason for hiding this comment

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

Does this work?

Suggested change
fn try_from(value: Vec<&str>) -> std::result::Result<Self, Self::Error> {
fn try_from(value: Vec<&str>) -> Result<Self> {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

///
/// [GDALMultiDimTranslateOptions]: https://gdal.org/api/gdal_utils.html#_CPPv428GDALMultiDimTranslateOptions
///
pub struct MultiDimTranslateOption {
Copy link
Member

Choose a reason for hiding this comment

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

Should probably be called MultiDimTranslateOptions, since there can be multiple options inside.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ChristianBeilschmidt
Copy link
Contributor Author

I forgot to replace the Cell<bool> with a plain boolean, I guess it can be this simple 🙈 .

@lnicola
Copy link
Member

lnicola commented Aug 27, 2022

bors r+

bors bot added a commit that referenced this pull request Aug 27, 2022
289: wrapper for gdalmdimtranslate r=lnicola a=ChristianBeilschmidt

- [X] I agree to follow the project's [code of conduct](https://github.com/georust/gdal/blob/master/CODE_OF_CONDUCT.md).
- [X] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---

I created a wrapper around `GDALMultiDimTranslate`. I used part of the logic for the wrapper for the VRT program.

Co-authored-by: Christian Beilschmidt <christian.beilschmidt@geoengine.de>
@bors
Copy link
Contributor

bors bot commented Aug 27, 2022

Build failed:

@lnicola
Copy link
Member

lnicola commented Aug 27, 2022

thread 'config::tests::test_error_handler' panicked at 'assertion failed: `(left == right)`
  left: `[(Warning, 1, "GPKG: bad application_id=0x00000000 on '/tmp/.tmpPTiAzK.gpkg'"), (Failure, 42, "foo"), (Warning, 1, "bar")]`,
 right: `[(Failure, 42, "foo"), (Warning, 1, "bar")]`', src/config.rs:230:9

Is this some sort of new GDAL warning?

@ChristianBeilschmidt
Copy link
Contributor Author

thread 'config::tests::test_error_handler' panicked at 'assertion failed: `(left == right)`
  left: `[(Warning, 1, "GPKG: bad application_id=0x00000000 on '/tmp/.tmpPTiAzK.gpkg'"), (Failure, 42, "foo"), (Warning, 1, "bar")]`,
 right: `[(Failure, 42, "foo"), (Warning, 1, "bar")]`', src/config.rs:230:9

Is this some sort of new GDAL warning?

Since it appeared in the LTS-CI, I guess not.

What could have happened:
The tests run in parallel. Some test sets the global error handler. Then some other test fails… which lands in the error handler. And this test gets some strange results.

I'm not sure, but we could get races on all the global stuff.
Maybe we need some global Mutexes on some test stuff that must not interfere with something else 🤷.

What do you think?

@jdroenner
Copy link
Member

bors merge

@bors
Copy link
Contributor

bors bot commented Sep 2, 2022

Build succeeded:

@bors bors bot merged commit f22bde0 into master Sep 2, 2022
@jdroenner jdroenner deleted the gdalmdimtranslate branch September 2, 2022 08:15
@lnicola
Copy link
Member

lnicola commented Sep 2, 2022

I'm a bit worried about the test failure here.

@jdroenner
Copy link
Member

jdroenner commented Sep 2, 2022

i guess the problem is not really related to this PR but to gdal and cargo test in general. Cargo runs all the tests in parallel and when two tests fail at the same time we have a problem with global state.
So we need to find a solution for that. We could use a mutex for reading / writing global state...

@lnicola
Copy link
Member

lnicola commented Sep 2, 2022

I think we can move them to an integration test.

bors bot added a commit that referenced this pull request Sep 2, 2022
297: Move error handling test to an integration test r=jdroenner a=lnicola

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/gdal/blob/master/CODE_OF_CONDUCT.md).
- [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---

CC #289 (comment)

Co-authored-by: Laurențiu Nicola <lnicola@dend.ro>
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

3 participants