Skip to content

Commit

Permalink
Use resources loader to handle non-filesystem situations (#120)
Browse files Browse the repository at this point in the history
* Use a resources loader if possible

* Remove logic duplication
  • Loading branch information
adferrand committed Mar 18, 2020
1 parent d15edfc commit a9ab4b3
Showing 1 changed file with 27 additions and 12 deletions.
39 changes: 27 additions & 12 deletions certifi/core.py
Expand Up @@ -6,25 +6,40 @@
This module returns the installation location of cacert.pem or its contents.
"""
import importlib
import os

try:
from importlib.resources import read_text
import importlib.resources
# Defeat lazy module importers.
importlib.resources.open_binary
_HAVE_RESOURCE_READER = True
except ImportError:
# This fallback will work for Python versions prior to 3.7 that lack the
# importlib.resources module but relies on the existing `where` function
# so won't address issues with environments like PyOxidizer that don't set
# __file__ on modules.
def read_text(_module, _path, encoding="ascii"):
with open(where(), "r", encoding=encoding) as data:
return data.read()
_HAVE_RESOURCE_READER = False

try:
import pkg_resources
# Defeat lazy module importers.
_HAVE_PKG_RESOURCES = True
except ImportError:
_HAVE_PKG_RESOURCES = False

def where():
f = os.path.dirname(__file__)
_PACKAGE_NAME = "certifi"
_CACERT_NAME = "cacert.pem"

return os.path.join(f, "cacert.pem")

def where():
if _HAVE_PKG_RESOURCES:
return pkg_resources.resource_filename(_PACKAGE_NAME, _CACERT_NAME)
else:
mod = importlib.import_module(_PACKAGE_NAME)
path = os.path.dirname(mod.__file__)
return os.path.join(path, _CACERT_NAME)


def contents():
return read_text("certifi", "cacert.pem", encoding="ascii")
if _HAVE_RESOURCE_READER:
return importlib.resources.read_text(_PACKAGE_NAME, _CACERT_NAME, encoding="ascii")
else:
with open(where(), "r", encoding="ascii") as data:
return data.read()

12 comments on commit a9ab4b3

@ivankravets
Copy link

@ivankravets ivankravets commented on a9ab4b3 Apr 5, 2020

Choose a reason for hiding this comment

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

@Lukasa could you revert back this commit?

  1. It broke Python 2 via open(where(), "r", encoding="ascii"), there is no encoding argument for Python 2. If you need "encoding", please use io.open.

  2. It produces strange errors:

File "***/certifi/core.py", line 33, in where
  File "/usr/local/lib/python3.7/site-packages/pkg_resources/__init__.py", line 1146, in resource_filename
    self, resource_name
  File "/usr/local/lib/python3.7/site-packages/pkg_resources/__init__.py", line 1738, in get_resource_filename
    "resource_filename() only supported for .egg, not .zip"
NotImplementedError: resource_filename() only supported for .egg, not .zip

@Lukasa
Copy link
Member

@Lukasa Lukasa commented on a9ab4b3 Apr 5, 2020

Choose a reason for hiding this comment

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

I don't care about breaking Python 2 (it's EOL, time to move on), but the Python 3.7 error is problematic. @adferrand, any idea what the heck is going on here? I'm going to back this out and ship a patch release to revert to the previously working state.

@alex
Copy link
Member

@alex alex commented on a9ab4b3 Apr 5, 2020 via email

Choose a reason for hiding this comment

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

@Lukasa
Copy link
Member

@Lukasa Lukasa commented on a9ab4b3 Apr 5, 2020

Choose a reason for hiding this comment

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

@alex done

@ivankravets
Copy link

Choose a reason for hiding this comment

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

It would be good to remove https://github.com/certifi/python-certifi/blob/master/setup.py#L57:59 lines if this package is not compatible with Python 2 & 3. To be honest, I don't see any reason why you should drop Python 2. A lot of packages depend on certifi. Also, a lot of people use different tools which depend on requests which depends on certifi. These users ARE NOT PYTHON DEVELOPERS. So, they just use Python which is shipped with OS or was installed before.

I'm on a user's side. Yes, we should use Python 3. However, package maintainers are responsible for the smooth transition. Our project @platformio is much much much more complex and we still maintain Python 2 and Python 3. I would be happy if we can drop Python 2 tomorrow.

@ivankravets
Copy link

Choose a reason for hiding this comment

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

@Lukasa what is the reason dropping Python 2? Do you depend on API from Python 3 which is not available in Python 2? We are developers and package maintainers. I still do not understand. Sorry.

@Lukasa
Copy link
Member

@Lukasa Lukasa commented on a9ab4b3 Apr 5, 2020

Choose a reason for hiding this comment

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

It would be good to remove https://github.com/certifi/python-certifi/blob/master/setup.py#L57:59 lines if this package is not compatible with Python 2 & 3.

Already done.

However, package maintainers are responsible for the smooth transition.

Agreed. The original sunset date of Python 2 was announced in 2008 as 2015. In 2014 that sunset date was extended five years, to 2020. I believe six years was plenty of time to put together a transition plan. Certifi has supported Python 3 since at least 2014, so it has been possible for dependencies to support Python 3 throughout that time. We managed our transition period, and it was at least six years long. This just simply does not seem to be our problem.

On top of this, however, the reason to want certifi is because you want to maintain the security of your infrastructure. Python 2 is no longer receiving security patches. It's a by-definition insecure platform. Complaining about certifi not supporting Python 2 is like complaining about the fact that your door locks don't work when your house's walls are falling apart. To stretch the metaphor: get out of the building.

@ivankravets
Copy link

Choose a reason for hiding this comment

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

maintain the security of your infrastructure

It's not about me. Over 1M developers use PlatformIO and people use different Pythons. Most of them are embedded developers. We have good rating of our extension https://marketplace.visualstudio.com/items?itemName=platformio.platformio-ide&ssr=false#review-details

People write bad reviews very often due to cases where problems are linked with similar dependencies like certifi. So, if something does not work, they think that the whole PlatformIO is bad.

We notify them that we will drop Python 2 soon. We also install Python 3 by default on Windows. However, we can "kill" 500K developers because of Python 2 EOL.

Nevertheless, it's your repository. We will just drop dependency on this package and will use raw cacert.pem.

Thanks for your work!

@Lukasa
Copy link
Member

@Lukasa Lukasa commented on a9ab4b3 Apr 5, 2020

Choose a reason for hiding this comment

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

For the moment this conversation is moot: I have pushed an update that reverted this commit and therefore restores Python 2 function. I am not planning to ship a new release at this time, nor instruct pip to refuse to install.

However, things are rapidly coming to a place where Python 2 and Python 3 support represents an entirely unreasonable maintenance burden. Python 2 support is going to start vanishing. People need to get on board.

@ivankravets
Copy link

Choose a reason for hiding this comment

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

It's up to you. I would be thankful if you inform about dropping Python 2 to PSF. See https://github.com/psf/requests/blob/master/setup.py#L48

@Lukasa
Copy link
Member

@Lukasa Lukasa commented on a9ab4b3 Apr 5, 2020

Choose a reason for hiding this comment

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

Given that we have reverted this patch and that @dstufft has suggested that he can provide an alternative that does not regress Python 2 support, this should not be necessary in the near term. But I am expressly not committing to a minimum time bound on supporting Python 2. I'm supporting it as long as it's practical, and then there must either be a discussion about those who need Python 2 support stepping up to do the work of keeping it running or I'm dropping that support. One of those two must happen.

@dotlambda
Copy link

Choose a reason for hiding this comment

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

Didn't 3e58560 break Python 2 compatibility already by using the encoding argument?

Please sign in to comment.