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

Fix a bug in the CMake script. #221

Merged
merged 1 commit into from
Jul 20, 2023
Merged

Conversation

thb-sb
Copy link
Member

@thb-sb thb-sb commented Jul 19, 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:

$ 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
#207

@thb-sb thb-sb requested a review from baentsch as a code owner July 19, 2023 11:51
@baentsch
Copy link
Member

Hmm -- CI fails... And the macOS log makes we wonder (again): Why in the world is liboqs again getting built using OpenSSL111? It seems brew _de_installs openssl@3 (??) Also, this seems to introduce even more "suffix surprises": "macOS-static" building a ".dylib"??

@thb-sb
Copy link
Member Author

thb-sb commented Jul 19, 2023

Hmm -- CI fails...
Indeed, -DOQS_PROVIDER_BUILD_STATIC=ON was missing in one of the blocks in the CircleCI configuration.
I added it: https://github.com/open-quantum-safe/oqs-provider/compare/7a36cc79b1450baa1aa40204db5225e6a858224d..8e0c642cdee3351220863a9a179168e1e2ee2968

Why in the world is liboqs again getting built using OpenSSL111? It seems brew _de_installs openssl@3 (??)

No, the VM has both OpenSSL 1.1.1 and 3.1. Unlike oqs-provider which requires OpenSSL >3, liboqs doesn't provide a minimum required version for OpenSSL, so 1.1.1 is picked.

Do you want me to try to fix that? I guess we could just do brew uninstall [email protected].
(edit: draft: #222, using OPENSSL_ROOT_DIR).

Also, this seems to introduce even more "suffix surprises": "macOS-static" building a ".dylib"??

This is precisely the subject of this PR! It shouldn't have built a dylib in the first place, this was a mistake of mine.

@baentsch
Copy link
Member

@thb-sb Linux again ... builds a .so so the new test for .a fails...

@thb-sb
Copy link
Member Author

thb-sb commented Jul 19, 2023

@thb-sb Linux again ... builds a .so so the new test for .a fails...

Fixed in https://github.com/open-quantum-safe/oqs-provider/compare/8e0c642cdee3351220863a9a179168e1e2ee2968..0a64fdb4fcda5bfc7ce5e7a05a602d58858aa54e (another block I forgot…)

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

Thanks for the fix!

@baentsch baentsch merged commit 4b7f545 into open-quantum-safe:main Jul 20, 2023
19 checks passed
@thb-sb thb-sb deleted the bug_cmake branch August 31, 2023 08:28
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 <[email protected]>
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.

2 participants