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

Support GEOS Installation from Library #291

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

keithdoggett
Copy link
Member

@keithdoggett keithdoggett commented Feb 25, 2022

Summary

Allows GEOS to be linked with a library (.so, .a, .dll, etc.) by specifying the RGEO_GEOS_LIB_DIR environment variable.

Other Information

This works, but there are still a couple issues that need to be discussed/resolved

  • Due to how find_library works if libgeos_c is not found in the RGEO_GEOS_LIB_DIR directory and it is installed on the system (/usr/local/lib for example), it will silently fall back on the system installed version instead of raising an error.
  • We may want to discuss automatically falling back on the geos-config flow if there is an error during the from library flow.
  • Discuss how to test this in CI

keithdoggett and others added 9 commits January 13, 2022 18:45
…or. Removed self-intersecting is_simple? test on CAPIPolygons because this will be version dependent.
Replace a manual validity check on MultiPolygon, which was iterating on
each polygon combination, by a direct call of GEOSisValid.
This make for less C code on our part, and allowed to fix a MacOS
related bug.
Get rid of `lenient_assertions` in favor of another way of handling validity: never fail fast, fail when
impossible to do otherwise.

Co-authored-by: Keith Doggett <keith.doggett887@gmail.com>
Co-authored-by: Quentin Wentzler <quentin.wentzler@klaxit.com>
Co-authored-by: Ulysse Buonomo <buonomo.ulysse@gmail.com>
@BuonOmo
Copy link
Member

BuonOmo commented Mar 11, 2022

@keithdoggett that's nice thanks, indeed the fallback may be really missleading if someone is expecting the build from a specific library.

Also, and more importantly, I'm not sure where we want to go with this. E.g. in which circumstances someone would want to load geos from somewhere else? And in that case, why not loading geos from somewhere in the path of this library (#272).

Hence I think this is a nice step towards getting in that place, since we'll only have to specify GEOS's local path then, but I just want to be sure about the reason why you started from that end :)

@keithdoggett
Copy link
Member Author

@BuonOmo yeah the main reason was to make that packaging easier eventually. I guess I figured that this was the hard part since I hadn't seen a C extension built in this way before, but there are some other use cases.

  1. Some programs ship with GEOS and if you're using that library for some part of your system it may make sense to ensure that RGeo has the same version so you're not running different versions of GEOS. (one example: https://www.cockroachlabs.com/docs/stable/install-cockroachdb-linux.html).

  2. Right now the only way to install it is if geos-config is installed on your system. In some environments it may be easier to work with the compiled libraries directly, especially if you want to use a version that is not available on the package manager by default. This just adds some more flexibility.

@BuonOmo
Copy link
Member

BuonOmo commented Mar 16, 2022

I definitely agree with you

I guess I figured that this was the hard part since I hadn't seen a C extension built in this way before

I've seen some, for instance the ruby-sass!

Base automatically changed from v3-dev to master March 22, 2022 16:41
@BuonOmo BuonOmo deleted the branch main July 7, 2022 15:38
@BuonOmo BuonOmo closed this Jul 7, 2022
@BuonOmo BuonOmo reopened this Jul 7, 2022
@BuonOmo BuonOmo changed the base branch from master to main July 7, 2022 16:04
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.

None yet

3 participants