search

yeesian / ArchGDAL.jl

A high level API for GDAL - Geospatial Data Abstraction Library
https://yeesian.github.io/ArchGDAL.jl/stable/
Other
137 stars 25 forks source link

Pin GDAL version. #354

Closed evetion closed 1 year ago

evetion commented 1 year ago

As discussed in #345. I also upped the version number, this release would also allow a working version on m1 macs on 1.8.2+.

evetion commented 1 year ago

A subsequent PR (+release) will update the GDAL version and fix the tests.

visr commented 1 year ago

Because the SOVERSION changed in GDAL 3.6, I bumped the major version of the GDAL_jll to 301. Another option would be to add GDAL_jll as a direct compat entry with the major version. Pinning the minor GDAL.jl version will stop any new features from being available here.

yeesian commented 1 year ago

Hmm yeah, I think it'll depend on the stability guarantees that we'd like GDAL.jl to provide -- therefore adding @visr as a reviewer to this PR.

One possibility is for the major version of GDAL.jl to depend not only on the stability of the source code in GDAL.jl, but also on the stability of its binary dependencies. In such a scenario, GDAL.jl should bump its major version if GDAL_jll is bumped in its major version. In that case, nothing will need to change here.

Otherwise, I am partial to to add GDAL_jll as a direct compat entry with the major version in this package for more stability (since it will help with the review process of other PRs here to keep the main branch green).

visr commented 1 year ago

It's not always easy to see what is breaking ahead of time. Also because when building new JLLs no downstream tests are performed automatically. Many of the JLLs have restrict the minor version between each other because the SOVERSION changes in between. Stuff like https://github.com/JuliaPackaging/Yggdrasil/pull/5408. But that is all ABI compatibility, not API. Both seem to sometimes break in new minor versions.

Thinking about it now, I think JLL versioning can most easily be employed to maintain working builds and known breaking changes. Then GDAL.jl can use it's major version also to signal the stability of its binary dependencies as @yeesian suggests. So the GDAL.jl release with GDAL 3.6.0 should have really been GDAL.jl 2, not 1.5. Then all packages that depend on GDAL.jl are automatically protected by semver breaking changes.

I'm not sure it still makes sense to merge this now. ArchGDAL.jl is generally functional with GDAL 3.6.0, and people may already rely on its features. Better to fix the last few tests.

evetion commented 1 year ago

Well, this PR is just to have a green master branch indeed, whether by pinning GDAL or GDAL_jll, so we can have a release. I've now opted for the latter.

The real question is what we do from now on, in a subsequent PR. It would probably up the compat for GDAL.jl to 1.5, which would fix everything, or indeed GDAL.jl 2.

visr commented 1 year ago

We could have a new release also with a few broken tests right? We could mark them as broken to get green CI, or fix them. Taking GDAL 3.6 out of user hands can also be seen as breaking.

evetion commented 1 year ago

I'm not sure it still makes sense to merge this now. ArchGDAL.jl is generally functional with GDAL 3.6.0, and people may already rely on its features. Better to fix the last few tests.

Multiple different tests are failing, all of which could break production code. We should fix the tests, fix the compat (ArchGDAL 0.10 if we break the progress API) and release anyway, the question is whether to release in between (0.9.4). I want to have a release in between ASAP, because 0.9.3 is buggy on M1 for recent Julia builds and has these breaking changes if you hit GDAL 3.6.

visr commented 1 year ago

Users that get hit by one of the GDAL 3.6 breaking changes can always pin GDAL_jll themselves as well right? The breaking GDAL has been out for over a month and I haven't seen one bug report related to it.

evetion commented 1 year ago

ArchGDAL.jl is generally functional a new release also with a few broken tests right? We could mark them as broken can always pin GDAL_jll themselves

It seems we have different ideas about the stability of a package. For me the public API is like a contract, and the subset that the tests cover is an even stricter contract.

Would be good to write down the expected stability that we agree on of this package somewhere, including a good changelog.

visr commented 1 year ago

Ok, we can do this for now.

different ideas about the stability of a package

Not sure we have different ideas about that, I said GDAL.jl has been breaking. The discussion is only on what is best now for most users.

yeesian commented 1 year ago

Would be good to write down the expected stability that we agree on of this package somewhere, including a good changelog.

That's a great callout, thanks! Feel free to add your thoughts on expected stability in https://github.com/yeesian/ArchGDAL.jl/issues/189 and good changelog in https://github.com/yeesian/ArchGDAL.jl/issues/251. (Making this comment so this thread is also linked from those issues.)