Should liboqs_DIR env var be capitalized?

[ajbozarth]

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]

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]

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]

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]

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

[baentsch]

With #275 closed I'd suggest we also close this discussion. Alternatively, can I suggest you provide a PR dealing with all cmake "peculiarities" surfaced, @ajbozarth ?

[ajbozarth]

ok, I'll brainstorm how I think we can update the docs to clarify this "peculiarity" and close this when that is open (I'll prob get to it this week)

[baentsch]

Closed by #292