Skip to content

Conversation

@asinghvi17
Copy link
Contributor

@asinghvi17 asinghvi17 commented Sep 18, 2024

https://github.com/paleolimbot/S2Geography is a wrapper around S2Geometry that provides a GEOS-like C++ API around the S2 objects which often don't act like geospatial objects.

This PR is still very WIP and I'm experimenting quite a bit with the build script following the lessons from my last PR for S2Geometry.

Copy link
Member

Choose a reason for hiding this comment

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

Can you please upstream this patch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened a PR a while ago google/s2geometry#379 and I just saw someone commented there - will see what they think. But for this version is this reasonable to get in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doh, never mind, this is for s2geography. I'll talk to the author there to try and upstream this.

@fingolfin
Copy link
Member

@asinghvi17 hi there, are you still interested in completing this?

@asinghvi17 asinghvi17 marked this pull request as ready for review November 24, 2025 21:51
@asinghvi17
Copy link
Contributor Author

Hey @fingolfin, thanks for the ping! I marked this as ready for review and am re-running CI now.

@asinghvi17 asinghvi17 closed this Nov 24, 2025
@asinghvi17 asinghvi17 reopened this Nov 24, 2025
@fingolfin
Copy link
Member

@asinghvi17 that won't work, you'll have to rebase this on latest master, or merge latest master into it

@asinghvi17
Copy link
Contributor Author

asinghvi17 commented Nov 24, 2025

Somehow it doesn't let me merge online, so I will do that locally tomorrow

@fingolfin
Copy link
Member

I merged master for you so CI can run

# from the JLLs.
script = raw"""
cd ${WORKSPACE}/srcdir/s2geography
atomic_patch -p1 ../patches/msvc_to_win32_target.patch
Copy link
Member

Choose a reason for hiding this comment

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

This failed because your patch was in bundled/msvc_to_win32_target.patch not in bundled/patches/msvc_to_win32_target.patch -- I've moved it now

@fingolfin fingolfin changed the title [WIP] New recipe: S2Geography [S2Geography] New recipe Nov 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants