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

chore: upgrade geos #252

Merged
merged 2 commits into from
Feb 6, 2025
Merged

chore: upgrade geos #252

merged 2 commits into from
Feb 6, 2025

Conversation

xhwhis
Copy link
Contributor

@xhwhis xhwhis commented Feb 6, 2025

geos@9 is yanked. Can you update a new version to crates.io?

Since geos 0.9 was yanked, geozero 0.14.0 no longer builds.

Previously, ./geozero-cli (and ./geozero-bench) depended not on the
geozero source code in the workspace, but on the published [email protected]

Because geozero-cli and geozero-bench could not build [email protected], CI was failing.

At least for now, let's have geozero-cli and geozero-bench depend on the
local crate, because it fixed this build.

Personally, I think it would make sense to _keep_ crates in the
workspace using the local source code. But maybe others disagree.

In particular, I think I would be very surprised to have geozero-bench
actually bench the published geozero crate rather than whats in the
working directory.

And I think it's good to know when API changes to ./geozero require
changes to ./geozero-bench and ./geozero-cli

At the point you are making those changes in ./geozero is the best time
for you to understand the ramifications of those changes, addressing any
"downstream" breakages in the local workspace.
@michaelkirk
Copy link
Member

I pushed up another commit to fix the build by having workspace crates use the local geozero. Let's see how it goes.

/cc @nyurik - you might have feelings about this. Tagging you so you don't think I'm trying to "sneak" it past you. I'm open to other suggestions which solve the problems discussed in my commit.

@urschrei
Copy link
Member

urschrei commented Feb 6, 2025

Very much in agreement with the commit message suggestion FWIW. It's not clear to me why one would want to refer to the published version in this scenario, and it makes maintenance more complex for a small team (which includes people who could not comprehend the RELEASING instructions, i.e. me)

@@ -56,4 +56,4 @@ tokio = { version = "1.30.0", default-features = false }
wkt = "0.12.0"

[patch.crates-io]
#geozero = { path = "./geozero" }
Copy link
Member

@pka pka Feb 6, 2025

Choose a reason for hiding this comment

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

git blames me for changing this line in e3f6502. I'm with you that a local build should always use ./geozero. 👍

@michaelkirk michaelkirk added this pull request to the merge queue Feb 6, 2025
Merged via the queue into georust:main with commit 3e62817 Feb 6, 2025
1 check passed
@xhwhis xhwhis deleted the geos-yanked branch February 7, 2025 02:18
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.

5 participants