"python3 -m pip install OL-GeoIP" fails

cclauss commented 5 years ago

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

py_GeoIP.c:25:1: error: unknown type name ‘staticforward’

Next steps

[ ] It exports GeoIP; does it export anything else?
[X] Find uses of GeoIP
[X] Diff our version with the upstream
[X] See when it was introduced; maybe the commit message has some info
[ ] Is geoip_database defined in the config on prod?
[ ] Does /usr/local/maxmind-geoip/GeoLiteCity.dat exist on prod?

Stakeholders

@nibrahim

hornc commented 5 years ago

I have suspected we don't need GeoIP for a while, it would be good to confirm we don't then remove it.

cclauss commented 5 years ago

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.

tfmorris commented 5 years ago

@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.

cclauss commented 5 years ago

@cdrini Maybe push a bit here?

cdrini commented 5 years ago

Added some next steps to issue.

@cclauss Is this a blocker for python3?

@tfmorris @hornc any clues as to where it might be used?

cdrini commented 5 years ago

Here's the diff of OL-GeoIP (downloaded src from pypi) and upstream at same version (https://github.com/jlopez/maxmind-geoip/commit/4ec7259b2cc2c8902919947a5e2db5ac09586c73 ):

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 = "noufal@archive.org")

Only notable change was adding /opt/local/lib to library_dirs and /opt/local/include to include_dirs. My python-package-fu is not very strong; what does this mean?

cclauss commented 5 years ago

I am unsure if this is a blocker but Latest commit 7206da2 on Dec 7, 2011 should be a cause for concern.

cdrini commented 5 years ago

It was added in this commit to requirements: 3948c39581d55f78953d0cc955fa0214dae66aa0 (14 Dec 2011)
Before that it was added to setup.py by @nibrahim e5ef75c6dc05f2992107b923e48c4471f4041ae8 on 11 Oct 2011 (same day as release on pypi)

Commit message was "Added an update to get GeoIP", so no useful data there.

cdrini commented 5 years ago

It exports at least GeoIP; this is used in https://github.com/internetarchive/openlibrary/blob/2b2895fb849aea8344d5aab77e883faae547dc00/openlibrary/core/geo_ip.py .

Note this line: https://github.com/internetarchive/openlibrary/blob/2b2895fb849aea8344d5aab77e883faae547dc00/openlibrary/core/geo_ip.py#L9

Every method in this file requires this database. geoip_database does not exist locally; does it exist on prod? Does /usr/local/maxmind-geoip/GeoLiteCity.dat exist locally (no) or on prod? If not, then this file (and likely GeoIP) are unused, and we need to find more dead code by tracing uses of this file.

tfmorris commented 5 years ago

If not, then this file (and likely GeoIP) are unused,

As I mentioned Jan. 4, https://github.com/internetarchive/openlibrary/issues/1694#issuecomment-451362505 unless this has changed recently, I believe it is used to identify the lending library.

https://github.com/internetarchive/openlibrary/blob/75452cf6f0e3c23e1030395c93e232686ebaa417/openlibrary/core/inlibrary.py#L110

I don't see any reason to support an obsolete private fork though. We should just switch to the current Maxmind supported bindings.

cdrini commented 5 years ago

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: https://github.com/internetarchive/openlibrary/commit/2c9274d46e508ae4060fb9e9e28ecacc429556d4#diff-bee60bee0cf02815ad9385b15edc8c97R25

xayhewalo commented 5 years ago

@cclauss did #1693 resolve this. It looks like we're not requiring OL-GeoIP for Python3 versions

cclauss commented 5 years ago

I am not sure yet if this "solved" the problem or merely "masked" the symptom.

tfmorris commented 4 years ago

@cdrini The addition of /opt/local actually happened in a commit from the main repo to support MacPorts https://github.com/jlopez/maxmind-geoip/commit/2fd74fb574416eb2ad0c47ddefcc5849e23c0f17

The only OL specific changes are supposedly to allow it to be uploaded to PyPI https://github.com/internetarchive/maxmind-geoip/commit/1e3237533d79f5914d8e76bdc1423ddd520dd486, but the official MaxMind package is now available from PyPI, so I think we can just switch.

There are two official MaxMind libraries:

Legacy Python API
GeoIP2 API I think we should stick with the second since it's a descendant of what we're using.

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.

hornc commented 4 years ago

closing as we have decided to remove, see #2896

internetarchive / openlibrary

"python3 -m pip install OL-GeoIP" fails #1694

Next steps

Stakeholders