-
Notifications
You must be signed in to change notification settings - Fork 101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Build a module instead of a shared library. #207
Conversation
Thanks for digging so deep to unearth these pretty specific linking options. For MacOS it seems you nailed it! Windows seems to have similar "peculiarities", though (see CI run).
Conceptually a pretty desirable idea (as the filename will stay fixed) -- just wondering whether there'd then be a (some other) convenient way to see (by looking at the file) which version of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a Windows machine/VM at your disposal to check out/fix what's going wrong with "MODULE" there? I know from experience how painful "remote CI" debugging is... If not, I'd be fine merging this PR if we'd set "SHARED" only for Windows platforms for the time being (and someone knowledgeable on that platform to look into the problem).
@thb-sb FYI, I just did a trial adapting your "APPLE" code to "MSVC and CYGWIN" (plus some CI changes) and "got lucky": All CI seems to pass now: Could you please check whether this is OK from your side? If so, I'm inclined to merge given the good CI results (in https://github.com/open-quantum-safe/oqs-provider/tree/module). If you'd have an idea why they don't show up in this PR, please let me know. Edit/add: Looking at this again, my changes to https://github.com/thb-sb/oqs-provider/tree/module are only showing up there but not in this PR -- so a merge would probably fail: Could you please check whether you can push the latest tip there into this PR again? |
Ho great, thank you very much!
I guess you don't have w access by default on my fork, explaining why the changes did not show up here? |
Fix #202. This commit switches the default type in CMake from `SHARED` to `MODULE`. Two issues have been found. ## First issue I ran into an issue while testing this commit on macOS. Here is the error I got from clang: ```shell clang: error: invalid argument '-compatibility_version 1.0.0' only allowed with '-dynamiclib' ``` After some investigation, I found out that the [`SOVERSION` and `VERSION`](https://cmake.org/cmake/help/latest/prop_tgt/SOVERSION.html) properties should not be used when building a shared library. This doesn't seem to lead to an error on Linux, but on macOS, it happens to set the following two flags: * `-compatibility_version` (given by `SOVERSION`) * `-current_version` (given by `VERSION`) However, CMake uses the `-bundle` flag when linking, instead of the `-dynamiclib` flag. I found a fix by reading the documentation: > For shared libraries, the MACHO_COMPATIBILITY_VERSION and MACHO_CURRENT_VERSION > properties can be used to override the compatibility version and current > version respectively. Setting these two flags to `OFF` removes `-compatibility_version` and `-current_version`. Perhaps we should simply remove `SOVERSION` and `VERSION`, as they're only meant to be used for shared libraries? ## Second issue OpenSSL has an API to load dynamic objects and libraries called `DSO`. When loading a provider, a call to `dlfcn_name_converter` is made to compute the name of the file: https://github.com/openssl/openssl/blob/79fa5873a9c2f2df392bc4b39146d8db37640118/crypto/dso/dso_dlfcn.c#L253-L280 Reader may notice that OpenSSL appends the macro `DSO_EXTENSION` to the end of the provider's name. `DSO_EXTENSION` is platform-specific: its value is `.so` on Unix, and `.dylib` on macOS. Because we're building a `MODULE` library here instead of a `SHARED` library, the output file's extension is `.so` instead of `.dylib`. Thus, OpenSSL fails to find the `oqs-provider` to load. To solve this issue, I used the [`SUFFIX`](https://cmake.org/cmake/help/latest/prop_tgt/SUFFIX.html) property of CMake targets to explicitely specify the extension when building for a XNU-based platform (macOS, iOS, watchOS, etc.). also change suffix on Windows. adapt CI to MODULE-defined paths.
Merged: Thanks for the contribution, @thb-sb! |
PR [#207] introduced a check for setting the right suffix to the library, depending on the target OS: https://github.com/open-quantum-safe/oqs-provider/blob/823f3178dd50eeb5febf29eb82680400c4d9e887/oqsprov/CMakeLists.txt#L61-L7://github.com/open-quantum-safe/oqs-provider/blob/823f3178dd50eeb5febf29eb82680400c4d9e887/oqsprov/CMakeLists.txt#L61-L79 However, mixed with PR [#201] and the `OQS_PROVIDER_BUILD_STATIC` option, the library may be suffixed with the wrong extension: we may end up with a static library named `oqsprovider.dylib`, even if it's an archive: ```shell $ cmake -GNinja \ -DOQS_PROVIDER_BUILD_STATIC=ON \ -B build $ cmake --build build $ file build/lib/oqsprovider.dylib build/lib/oqsprovider.dylib: current ar archive random library $ # ^ should be named `oqsprovider.a`! ``` The check mentioned above is only relevant when oqsprovider is built as a module. This commit fixes this bug and introduces a check in the CircleCI jobs to verify that `oqsprovider.a` is actually produced. [#201](#201) [#207](#207)
* Install only oqsprovider.so after open-quantum-safe/oqs-provider#207 * Enable additional algorithsm (see open-quantum-safe/oqs-provider#210)
* Install only oqsprovider.so after open-quantum-safe/oqs-provider#207 * Enable additional algorithsm (see open-quantum-safe/oqs-provider#210)
Fix open-quantum-safe#202. This commit switches the default type in CMake from `SHARED` to `MODULE`. Using [`SUFFIX`](https://cmake.org/cmake/help/latest/prop_tgt/SUFFIX.html) property of CMake targets to explicitly specify the module extension when building for a XNU-based platform (macOS, iOS, watchOS, etc.) and Windows such as to allow OpenSSL to find the provider on a given platform. Signed-off-by: Felipe Ventura <felipe.ventura@entrust.com>
PR [open-quantum-safe#207] introduced a check for setting the right suffix to the library, depending on the target OS: https://github.com/open-quantum-safe/oqs-provider/blob/823f3178dd50eeb5febf29eb82680400c4d9e887/oqsprov/CMakeLists.txt#L61-L7://github.com/open-quantum-safe/oqs-provider/blob/823f3178dd50eeb5febf29eb82680400c4d9e887/oqsprov/CMakeLists.txt#L61-L79 However, mixed with PR [open-quantum-safe#201] and the `OQS_PROVIDER_BUILD_STATIC` option, the library may be suffixed with the wrong extension: we may end up with a static library named `oqsprovider.dylib`, even if it's an archive: ```shell $ cmake -GNinja \ -DOQS_PROVIDER_BUILD_STATIC=ON \ -B build $ cmake --build build $ file build/lib/oqsprovider.dylib build/lib/oqsprovider.dylib: current ar archive random library $ # ^ should be named `oqsprovider.a`! ``` The check mentioned above is only relevant when oqsprovider is built as a module. This commit fixes this bug and introduces a check in the CircleCI jobs to verify that `oqsprovider.a` is actually produced. [open-quantum-safe#201](open-quantum-safe#201) [open-quantum-safe#207](open-quantum-safe#207) Signed-off-by: Felipe Ventura <felipe.ventura@entrust.com>
Fix #202.
This commit switches the default type in CMake from
SHARED
toMODULE
.Two issues have been found.
First issue
I ran into an issue while testing this commit on macOS. Here is the error I got from clang:
After some investigation, I found out that the
SOVERSION
andVERSION
properties should not be used when building a shared library. This doesn't seem to lead to an error on Linux, but on macOS, it happens to set the following two flags:-compatibility_version
(given bySOVERSION
)-current_version
(given byVERSION
)However, CMake uses the
-bundle
flag when linking, instead of the-dynamiclib
flag.I found a fix by reading the documentation:
Setting these two flags to
OFF
removes-compatibility_version
and-current_version
.Perhaps we should simply remove
SOVERSION
andVERSION
, as they're only meant to be used for shared libraries?Second issue
OpenSSL has an API to load dynamic objects and libraries called
DSO
. When loading a provider, a call todlfcn_name_converter
is made to compute the name of the file:https://github.com/openssl/openssl/blob/79fa5873a9c2f2df392bc4b39146d8db37640118/crypto/dso/dso_dlfcn.c#L253-L280
Reader may notice that OpenSSL appends the macro
DSO_EXTENSION
to the end of the provider's name.DSO_EXTENSION
is platform-specific: its value is.so
on Unix, and.dylib
on macOS.Because we're building a
MODULE
library here instead of aSHARED
library, the output file's extension is.so
instead of.dylib
. Thus, OpenSSL fails to find theoqs-provider
to load.To solve this issue, I used the
SUFFIX
property of CMake targets to explicitely specify the extension when building for a XNU-based platform (macOS, iOS, watchOS, etc.).