Skip to content
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

Merged
merged 1 commit into from Jul 7, 2023
Merged

Build a module instead of a shared library. #207

merged 1 commit into from Jul 7, 2023

Conversation

ghost
Copy link

@ghost ghost commented Jul 4, 2023

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:

clang: error: invalid argument '-compatibility_version 1.0.0' only allowed with '-dynamiclib'

After some investigation, I found out that the SOVERSION and VERSION 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 property of CMake targets to explicitely specify the extension when building for a XNU-based platform (macOS, iOS, watchOS, etc.).

@ghost ghost marked this pull request as ready for review July 4, 2023 11:59
@ghost ghost requested a review from baentsch as a code owner July 4, 2023 11:59
@baentsch
Copy link
Member

baentsch commented Jul 4, 2023

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).

Perhaps we should simply remove SOVERSION and VERSION, as they're only meant to be used for shared libraries?

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 oqsprovider it contains? Or would one simply have to load it and query it? Certainly doable. But whichever way it's done, would some form of documentation be advisable in "USAGE.md"? Or would it be preposterous to assume users just getting their hands on an oqsprovider.dylib/so/dll and wanting to know which version it is?

Copy link
Member

@baentsch baentsch left a 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).

@baentsch
Copy link
Member

baentsch commented Jul 5, 2023

@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?

@ghost
Copy link
Author

ghost commented Jul 6, 2023

@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).

Ho great, thank you very much!

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?

I guess you don't have w access by default on my fork, explaining why the changes did not show up here?
Anyway, I pulled your changes and folded everything :)

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.
@baentsch baentsch merged commit 466a537 into open-quantum-safe:main Jul 7, 2023
@baentsch
Copy link
Member

baentsch commented Jul 7, 2023

Merged: Thanks for the contribution, @thb-sb!

@ghost ghost mentioned this pull request Jul 19, 2023
baentsch pushed a commit that referenced this pull request Jul 20, 2023
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)
iyanmv added a commit to iyanmv/PKGBUILDs that referenced this pull request Jul 22, 2023
* Install only oqsprovider.so after open-quantum-safe/oqs-provider#207
* Enable additional algorithsm (see open-quantum-safe/oqs-provider#210)
archlinux-github pushed a commit to archlinux/aur that referenced this pull request Jul 22, 2023
* Install only oqsprovider.so after open-quantum-safe/oqs-provider#207
* Enable additional algorithsm (see open-quantum-safe/oqs-provider#210)
@ghost ghost deleted the module branch August 31, 2023 08:28
feventura pushed a commit to EntrustCorporation/oqs-provider that referenced this pull request Mar 16, 2024
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>
feventura pushed a commit to EntrustCorporation/oqs-provider that referenced this pull request Mar 16, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change build (back) to "MODULE"
1 participant