search

open-quantum-safe / liboqs

C library for prototyping and experimenting with quantum-resistant cryptography
https://openquantumsafe.org/
Other
1.78k stars 436 forks source link

Figure out a way to report unused/uninitialized CMake project variables. #865

Closed xvzcf closed 3 years ago

xvzcf commented 3 years ago

To help prevent issues like https://github.com/open-quantum-safe/liboqs/issues/859 in the future, it would be nice if CMake could list all unused/uninitialized variables in the project.

CMake has the --warn-uninitialized and --warn-unused-vars flags; the former behaves inconsistently among different CMake versions (and in CMake 3.19 did not catch https://github.com/open-quantum-safe/liboqs/issues/859); the latter is broken and has been removed in CMake 3.19.

baentsch commented 3 years ago

After some experimentation there is a solution: We'd simply need to diligently reference all variables as such in our cmake files, i.e., in the canonical cmake notation "${VARIABLE_NAME}".

If doing this, --warn-uninitialized works correctly (and would have caught #859). If using the "shortcut notation" of (a variable as) a constant, e.g., as in if(VARIABLE_NAME) the warning is not issued. This arguably is OK, as variables and constants are different and --warn-uninitialized is documented to operate only on variables.

Thus, I see three alternative ways forward to avoid problems such as #859 in the future and close this issue: 1) We are using "full" variable notation everywhere and run cmake with --warn-uninitialized in CI to catch future issues such as #859. This means rewriting all cmake-code from e.g., if(OQS_USE_OPENSSL) to if(${OQS_USE_OPENSSL}). 2) We create an issue with cmake to get variables and constants treated identically with regard to --warn-uninitialized and see whether/if this gets resolved. I wouldn't hold my breath for that, though. 3) We write our own parser (or piggyback on cmake output features like --trace-expand) to detect incorrectly written variable names ourselves. But I'd consider that a bit of a wasted effort.

@xvzcf : Do you see other alternatives? OK to go with option 1? Everyone should adhere to that then, though. I volunteer to re-write all cmake code to close this issue following option 1.

xvzcf commented 3 years ago

Nice find, and yeah option 1 seems like the best way forward.

baentsch commented 3 years ago

OK - will implement then. When starting, copy_from_upstream.py (generating quite a few of the not-quite-correct cmake instructions) fails, though:

>:~/git/oqs/liboqs-copy/scripts/copy_from_upstream$ python3 copy_from_upstream.py copy
Traceback (most recent call last):
  File "copy_from_upstream.py", line 580, in <module>
    copy_from_upstream()
  File "copy_from_upstream.py", line 491, in copy_from_upstream
    process_families(instructions, os.environ['LIBOQS_DIR'], True, True)
  File "copy_from_upstream.py", line 481, in process_families
    extensions={"tables"})
  File "/home/mib/.local/lib/python3.6/site-packages/mdformat/_api.py", line 57, in file
    codeformatters=codeformatters,
  File "/home/mib/.local/lib/python3.6/site-packages/mdformat/_api.py", line 22, in text
    codeformatters=codeformatters,
  File "/home/mib/.local/lib/python3.6/site-packages/mdformat/_util.py", line 32, in build_mdit
    plugin = mdformat.plugins.PARSER_EXTENSIONS[name]
KeyError: 'tables'

--> Does this ring a bell, @bhess ? Do I miss something? pip3 install mdformat markdown run before...

bhess commented 3 years ago

It looks like mdformat-tables is missing. @baentsch could you try pip3 install mdformat-tables or pip3 install -r requirements.txt?

baentsch commented 3 years ago

@baentsch could you try pip3 install mdformat-tables or pip3 install -r requirements.txt?

Perfect&Thanks: Just overlooked the requirements.txt :-(

baentsch commented 3 years ago

Nice find, and yeah option 1 seems like the best way forward.

Caveat, after digging in to the details: The "automatic" variables (declared or not) in https://github.com/open-quantum-safe/liboqs/blob/main/src/oqsconfig.h.cmake make this very problematic: In order to not trigger warnings for all of them not present on a specific platform (e.g., if CPU flags are missing), they'd all need to be explicitly initialized. I'd consider this over the top again, particularly considering they're all automatically generated (e.g., via copy-from-upstream). I'll thus apply option 1 only to manually edited variables and leave the rest as-is. Objections before the PR, please :)