-
Notifications
You must be signed in to change notification settings - Fork 92
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
Updates for clean builds across iOS / macOS / Android (all platforms) #363
Conversation
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.
Thanks for the contribution, @andybrucenet . Can you please explain the benefit of this script to (other) users of oqsprovider
, e.g. by way of addition to README.md or USAGE.md? Related question: Why are there no tests for this script (ascertaining delivery of those benefits)? Have tests been done to ensure the changes do no interfere with downstream deployments of oqsprovider
, e.g., in oqs-demos?
@@ -0,0 +1,1101 @@ | |||
# SPDX-License-Identifier: BSD-3-Clause |
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.
This would change the nature of the project as a "clean" MIT-licensed project.
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.
The script enables versioned and separated output for easy consumption by consumers who prefer a single command to build all platform outputs at one time. However, it's of lesser value to generic users as it is opinionated.
From a tests perspective, unsure how those would be implemented due to the script requiring the use of openssl. Particularly, openssl output in the same versioned structure. Some openssl build projects use similar variants to this structure but which openssl build project is used depends on the consumer's needs. In my case, I leverage openssl_for_android for wrapping droid builds and OpenSSL for iOS / macOS builds.
The only real required changes are for the compile mods listed in the PR. I had hopes the script would be useful to others but it's not required.
I can modify the PR to exclude the script and simply submit the compile changes as on second thought the script is too specific to be of general use.
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.
I had hopes the script would be useful to others but it's not required
I think it does -- but I'm unsure whether this is the right place. What about giving it a home in oqs-demos next to the Docker image, promising to deliver a cross-platform binary? This then basically serves the same purpose as the Dockerfile, namely to alleviate users of the need to have to pick-and-parcel components to get a runnable image.
on second thought the script is too specific to be of general use
This I'd second.
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.
As we agree this is too specific to be of general use (and as it doesn't have a dedicated CI run to ensure it "stays good" and has code with different licensing terms) I don't think this script should be within oqs-provider
. That said, I do think it has value, but may better be placed within oqs-demos. Tagging @SWilson4 @praveksharma for their opinion. I'd think the same argumentation goes for the related PR in liboqs.
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.
As we agree this is too specific to be of general use (and as it doesn't have a dedicated CI run to ensure it "stays good" and has code with different licensing terms) I don't think this script should be within
oqs-provider
. That said, I do think it has value, but may better be placed within oqs-demos. Tagging @SWilson4 @praveksharma for their opinion. I'd think the same argumentation goes for the related PR in liboqs.
I agree that oqs-demos location would be a better home. That said, I don't think I have bandwidth to offer support on or maintain this script, and I would hesitate to add another component into a subproject for which we already have tenuous support issues (i.e., red CI) without some sort of maintenance commitment (probably from @andybrucenet).
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.
@andybrucenet Would/Could you commit to maintaining this script when it'd land in oqs-demos
? Either way, I'd suggest to close it here requesting a PR at oqs-demos
if you'd be willing to provide a test for it there as well as maintain it. The same thought applies to open-quantum-safe/liboqs#1717.
@@ -27,3 +27,6 @@ scripts/__pycache__ | |||
# MacOS | |||
.DS_Store | |||
|
|||
# CMake & testing | |||
/build* |
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.
Why not keep using the existing (already excluded) _build and tmp folders?
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.
To make oqs-provider match the liboqs folder structure, no other reason than that.
@andybrucenet I noticed the DCO check failed The DCO process is required to assert a few things about the contribution. See DCO documentation. Hope that helps |
@planetf1 please see open-quantum-safe/liboqs#1760 |
Closing as there does not seem to be readiness to commit to maintain this script, nor to resolve the CI failures. If you want to re-consider @andybrucenet please re-open as PR to oqs-demos. |
We will be integrating oqs-provider with our specific static liboqs / openssl builds. This fork ensures that we can build static oqs-provider libraries for all platforms.
Specific changes:
The required changes include the <stdint.h> include and the CMakeLists mods.
There are no known issues hit by this PR.