Revise extra dependency handling

[lukpueh]

[Updated on Jan 23, 2020]

Description of issue or feature request:

securesystemslib lists some dependencies that require C-code (cryptography requiresopenssl, pynacl requires libsodium) as optional to allow for a pure-python installation. The runtime handling of missing optional dependencies should be revised.

Current behavior:
cryptography and pynacl are listed as optional (extra) dependencies, but securesystemslib does not fare (consistently) well, if installed without them.

Expected behavior:

• Public facing modules (e.g. interface.py and keys.py) must be importable, even if the optional dependencies are not installed. Fixed with Improve handling of native dependencies #200

• Each public facing function always should be callable and present meaningful user-feedback if an optional dependency that is required for that function is not installed. Fixed with Improve handling of native dependencies #200

• Also address or keep in mind recently merged or pending functionality, that has non-pure Python dependencies (Add gpg support and custom (sub)process module #174, sphincs+ support, for post-quantum crypto #169, External Signing using CCID/PIV interface. #170).

-Optional: colorama was made a strict dependency in #178 to quickfix #155. @SantiagoTorres, to consider making it a optional again (with respect to required adoptions as outlined above). Fixed with #200

• It would be nice to fine-tune code coverage measurement (see Improve handling of native dependencies #200 (comment) ff.)

[joshuagl]

When looking at available options for testing for the presence of optional dependencies it seems there's an old mechanism imp, which was deprecated in Python 3.4 and a new mechanism importlib, which is available since Python 3.1.

securesystemslib's tox environment tests Python 2.7, 3.5 and 3.6 - but that doesn't necessarily mean those are the only versions we care to support. PyPI lists securesystemslib-0.11.3 as supporting Python 2.7, 3.4, 3.5 and 3.6.

It may be reasonable to take the same approach as Ansible (see Ansible commit 2732cde) and use imp for Python versions < 3 and importlib for anything else.

However with Python 2's EOL being close I wanted to get some feedback on whether it's even desirable to continue to maintain Python2 support in securesystemslib?
As we consider this It's worth noting that dateutil have pledged to drop support for Python 2.7 no later than 2020. It's not clear if the project has settled on a schedule yet but their python3statement entry indicates they won't be making releases that support Python2 beyond 2019-07-01.

[lukpueh]

Thanks for your assessment, @joshuagl .

What's the benefit of using imp/importlib over a more vanilla approach that just catches ImportError? I think that's what securesystemslib already does in some cases.

In my mind, we would encapsulate imports and use of optional libraries in non-public modules like so:

# in `securesystemslib/optional_foo_private_module.py`
try:
  import foo

except ImportError:
  NO_FOO = True

def foo():
  if NO_FOO:
    raise("<Helpful message about missing foo>") 
  foo.foo()

This would keep our public interface clean of import-related case handling.

# in `secureystemslib/public_interface.py`
import optional_foo_private_module

def foo():
  """Call foo or raise helpful error message if not installed. """
  optional_foo_private_module.foo()

What do you think about that pattern?

Regarding Python 2.7, we discussed this recently and decided that we'll keep it around for a little longer. But I agree, we won't be able to do this for too long. We'll run into troubles at the latest when our dependencies drop support. Still, I would prefer if we could resolve this issue without dropping 2.7.

[joshuagl]

I had noticed that the try: import XXX / except ImportError pattern is present in places in securesystemslib and it's what I've used in the past. I'll be happy to proceed with that pattern, especially as it postpones a decision on Python2 without additional coding effort.

[joshuagl]

Looking at outstanding native dependencies that need handling:

Sphincs (#169) and CCID/PIV (#170) can be handled in a similar fashion to other key algorithm modules, that is: setting a constant to False on ImportError and raiseing UnsupportedLibraryError at call time from functions requiring the library. What's the best way to handle this in terms of this issue? Comment on the PRs suggesting that such a change be added? Propose patches to the existing PR?

I have a WIP patch for GPG, raiseing an UnsupportedLibraryError is semantically incorrect, as it's a programme that's missing not a library, but it may be better to throw a consistent Exception for application authors to catch when dependencies are missing. What do you think @lukpueh ?

I also need to figure out how to test it. The gpg command is rarely missing on modern Linux, as most distro package managers pull it in as a dependency, and testing only on Windows feels insufficient.

[lukpueh]

Looking at outstanding native dependencies that need handling:

💯

Sphincs (#169) and CCID/PIV (#170) can be handled in a similar fashion to other key algorithm modules, that is: setting a constant to False on ImportError and raiseing UnsupportedLibraryError at call time from functions requiring the library. What's the best way to handle this in terms of this issue? Comment on the PRs suggesting that such a change be added? Propose patches to the existing PR?

Leaving a comment on the two PRs, pointing to the discussion here, and your awesome fix in #200 should go a long way. Would you mind doing that?

I have a WIP patch for GPG, raiseing an UnsupportedLibraryError is semantically incorrect, as it's a programme that's missing not a library, but it may be better to throw a consistent Exception for application authors to catch when dependencies are missing. What do you think @lukpueh ?

Throwing the same error sounds like a good idea. And I think the semantics even work if the securesystemslib.gpg is seen as the unsupported library. ;)

I also need to figure out how to test it. The gpg command is rarely missing on modern Linux, as most distro package managers pull it in as a dependency, and testing only on Windows feels insufficient.

Might apt-get remove gnupg gnupg2 work?

[lukpueh]

This is fixed for existing code in #200 and #206. Authors of pending relevant PRs have been informed to follow the new strategy (#170 (comment), #169 (comment)). I have created a separate feature request for a fine-tune code coverage measurement setup in #208. Thanks for the hard work, @joshuagl! Closing here.