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

Pin GDAL version. #354

Merged
merged 3 commits into from
Dec 30, 2022
Merged

Pin GDAL version. #354

merged 3 commits into from
Dec 30, 2022

Conversation

evetion
Copy link
Collaborator

@evetion evetion commented Dec 30, 2022

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 evetion requested a review from yeesian December 30, 2022 08:06
@evetion
Copy link
Collaborator Author

evetion commented Dec 30, 2022

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

@visr
Copy link
Collaborator

visr commented Dec 30, 2022

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

yeesian commented Dec 30, 2022

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

@yeesian yeesian requested a review from visr December 30, 2022 13:27
@visr
Copy link
Collaborator

visr commented Dec 30, 2022

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 JuliaPackaging/Yggdrasil#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
Copy link
Collaborator Author

evetion commented Dec 30, 2022

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

visr commented Dec 30, 2022

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

evetion commented Dec 30, 2022

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

visr commented Dec 30, 2022

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.

Copy link
Owner

@yeesian yeesian left a comment

Choose a reason for hiding this comment

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

LGTM here to get main branch back to green -- my understanding is to remove the upperbound on GDAL_jll after the tests have been fixed for GDAL 3.6 (in a separate PR).

Taking GDAL 3.6 out of user hands can also be seen as breaking.

I guess this can be seen as a "rollback" of the introduction of GDAL 3.6 until the breakages are addressed. Given that there might be quite a surface area of tests to be fixed, I'd prefer to rollback to the latest known "green state" for now.

@evetion
Copy link
Collaborator Author

evetion commented Dec 30, 2022

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

visr commented Dec 30, 2022

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 yeesian merged commit b4844ca into master Dec 30, 2022
@yeesian yeesian deleted the fix/pin-gdal branch December 30, 2022 14:37
@yeesian
Copy link
Owner

yeesian commented Dec 30, 2022

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 #189 and good changelog in #251. (Making this comment so this thread is also linked from those issues.)

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.

3 participants