libarchive: Fix bcrypt logic on Windows - #2842
Conversation
69a6774 to
3ec3f2f
Compare
bgilbert
left a comment
There was a problem hiding this comment.
Couple small things, LGTM otherwise!
| if not get_option('cng').disabled() | ||
| cdata.set('HAVE_BCRYPT_H', 1) |
There was a problem hiding this comment.
| if not get_option('cng').disabled() | |
| cdata.set('HAVE_BCRYPT_H', 1) | |
| cdata.set('HAVE_BCRYPT_H', 1) | |
| if not get_option('cng').disabled() |
There was a problem hiding this comment.
I mentally treat it as two different kinds of dependencies. When writing this, I consider the scenario where bcrypt is required because host_machine.system() == 'windows' but the user doesn't want it otherwise, as that's the most complicated state here.
My original PR reflected this viewpoint, but you want to merge them more. Should HAVE_BCRYPT_H be defined even when the user does not want it (when get_option('cng').disabled() == true)? If we went a little further with the merging logic, I could make Meson ignore get_option('cng') if host_machine.system() == 'windows' and just make it fully required.
I have tested it localy and the library compiles on Windows without HAVE_BCRYPT_H set.
I guess reading the code more to understand this, inspecting upstream CMake build system or possibly asking upstream could help.
There was a problem hiding this comment.
Update: Upstream's CMake doesn't care, it gladly let's you disable ENABLE_CNG, which will lead to linker errors on __archive_mktempx. I will make an issue/PR upstream. I'll let them decide when the macros should be defined.
There was a problem hiding this comment.
Ultimately these sorts of questions usually have to be resolved by searching the upstream source. With some upstreams we're forced to use configuration tricks, for example by disabling a HAVE_X macro when we have X but want to disable a feature that's tied to it. Here that isn't necessary because we can set the ARCHIVE_CRYPTO_*_WIN macros if we want to use CNG, and skip them otherwise.
And so, there's a Gricean thing going on here: if we decline to set HAVE_BCRYPT_H when we do indeed have bcrypt.h, we're implying that we need to make that distinction for some reason, which needlessly makes the code more difficult to understand.
You're right that this doesn't make a functional difference; it's just a cleanliness thing. If you feel strongly about it, I'm okay leaving it as is.
See added comment for explanation.
3ec3f2f to
3bb0154
Compare
See added comment for explanation.
Before, the user was allowed to set
cngtodisabledto completely disablebcrypt, which caused compilation issues if bcrypt is unconditionally needed:This PR makes the build system recognize the scenario where bcrypt must be present.