Skip to content

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

Closed
ajbozarth opened this issue Oct 5, 2023 · 4 comments
Closed

The liboqs_DIR env var should be capitalized #273

ajbozarth opened this issue Oct 5, 2023 · 4 comments

Comments

@ajbozarth
Copy link
Member

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 to LIBOQS_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.

@baentsch
Copy link
Member

baentsch commented Oct 6, 2023

I am willing to open a PR with this change if the community is ok with it.

It's arguably more whether cmake is ok with it on all platforms: Looking forward to the PR.

@ghost
Copy link

ghost commented Oct 6, 2023

Hello @ajbozarth,

Environmental variable convention is to capitalized the entire variable name

Although I agree with you on that point, I think we don't have the choice here, since it's specific to the find_package rule.
The documentation states the following:

Search paths specified in cmake-specific environment variables. These are intended to be set in the user's shell configuration, and therefore use the host's native path separator (; on Windows and : on UNIX). This can be skipped if NO_CMAKE_ENVIRONMENT_PATH is passed or by setting the CMAKE_FIND_USE_CMAKE_ENVIRONMENT_PATH to FALSE:

  • <PackageName>_DIR

The liboqs CMake package is called liboqs and not LIBOQS, so liboqs_DIR has to be used.

Also, using environment variables to set values in CMake isn't usually a good idea. Setting cache entries using -DXXX=YYY at configure time is better.

Anyway, the following diff should allow people to use LIBOQS_DIR or liboqs_DIR, as cache entries or environment variables:

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)

@baentsch
Copy link
Member

baentsch commented Oct 6, 2023

Thanks for making this cmake naming dependency (more) explicit (than I did), @thb-sb. Maybe you'd like to start the PR with your patch and @ajbozarth can complete with changing capitalization in the places he considers important?

@ajbozarth
Copy link
Member Author

ajbozarth commented Oct 6, 2023

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

@open-quantum-safe open-quantum-safe locked and limited conversation to collaborators Oct 7, 2023
@baentsch baentsch converted this issue into discussion #277 Oct 7, 2023

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants