-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
"python3 -m pip install OL-GeoIP" fails #1694
Comments
I have suspected we don't need GeoIP for a while, it would be good to confirm we don't then remove it. |
I will try to do that. I also sense that a run of https://github.com/jendrikseipp/vulture on the codebase might highlight other deadcode. |
@hornc What do you think has changed on that front (and when)? In the past year or so, when I was getting spurious "Your library can lend you this" messages, updating GeoIP was the solution. It is(was?) part of the "pretend we're lending library" thing. If the geolocation piece is handled differently or the "pretend" part is gone, it'd be a good dependency to get rid of because it requires constant updating to be current. |
@cdrini Maybe push a bit here? |
Here's the diff of OL-GeoIP (downloaded src from pypi) and upstream at same version (jlopez/maxmind-geoip@4ec7259 ): diff maxmind-geoip/setup.py OL-GeoIP-1.2.4/setup.py
1c1,6
< from distutils.core import setup, Extension
---
> from setuptools import setup, Extension
>
> description = """
> This is a fork of the Maxmind GeoIP Python wrapper library originally from GitHub at
> https://github.com/jlopez/maxmind-geoip with some minor changes for the openlibrary.org project
> """
4,7c9,12
< libraries = ['GeoIP'],
< sources = ['py_GeoIP.c'],
< library_dirs = ['/usr/local/lib'],
< include_dirs = ['/usr/local/include'])
---
> libraries = ['GeoIP'],
> sources = ['py_GeoIP.c'],
> library_dirs = ['/opt/local/lib', '/usr/local/lib'],
> include_dirs = ['/opt/local/include', '/usr/local/include'])
9,12c14,21
< setup (name = 'GeoIP-Python',
< version = '1.2.4',
< description = 'This is a python wrapper to GeoIP',
< ext_modules = [module1])
---
> setup (name = 'OL-GeoIP',
> version = '1.2.4',
> description = description,
> ext_modules = [module1],
> maintainer = "Noufal Ibrahim",
> # url = "https://github.com/nibrahim/maxmind-geoip/tarball/master",
> # # url = "https://github.com/nibrahim/maxmind-geoip",
> maintainer_email = "[email protected]") Only notable change was adding |
I am unsure if this is a blocker but Latest commit 7206da2 on Dec 7, 2011 should be a cause for concern. |
It exports at least Note this line:
Every method in this file requires this database. |
As I mentioned Jan. 4, #1694 (comment) unless this has changed recently, I believe it is used to identify the lending library. openlibrary/openlibrary/core/inlibrary.py Line 110 in 75452cf
I don't see any reason to support an obsolete private fork though. We should just switch to the current Maxmind supported bindings. |
Can you expand on your previous comment, @tfmorris ? I don't think I know the feature you're referring to. What was it? Why might've it used GeoIP? When was it killed? @jdlrobson Do you have any context on this? Asking cause I noticed you made this change a while back: |
I am not sure yet if this "solved" the problem or merely "masked" the symptom. |
@cdrini The addition of The only OL specific changes are supposedly to allow it to be uploaded to PyPI internetarchive/maxmind-geoip@1e32375, but the official MaxMind package is now available from PyPI, so I think we can just switch. There are two official MaxMind libraries:
Note that the GeoIP Lite databases that they depend on are no longer freely available without registering for an account, but that's a separate problem. |
closing as we have decided to remove, see #2896 |
https://pypi.org/project/OL-GeoIP/1.2.4 is not updated since 2011
Do we need GeoIP?
Do we know what mods were made from upstream for the OL project?
Should we try to use upstream instead?
https://github.com/internetarchive/openlibrary/blob/master/requirements_common.txt#L23
https://travis-ci.org/internetarchive/openlibrary/jobs/465261888#L881
Next steps
GeoIP
; does it export anything else?GeoIP
geoip_database
defined in the config on prod?/usr/local/maxmind-geoip/GeoLiteCity.dat
exist on prod?Stakeholders
@nibrahim
The text was updated successfully, but these errors were encountered: