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

Add generator files. Wrap latest 1.9 release. #24

Merged
merged 3 commits into from
Oct 15, 2023
Merged

Add generator files. Wrap latest 1.9 release. #24

merged 3 commits into from
Oct 15, 2023

Conversation

evetion
Copy link
Member

@evetion evetion commented Oct 14, 2023

@evetion evetion requested a review from visr October 14, 2023 08:17
@visr
Copy link
Member

visr commented Oct 14, 2023

Pin the 1.9 jll release, so we don't automatically update to 1.10

Why is that bad? I understand we don't want a breaking release, but why block a minor release?

@evetion
Copy link
Member Author

evetion commented Oct 14, 2023

Pin the 1.9 jll release, so we don't automatically update to 1.10

Why is that bad? I understand we don't want a breaking release, but why block a minor release?

I don't think we can assume semver on external C dependencies. Like we've seen with GDAL and PROJ, I rather be more strict in what we allow.

@visr
Copy link
Member

visr commented Oct 14, 2023

I don't think we can assume semver on external C dependencies.

Not in general no, but unnecessarily restricting to minor versions prevents upgrades from flowing through the ecosystem. Generally handling breaking versions is done at the Yggdrasil and General registry side, and the strategy there is to only restrict to minor versions if they break the ABI. For e.g. PROJ we have the x00.y00,z00 versioning scheme set up to put semver on top of the original version number. For LibSpatialIndex there has not yet been a reason to do so.

If libspatialindex 1.10 comes out with this PR merged, it can still install that by installing the current 0.2.0 release of this package. So it would also have to be edited for past releases in the General registry.

@evetion
Copy link
Member Author

evetion commented Oct 14, 2023

Not in general no, but unnecessarily restricting to minor versions prevents upgrades from flowing through the ecosystem. Generally handling breaking versions is done at the Yggdrasil and General registry side, and the strategy there is to only restrict to minor versions if they break the ABI. For e.g. PROJ we have the x00.y00,z00 versioning scheme set up to put semver on top of the original version number. For LibSpatialIndex there has not yet been a reason to do so.

I'm aware, but ABI breakage is rare, while behaviour breakage is much more common, and you just don't know when updating a jll. Letting possible breakages flow automatically through the ecosystem seems like a bad move, as each it happens we break semver for the ecosystem. I think pinning strictly to a single version(s) might be even better, and make new releases only after reverse dependency checks. It's a balance between automatic updates (and developer ease) on the one hand, versus the user experience. It's an incredibly bad user experience to one day add an unrelated package, thereby update some packages automatically, and now your script errors. Since the jll is probably never a direct dependency, reverting this update is actually painful, as it involves understanding that a jll update caused it, including which jll caused it, and manually finding the version that doesn't break and now pinning it. Using a Manifest is a solution, but is not used extensively for the general environment.

If libspatialindex 1.10 comes out with this PR merged, it can still install that by installing the current 0.2.0 release of this package. So it would also have to be edited for past releases in the General registry.

That is a good point.

Anyway, this PR doesn't seem the place to discuss this (libspatialindex is rarely updated and used less than say GDAL or PROJ), so I will revert the ~ here.

@rafaqz
Copy link
Member

rafaqz commented Oct 14, 2023

This came up at the recent Munster workshop. We really cant mix non-semver versions into a semver ecosystem (and still say we have stick to semver in any packages with non-semver deps).

I came out of that strongly in favour of locking down our binary deps like this wherever they hit a package with real semver.

What we have with semver and hard upper bounds is truly amazing compared to Python/R ecosystems, and we should lean into it even more.

Instead of reverting the ~, we can add it to all the previous versions in the registry?

@visr
Copy link
Member

visr commented Oct 14, 2023

See also the Slack thread @evetion started in #pkg-dev. I agree of course the whole situation would be better if we had reverse CI in Yggdrasil to evaluate breaking changes before instead of sometimes after the fact.

@evetion evetion mentioned this pull request Oct 14, 2023
@evetion
Copy link
Member Author

evetion commented Oct 14, 2023

I've reverted the pin here, but made a new issue. This PR is about providing the code to autogenerate the C bindings.

gen/generator.jl Outdated Show resolved Hide resolved
gen/generator.toml Show resolved Hide resolved
@evetion evetion merged commit 418d2f3 into master Oct 15, 2023
9 of 11 checks passed
@evetion evetion deleted the feat/wrap branch October 15, 2023 13:29
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