Allow using system installation of argon2

[seifertm]

This PR checks whether extras/libargon2/src is present (i.e. whether the argon2 submodule was checked out). If it is not present, the compilation step of argon2 (build_clib) will be skipped.

During build_ext, the compiler is called with the system-specific include and library dirs anyway, so it will just find argon2, if it is installed correctly on the system.

Note that this PR also changes the library name from libargon2 to just argon2. As far as I know, the lib prefix is platform specific. GCC, for example, automatically looks for libargon2.so when -largon2 is specified, whereas MSVC should just expect argon2.dll. Changing the library name causes distutils to append -largon2 on the command line. This is the only change where I am unsure if it breaks something on Windows or MacOS.

Solves #28.

Tested on Linux x86_64.

What are your thoughts on this?

[codecov]

Codecov Report

Merging #29 into master will decrease coverage by 1.61%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
- Coverage   88.19%   86.58%   -1.62%     
==========================================
  Files           8        8              
  Lines         161      164       +3     
  Branches       19       20       +1     
==========================================
  Hits          142      142              
- Misses         19       22       +3

Impacted Files	Coverage Δ
src/argon2/_ffi_build.py	0% <0%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f9709f...b74be14. Read the comment docs.

[seifertm]

I am running argon_cffi -> passlib -> devpi-server successfully with this change set.
Any thoughts on this?

[hynek]

Sorry I didn’t get around looking at it sooner. I’m not entirely enthusiastic about guessing the intent by the presence or absence of the argon2 lib, that’s an invitation for blunders and lots of bogus bug reports. :)

How would you feel about having an env variable such that you could do something like this:

$ env ARGON2_CFFI_USE_SYSTEM=1 pip install --no-binary=:all: argon2-cffi

?

[seifertm]

You are right. I like your idea better than my current solution.
I'll try to adjust the PR accordingly.

[seifertm]

• Reverted default behaviour of setup.py to try to build libargon2 from the submodule
• If the env variable ARGON2_CFFI_USE_SYSTEM=1, then the build phase build_clib is disabled (this seemed nicer than hacking it into the custom build_clib command)
• Cleared include directories in _ffi_build.py when the env variable is set, in order to prevent mixing includes from the bundled argon2 sources and the system

[seifertm]

The TravisCI build fails due to an error reported by flake8. I don't think the issue is caused by this PR, because I never touched src/argon2/low_level.py.
Should I fix it within this Pull Request, anyway?

[hynek]

Yeah, that would be nice!

Another question: would it make sense to somehow allow for customisation of where we look for the installed library? Or should we delegate that to overwriting LDFLAGS and whatnot?

[seifertm]

• Fixed an overlong line in setup.py (squashed commit)
• Fixed an indentation error in setup.py (squashed commit)
• Ignored the flake8 error for ambiguous variable names that is caused by the Type enum value I which denotes the argon2i algorithm flavour. To me, disabling the check made more sense than to work around the issue by renaming the enum constant.

Personally, I would go with the customization as is. I don't have a use case for an argon2 clib in a non-standard location on my system. Nobody seemed to bother for the last two years, either :)
I suggest that we tackle any further customization, like custom library paths or custom include paths in a separate issue, if needed.

I am unsure, how this PR has affected test coverage, by the way…

[seifertm]

ping @hynek :)

[hynek]

Hey I’m super sorry that I’m sort of stalling this whole thing and I really appreciate the work and patience you’re putting out there.

The main reason is that it’s seriously scary to me. 👻

But I promise I’ll see this to land, since more and more distros are starting to package it.

---

In any case, we’ll have to find some way to test this behaviour in CI, to ensure it doesn’t break later – just Travis is fine. So we’ll have to get an installation of argon2 into Travis and then install argon2_cffi on top of it.

Is that something you’d be willing to do? Have you done something like this before?

---

Re: coverage: it used to be 100% until it went down without any changes in our code so I’ll have to figure it out myself – don’t worry about it.

[seifertm]

If other distros want to create a package, it could be worthwhile to involve them. Do you know of any specific bug tracker issues where we could point to this PR? We could possibly acquire a few more testers.

I have previously worked with Travis and I will try to get a running test case there.
Honestly, I am not too worried that this PR will break anything, because it should only affect the packaging. However, let me know if there is anything else I can do to make the changes less scary :)

[hynek]

I don’t think we need to do any custom tailoring for now. It should be enough to prove that with the correct CFLAGS/LDFLAGS, a custom Argon2 installation is preferred over the vendored one.

[seifertm]

• Created the tox environment "system-argon2" which installs argon2_cffi from sources with ARGON2_CFFI_USE_SYSTEM=1 and runs tests. This step fails on my machine, when argon2 is not installed.
• Added and configured the new tox environment as a run in TravisCI

[hynek]

Wow that’s sweet!

We just need a way to ensure that the system one is actually used.

Do you think at least on of the two following options (ideally both 😇) would be possible:

1. Show some path to the library used in purest using pytest_report_header.
2. Add an assertion that if ARGON2_CFFI_USE_SYSTEM=1, it’s used?

I think it should be rather easy but cffi isn’t on the top of my head right now.

[seifertm]

Huh, this seems to be more difficult than I expected…
There is ctypes.util.find_library('argon2'), but depending on the OS, it does not always return the full path to the library (e.g. on my Linux system, it returns just libargon2.so.0). Examining sys.path did not look promising, either. I was hoping that I could get some meta information via FFI().dlopen(<Python bindings>), but the resulting object does not provide any methods or properties.

We could use ldd to inspect the bindings, but I don't know of a way to make it cross-platform.

What I did is disabling the initialization of the vendored argon2 submodule for the system-argon2 case in Travis. This way, we can be sure that the bindings link against the system version.

I couldn't come up with anything better. Any other ideas or pointers?

[hynek]

Hm not really but maybe @reaperhulk has an idea?

[reaperhulk]

I don't have a very good suggestion for the short term here, but I would suggest that you submit a PR to libargon2 adding a function that returns the current version. Having it be a constant in the headers but not callable makes it very difficult to confirm the runtime version you've loaded.

[hynek]

Hm i don’t think that would be super helpful for my problem, because the version could very much match. I really need the path of the lib... so I guess I should rather harass the cffi folks? 🤔

[hynek]

(FTR I need to ship attrs 17.4, this one is the next in my FOSS queue)

[hynek]

Thanks!

[koobs]

Just wanted to say thank you @seifertm @hynek for getting this done. Proved helpful during packaging argon2_cffi for FreeBSD ports