python-u2flib-host icon indicating copy to clipboard operation
python-u2flib-host copied to clipboard

Code duplication between this project and python-u2flib-server

Open moreati opened this issue 10 years ago • 2 comments

Porting python-u2flib-server to cryptography I noticed that a few modules are duplicated in this project, and they're starting to diverge. Two examples are

  • u2flib_host.utils and u2flib_server.utils
  • u2flib_host.soft and python-u2flib-server/test/soft_u2f_v2.py

Are these intentional tradeoffs that you'd like to keep? Or mistakes you'd like fixed?

The options I see are

  1. Keep it as is, try to keep them in sync by hand.
  2. Make u2flib-server use the implementation in u2flib-host, and depend on it.
  3. Vice versa
  4. Create a third package that provides the common parts of u2flib-host and u2flib-server.

1 could allow bugs fixed in one package to go unfixed in the other. 2, 3 & 4 increase the complexity of making/keeping track of/declaring dependencies when releasing.

moreati avatar Aug 18 '15 14:08 moreati

I'm not a fan of options 2 or 3, really. Mainly I want to avoid having to pull in hidapi as a dependency in the server library.

Regarding the utils modules these are probably simple enough that duplication isn't a big deal. The soft U2F device is a bit more complex, and it would be good to avoid having it duplicated. Since it's only used in the server library for testing it might be fine to add the host library as a test dependency.

As for option 4 I don't think the current duplication warrants the overhead of adding a new package. An option would be to use the yubicommon package for this, but I'd like to avoid having it contain too package specific code. The utils stuff might be fine for it.

dainnilsson avatar Aug 19 '15 12:08 dainnilsson

I agree utils.py duplicates are small enough to stay where they are.

Making python-u2flib-host a test_requires of python-u2flib-server feels like a good idea. I'd prefer not to put anything in yubicommon that's specific to U2F.

moreati avatar Aug 19 '15 13:08 moreati