Build script to build liboqs#2417
Conversation
dcac969 to
32aacf1
Compare
baentsch
left a comment
There was a problem hiding this comment.
Thanks for this PR @charishma1407 ! I like it as it makes life easier for users -- and is even more complete than the similar build script we have in the even more user-focused oqs-provider project.
That said, some questions:
- Who is the target audience for this?
liboqsis a library that not too many users would (arguably, should :) build for themselves: One could argue that not having a build script excludes people from building the project that may have a propensity to "shoot themselves into the foot" having/wrongly using such script. - Why no Windows support?
- Why no CI (testing)?
- Who would maintain this going forward (e.g., when new config options or algs get added/changed)?
bhess
left a comment
There was a problem hiding this comment.
Hi @charishma1407, thanks for the PR, I agree this is a useful convenience for users. To echo and build on @baentsch's last points: my main concern is long‑term maintainability. The script hard‑codes build options and algorithms but isn't exercised in CI, so as new options or algorithms are added it could easily drift out of sync. Adding CI coverage for the script, or otherwise deriving its options from a single source of truth, would help reduce that risk. On the other hand, this might add quite some complexity that counters the goal to simplify the build.
|
Thanks @charishma1407 for adding thorough CI testing! This addresses those parts of the questions of @bhess and myself -- but it has one drawback: It requires an awful lot of compute cycles covering many --but surely not all-- current option-configs; also it will conceptually never be able to address the maintenance question (new config options added in the future). So allow me to make a "wild" proposal: Would it be conceivable that a) at minimum this PR adds a feature to the script to issue a build-time warning if it discovers that any config file it depends upon and that are checked in (e.g., CONFIGURE.md, CMakeLists.txt) is more current than the build script? Even better (but I don't know whether it's possible) would be b) that such "build-script-out-of-date" check (in CI) triggers a Copilot "agent" creating an "AI-written" update to the build script taking the latest config options into account and ideally, even generating a PR for that? |
|
Hi @baentsch and @bhess I made changes according to the comments
|
8a95d63 to
bc367cc
Compare
baentsch
left a comment
There was a problem hiding this comment.
Basically LGTM, see single comments (Yes, I should have grouped them into one coherent review :-()
6cd9dfb to
dc886ac
Compare
| echo "" | ||
|
|
||
| echo "Entering Nix development environment..." | ||
| nix develop |
There was a problem hiding this comment.
Have you tested the nix version? Claude says the following (but I don't use nix so I have no way of verifying whether this is a legitimate complaint or not):
nix developspawns an interactive subshell and then falls through to cmake/ninja in the parent shell outside the nix env — the NixOS path almost certainly doesn't work as intended. Eitherexec nix develop -c ./build_liboqs.sh --inside-nixstyle, or document that users shouldnix developthemselves first.
I'm also worried about maintainability going forward -- not that the script is unmaintainable, just that we will forget to update it. I think the staleness check is a good step. But probably we need some documentation reminding us? Maybe in the PR template? Or somewhere else? |
96aaa02 to
f94eca7
Compare
Signed-off-by: P Charishma Kumari <pcharishmakumari@Ps-MacBook-Pro.local>
Signed-off-by: P Charishma Kumari <pcharishmakumari@Ps-MacBook-Pro.local>
Signed-off-by: P Charishma Kumari <pcharishmakumari@Ps-MacBook-Pro.local>
Signed-off-by: P Charishma Kumari <pcharishmakumari@Mac.lan>
Signed-off-by: P Charishma Kumari <pcharishmakumari@mac.j9d-in.ibm.com>
- Tests script on Ubuntu and macOS with multiple configurations - Validates drift detection between script and CMake options - Includes syntax and quality checks - Addresses maintainer feedback about keeping script in sync Signed-off-by: P Charishma Kumari <pcharishmakumari@Ps-MacBook-Pro.local>
Signed-off-by: P Charishma Kumari <pcharishmakumari@Ps-MacBook-Pro.local>
Signed-off-by: P Charishma Kumari <pcharishmakumari@Ps-MacBook-Pro.local>
- Reduced test matrix from 13 to 4 essential configurations - 69% reduction in test jobs, ~70% reduction in compute time - Focuses on: default, minimal, shared+no-openssl, and cross-platform - Staleness detection provides early warning for drift - Addresses maintainer concern about compute cost Signed-off-by: P Charishma Kumari <pcharishmakumari@Ps-MacBook-Pro.local>
Signed-off-by: P Charishma Kumari <pcharishmakumari@Ps-MacBook-Pro.local>
Signed-off-by: P Charishma Kumari <pcharishmakumari@Ps-MacBook-Pro.local>
Signed-off-by: P Charishma Kumari <pcharishmakumari@mac.j9d-in.ibm.com>
…ed on CMake options Signed-off-by: P Charishma Kumari <pcharishmakumari@mac.j9d-in.ibm.com>
Signed-off-by: P Charishma Kumari <pcharishmakumari@mac.j9d-in.ibm.com>
Signed-off-by: P Charishma Kumari <pcharishmakumari@mac.j9d-in.ibm.com>
Signed-off-by: P Charishma Kumari <pcharishmakumari@mac.j9d-in.ibm.com>
Signed-off-by: P Charishma Kumari <pcharishmakumari@Ps-MacBook-Pro.local>
Signed-off-by: P Charishma Kumari <pcharishmakumari@Ps-MacBook-Pro.local>
Signed-off-by: P Charishma Kumari <pcharishmakumari@Ps-MacBook-Pro.local>
Signed-off-by: P Charishma Kumari <pcharishmakumari@Ps-MacBook-Pro.local>
Signed-off-by: P Charishma Kumari <pcharishmakumari@mac.j9d-in.ibm.com>
Signed-off-by: P Charishma Kumari <pcharishmakumari@mac.j9d-in.ibm.com>
Signed-off-by: P Charishma Kumari <pcharishmakumari@Ps-MacBook-Pro.local>
Signed-off-by: P Charishma Kumari <pcharishmakumari@Ps-MacBook-Pro.local>
Signed-off-by: P Charishma Kumari <pcharishmakumari@Ps-MacBook-Pro.local>
Signed-off-by: P Charishma Kumari <pcharishmakumari@Ps-MacBook-Pro.local>
f94eca7 to
16e2b28
Compare
|
Hi @xuganyu96
This is script is not intended to remove the dependency on manual installation or CMAKE usage. As documented user can also go with the manual installation steps also. |
Signed-off-by: P Charishma Kumari <pcharishmakumari@Ps-MacBook-Pro.local>
Purpose
This PR adds a build script (build_liboqs.sh) to simplify the compilation process for liboqs. The script automates the CMake configuration and build steps, making it easier for developers to build the library with common options.
Changes
Testing
The script has been tested locally and successfully builds the liboqs library.
Checklist
No - This PR only adds a build convenience script and does not modify any cryptographic algorithms.
No - This PR does not modify the algorithm list or any APIs. It only adds a build helper script.
AI Assistance
This contribution was created with the use of generative AI for builld-script-test.yml workflow