v.out.ogr: fixes ERROR: Unable to create OGR spatial reference with Shapefile export and expands unit tests #3869
Conversation
github-actions
bot
added
vector
| @@ -324,7 +324,6 @@ int main(int argc, char *argv[]) | |||
| inwkt = NULL; | |||
| } | |||
| proj_destroy(source_crs); | |||
| Ogr_projection = NULL; | |||
There was a problem hiding this comment.
Are you making the actual change or just adding tests? Do the test fail without the change?
There was a problem hiding this comment.
This is indeed the main change, the new test is a consequence of this change. The new test fails against the main branch, but succeeds on this branch. This is why the PR is incremental on #3848.
There was a problem hiding this comment.
This change looks incorrect to me, because if the OSRImportFromWkt() call at line 331 below is made, it will crash in non-debug GDAL builds.
Perhaps you also need to do "Ogr_projection = OSRNewSpatialReference(NULL)" to re-instanciate the object, probably within the if (inwkt) branch below
And you likely also need to do a OSRDestroySpatialReference(Ogr_projection) before nullifying the pointer to avoid a memory leak
There was a problem hiding this comment.
@rouault This is my interpretation of your comment:
https://github.com/ldesousa/grass/blob/b2ab84646a9aa4a869c97167cc6a954beb14201c/vector/v.out.ogr/main.c#L327
The tests fail with this alternative formulation. Whatever is happening inside OSRImportFromWkt it seems to require a fully set "instance" of OSRNewSpatialReference. Or perhaps I am missing something else. Please advise.
Btw, I believe the tests are not run with GDAL in debug mode. At least on my system shouldn't be the case.
There was a problem hiding this comment.
yes, that's on the good track. But you are still missing a OSRDestroySpatialReference(Ogr_projection) before Ogr_projection = NULL to avoid a memory leak
There was a problem hiding this comment.
@rouault This is not the right track, because the tests fail. Putting it another way: why is the Ogr_projection = NULL instruction necessary?
There was a problem hiding this comment.
@ldesousa ah, I was mostly speaking from a point of view of using the OGR API. Why the functionality doesn't work as expected here is not obvious to me. Let's try to find a moment in the next days to look at that behind the same screen
There was a problem hiding this comment.
@rouault Today the tests are passing, yesterday I might have been running them from the wrong branch, my apologies. This is the latest formulation, let me know if it meets your guidance:
https://github.com/ldesousa/grass/blob/5a7ade03f84cd7718db3a2cfb1d476f5ccc65866/vector/v.out.ogr/main.c#L327
| dbase = env["GISDBASE"].replace("'", "").replace(";", "") | ||
| rmtree(f"{dbase}/{self.temp_location}", ignore_errors=True) | ||
|
|
||
| def test_1(self): |
There was a problem hiding this comment.
Please name the test according to the tested format or whatever this is testing. Short one-sentence docstring would be nice, but not needed.
There was a problem hiding this comment.
I added a new commit with these changes.
ldesousa
changed the title
echoix
changed the title
This PR is incremental on PR #3848. Please merge that one before merging this one.