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

Support OpenSSL on Windows #915

Merged
merged 5 commits into from
Feb 23, 2021
Merged

Support OpenSSL on Windows #915

merged 5 commits into from
Feb 23, 2021

Conversation

thomwiggers
Copy link
Member

Attempts to add support for OpenSSL on Windows. We probably should add this to the CI matrix as well.

See also open-quantum-safe/liboqs-rust#17

@thomwiggers thomwiggers marked this pull request as draft February 15, 2021 14:57
@thomwiggers
Copy link
Member Author

It might be better to not include rand_nist in common anyway, if it's only necessary for tests.

@thomwiggers thomwiggers marked this pull request as ready for review February 16, 2021 14:03
@thomwiggers
Copy link
Member Author

thomwiggers commented Feb 16, 2021

AFAICT it works with openssl with this change.

See also open-quantum-safe/liboqs-rust#30

@xvzcf
Copy link
Contributor

xvzcf commented Feb 16, 2021

Nice. We should probably also remove/rework the Windows guard here.

@dstebila
Copy link
Member

This is my first time dealing with testapproval. Should we run the tests being held? Who would trigger that? Or can we decide it is not necessary for this PR (since the remaining tests are arm tests, unaffected by this PR) and just merge as is?

@baentsch
Copy link
Member

baentsch commented Feb 18, 2021

Who would trigger that? Or can we decide it is not necessary for this PR (since the remaining tests are arm tests, unaffected by this PR) and just merge as is?

I'd suggest we run these tests once to (have the system) ascertain that indeed this doesn't impact the other platforms. As all else passed I just triggered those runs (edit: and they passed while you were sleeping :). We then now have a clean decision base to approve/merge. I personally would make the actual merge decision dependent on whether @xvzcf's comment

Nice. We should probably also remove/rework the Windows guard here.

is addressed in this PR or via creation of a new issue.

@thomwiggers
Copy link
Member Author

WRT the Windows guards, I'm not exactly sure what's going on there.

@thomwiggers
Copy link
Member Author

From the meeting minutes

Note that this should also allow us to build with BIKE support on Windows.

So probably this PR should also go into that.

@thomwiggers
Copy link
Member Author

Why was BIKE even disabled on Win32 if that's just because of OpenSSL, because it works fine without OpenSSL on Linux and Mac...

@xvzcf
Copy link
Contributor

xvzcf commented Feb 18, 2021

Why was BIKE even disabled on Win32 if that's just because of OpenSSL, because it works fine without OpenSSL on Linux and Mac...

Yeah, I went back to my original CMake commit to jog my memory, and it turns out I misspoke in the meeting (my bad). I disabled BIKE on Windows because it was not listed as being supported here, not because of OpenSSL; and from quickly skimming through the PR's most recent AppVeyor logs, it seems a bit more work might be required to get BIKE working on Windows.

@thomwiggers
Copy link
Member Author

The Windows guard was changes so that OpenSSL is disabled by default on Windows and enabled by default otherwise.

BIKE is unavailable on Windows now, and enabled by default otherwise using cmake_dependent_option:

 cmake_dependent_option(OQS_ENABLE_KEM_BIKE "" ON "NOT WIN32" OFF)

@baentsch baentsch merged commit d9e0258 into main Feb 23, 2021
@baentsch baentsch deleted the allow-openssl-windows branch February 23, 2021 05:12
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.

4 participants