1.0 Release with latest GDAL3 bindings #73
Conversation
1. stop removing prefixes from GDAL function names, thereby overloading them 2. removes the C submodule, everything is not together in the main module I realize the function renaming is quite breaking. In the README.md I wrote a bit about the changes and how to update code with the [renamer](https://gist.github.com/visr/0a2ad3fe92073345c93c2ca42f5f58a0#file-renamer-jl). Before we had many methods of (for instance) destroy. We used multiple dispatch to select the right C function if you called GDAL.destroy(x), depending on the type of x: ```json "destroy": [ "gdaldestroy", "gdal_cg_destroy", "ogr_fld_destroy", "ogr_gfld_destroy", "ogr_fd_destroy", "ogr_f_destroy", "ogr_ds_destroy", "ogr_sm_destroy", "ogr_st_destroy", "ogr_stbl_destroy" ] ``` This only worked because we rewrote the wrapping code, to pretend that `Ptr{Cvoid}` was actually `Ptr{OGRFeatureH}`, where `OGRFeatureH` was an abstract type we put in https://github.com/JuliaGeo/GDAL.jl/blob/b75ac1c3a351ac934e35827289a0d522a1fa7005/src/types.jl. While it was nice to use dispatch here, it did not match well with the C API. Firstly, it meant that wrapping became a complicated puzzle which did not neccesarily work. In `types.jl` we used subtyping to simulate that some functions could take different types of inputs. Now it is up to the user to select the right function. The functions will accept everything that can be converted to the types that C expects. This is in line with the new default Clang.jl behavior. This more straightforward wrapping of the GDAL C API is a better fit for this package, allowing others such as ArchGDAL.jl to try to make it more Julian. Regarding the C submodule, since we now do less rewriting, there is no strong reason anymore to keep them separate, folding them together.
|
I already like the title 👍 |
This code is mostly copy pasted from JuliaGeo/GDAL.jl#73. Three transformations are applied: - docstrings are added, created from PROJ Doxygen XML output - functions that return a Cstring are wrapped in unsafe_string to return a String - functions that return a Ptr{Cstring} are wrapped in unsafe_loadstringlist to return a Vector{String}
There was a problem hiding this comment.
For context to potential readers:
This PR grew out of conversations we had in private about how the rewriting of GDAL function names into names that were more Julian took a lot of effort (to resolve name clashes under the new default Clang behavior).
Given that
- the benefit is a shorter function name without any added features, and
- this package wasn't designed to be end-user facing as the API follows that of GDAL's C API,
we have decided that the benefits did not justify the effort to continue the rewriting of GDAL function names in GDAL.jl.
|
|
|
Do we have an overview of all (registered?) packages that have GDAL as a dependency? This release will break all those packages. I'm actually not in favor of deprecating all individual functions, but we should have a general deprecation warning at package load. |
I cannot seem to find a trace of it in the GDAL 3 repository, so I think it really dissappeared. |
Good catch thanks. I don't quite understand it yet. Has ccall behavior changed? Maybe this method should be defined by the |
That was a mistake on my end which led to a false alarm: I was calling the wrong GDAL method. |
If https://discourse.julialang.org/t/reverse-dependencies-of-a-package/9774/5 is correct, it's just GeoArrays and ArchGDAL. Though of course there are plenty scripts out there that use this package directly.
I thought about deprecations for functions. But doing it properly (e.g. redirecting automatically to a working alternative) will be a difficult and large task. If anybody would be up for doing that it would be great, but I don't want to spend my time on it frankly. The general deprecation warning at package load, I'm not sure how we can do that such that it won't be annoying to users that have updated their code. The new version indicates breaking changes, and what they are and how to update can be found on the README. |
|
I agree that (i) we should start having a better habit of maintaining a CHANGELOG as well as a smoother deprecation process, but also agree that (ii) it would have been too much work for us to provide that kind of user support in pre-1.0 versions. We should have a version that provides deprecation warnings to get some practice before 1.0, but I agree that it is too big of a job for this PR (after having gone through yeesian/ArchGDAL.jl#85 to get a sense for the complexity of the task). |
|
I want to add that I like that the docstrings still refer to the original GDAL function names. It makes it easier for people who are doing a search based on the original names. |
Yeah that was quite the effort, nicely done! Probably that was the single largest updating effort haha. |
|
Any objections to me merging this? How shall we do it with tagging releases? Do a simultaneous release of GDAL, ArchGDAL and GeoArrays? |
|
Let's do a simultaneous release. I've added a compat |
|
No objection from my side. I have a few projects depending on GDAL, with test the new version soon and will freeze GDAL version until then. |
Yeah, I think this package is ready for v1.0 :) I think a new release of GDAL.jl should be made first -- modulo tests from downstream packages working locally. And we'll wait for green from Travis and AppVeyor before updating downstream packages (ArchGDAL then GeoArrays; in the order of their dependencies). What do you all think? |
This code is mostly copy pasted from JuliaGeo/GDAL.jl#73. Three transformations are applied: - docstrings are added, created from PROJ Doxygen XML output - functions that return a Cstring are wrapped in unsafe_string to return a String - functions that return a Ptr{Cstring} are wrapped in unsafe_loadstringlist to return a Vector{String}
This code is mostly copy pasted from JuliaGeo/GDAL.jl#73. Three transformations are applied: - docstrings are added, created from PROJ Doxygen XML output - functions that return a Cstring are wrapped in unsafe_string to return a String - functions that return a Ptr{Cstring} are wrapped in unsafe_loadstringlist to return a Vector{String}
Fixes #70.
I realize the function renaming is quite breaking. In the README.md I wrote
a bit about the changes and how to update code with the renamer.
Before we had many methods of (for instance) destroy. We used multiple dispatch to select the right C function if you called
GDAL.destroy(x),depending on the type of
x:This only worked because we rewrote the wrapping code, to pretend that
Ptr{Cvoid}was actuallyPtr{OGRFeatureH}, whereOGRFeatureHwas an abstract type we put in types.jl. While it was nice to use dispatch here, it did not match well with the C API. Firstly, it meant that wrapping became a complicated puzzle which did not neccesarily work. Intypes.jlwe used subtyping to simulate that some functions could take different types of inputs. Now it is up to the user to select the right function. The functions will accept everything that can be converted to the types that C expects. This is in line with the new default Clang.jl behavior. This more straightforward wrapping of the GDAL C API is a better fit for this package, allowing others such as ArchGDAL.jl to try to make it more Julian. As you can see the wrapping code is now much simpler as a result, which should make it easier to maintain this package.Regarding the C submodule, since we now do less rewriting, there is no strong reason anymore to keep them separate, folding them together.