-
Notifications
You must be signed in to change notification settings - Fork 96
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
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
The liboqs_DIR env var should be capitalized #273
Comments
It's arguably more whether |
Hello @ajbozarth,
Although I agree with you on that point, I think we don't have the choice here, since it's specific to the
The liboqs CMake package is called Also, using environment variables to set values in CMake isn't usually a good idea. Setting cache entries using Anyway, the following diff should allow people to use diff --git a/CMakeLists.txt b/CMakeLists.txt
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -61,6 +61,12 @@ get_filename_component(OPENSSL_LIB_DIR $
set(OPENSSL_MODULES_PATH ${OPENSSL_LIB_DIR}/ossl-modules)
endif()
+if (LIBOQS_DIR)
+ set(liboqs_DIR "${LIBOQS_DIR}" CACHE PATH "path to liboqs configuration directory" FORCE)
+elseif (DEFINED ENV{LIBOQS_DIR})
+ set(liboqs_DIR "$ENV{LIBOQS_DIR}" CACHE PATH "path to liboqs configuration directory" FORCE)
+endif()
+
# Add required include for liboqs
find_package(liboqs REQUIRED)
get_target_property(LIBOQS_INCLUDE_DIR OQS::oqs INTERFACE_INCLUDE_DIRECTORIES) |
Thanks for making this |
Thank you for all the quick feedback, knowing that it's a generated name from cmake does explain why I couldn't trace the error message within the repo. Once #275 is merged I'll open my PR, it's already viewable at main...ajbozarth:oqs-provider:liboqsvar |
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
Environmental variable convention is to capitalized the entire variable name, however liboqs_DIR does not follow this. It is also the only env var detailed in the config docs that uses lowercase https://github.com/open-quantum-safe/oqs-provider/blob/main/CONFIGURE.md#liboqs_dir
I suggest we change
liboqs_DIR
toLIBOQS_DIR
in all docs, code, and scripts to match convention. I believe we should make this change sooner rather than later to avoid user/dev thrash.I am willing to open a PR with this change if the community is ok with it.
The text was updated successfully, but these errors were encountered: