Yubico / python-fido2

Provides library functionality for FIDO 2.0, including communication with a device over USB.
BSD 2-Clause "Simplified" License
432 stars 109 forks source link

Add FreeBSD support #64

Closed grembo closed 5 years ago

grembo commented 5 years ago

Includes unit tests.

This also adds pytest-runner, so that unit tests can be excluded based on platform in conftest.py easily (pyfakefs isn't available for FreeBSD).

Tested with yubico/yubikey-manager (ykman fido ...).

I copied the google license headers to the new files in _pyu2f. Code is formatted to pass pycodestyle (existing files in _pyu2f use some custom formatting).

grembo commented 5 years ago

Please note that unit tests are failing in travis CI on 3.4 only, all other builds (2.7, 3.5, 3.6, 3.7) work correctly. This is due to pytest deprecating support for 3.4. Their deprecation support plan states:

Thanks to the python_requires setuptools option, Python 2.7 and Python 3.4 users using a modern pip version will install the last pytest 4.6 version automatically even if 5.0 or later are available on PyPI.

This seems to work okay on the 2.7 builder (which uses pip 19.x), but not on the 3.4 builder (which uses pip 18.x). Updating pip on the 3.4 builder to a newer version should fix this problem.

I'll force push a workaround to setup.py for the time being, so it should build with older versions of pip on python <3.5 too until the builder is updated.

See also: https://github.com/pytest-dev/pytest/issues/5519 https://pytest.readthedocs.io/en/latest/py27-py34-deprecation.html

dainnilsson commented 5 years ago

Thanks! I'd prefer not to change how the tests are run though, would replacing the pytest dependency with something like this in setup.py get the job done?

tests_require = ['mock>=1.0.1']
if sys.platform.startswith('linux'):
    tests_require.append('pyfakefs>=3.4')

setup(
    tests_require=tests_require,
grembo commented 5 years ago

Thanks for you quick response!

Thanks! I'd prefer not to change how the tests are run though, would replacing the pytest dependency with something like this in setup.py get the job done?

tests_require = ['mock>=1.0.1']
if sys.platform.startswith('linux'):
    tests_require.append('pyfakefs>=3.4')

That aspect of the problem is already handled in the patch:

tests_require=[
        'mock>=1.0.1',
        'pyfakefs>=3.4;platform_system=="Linux"',

(which means the same in a declarative way and is independent of pytest).

The problem is, that this would still import linux_test.py (which then fails on importing pyfakefs). Using pytest allows doing this in conftest.py:

if not sys.platform.startswith('linux'):
    collect_ignore.append('_pyu2f/linux_test.py')
if not sys.platform.startswith('darwin'):
    collect_ignore.append('_pyu2f/macos_test.py')
if not sys.platform.startswith('freebsd'):
    collect_ignore.append('_pyu2f/freebsd_test.py')

which I thought was quite elegant and easy to understand. I also liked the idea, that this way changes in one platform specific unit test would never affect other platforms. Reading the setuptools sources, this wasn't possible without overriding setuptool's TestLoader implementation by setting test_loader in setup.py to a custom implementation, which didn't seem worth the added complexity.

If you don't like the idea of using pytest, linux_test.py could be changed in a more pragmatic way, so the import of pyfakefs reads like

try:
    from pyfakefs import fake_filesystem
except ImportError:
    try:
        from fakefs import fake_filesystem
    except ImportError:
        if sys.platform.startswith('linux'):
             raise

Which should have the same net effect of unit tests not failing, but without isolation of platforms and some extra warnings like "Not running on ".

What do you think?

dainnilsson commented 5 years ago

Ah, thanks for the clarification, makes perfect sense!

In that case, yes, I would prefer the alternative you now suggested. I agree it's not quite as clean, but I think it's worth it to avoid having to add the pytest dependency (I'm also trying to make sure testing this project is consistent with our other various projects).

grembo commented 5 years ago

Ah, thanks for the clarification, makes perfect sense!

In that case, yes, I would prefer the alternative you now suggested. I agree it's not quite as clean, but I think it's worth it to avoid having to add the pytest dependency (I'm also trying to make sure testing this project is consistent with our other various projects).

Okay, done.

dainnilsson commented 5 years ago

Thank you! Merged!

grembo commented 5 years ago

Cool, thanks!