Skip to content

Prefix CMake options with QJS_ ? #896

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

Closed
past-due opened this issue Feb 7, 2025 · 3 comments · Fixed by #897
Closed

Prefix CMake options with QJS_ ? #896

past-due opened this issue Feb 7, 2025 · 3 comments · Fixed by #897

Comments

@past-due
Copy link
Contributor

past-due commented Feb 7, 2025

A best-practice recommendation for CMake projects - especially those used as libraries - is to prefix option names with the project prefix / name. (Example: https://discourse.cmake.org/t/best-practices-for-option-naming/2039 )

The CMakeLists.txt currently exposes various build options that would benefit from this:

xoption(BUILD_EXAMPLES "Build examples" OFF)
xoption(BUILD_STATIC_QJS_EXE "Build a static qjs executable" OFF)
xoption(BUILD_CLI_WITH_MIMALLOC "Build the qjs executable with mimalloc" OFF)
xoption(BUILD_CLI_WITH_STATIC_MIMALLOC "Build the qjs executable with mimalloc (statically linked)" OFF)
xoption(CONFIG_ASAN "Enable AddressSanitizer (ASan)" OFF)
xoption(CONFIG_MSAN "Enable MemorySanitizer (MSan)" OFF)
xoption(CONFIG_TSAN "Enable ThreadSanitizer (TSan)" OFF)
xoption(CONFIG_UBSAN "Enable UndefinedBehaviorSanitizer (UBSan)" OFF)

Would become:

xoption(QJS_BUILD_EXAMPLES "Build examples" OFF)
xoption(QJS_BUILD_STATIC_EXE "Build a static qjs executable" OFF)
xoption(QJS_BUILD_CLI_WITH_MIMALLOC "Build the qjs executable with mimalloc" OFF)
xoption(QJS_BUILD_CLI_WITH_STATIC_MIMALLOC "Build the qjs executable with mimalloc (statically linked)" OFF)
xoption(QJS_ENABLE_ASAN "Enable AddressSanitizer (ASan)" OFF)
xoption(QJS_ENABLE_MSAN "Enable MemorySanitizer (MSan)" OFF)
xoption(QJS_ENABLE_TSAN "Enable ThreadSanitizer (TSan)" OFF)
xoption(QJS_ENABLE_UBSAN "Enable UndefinedBehaviorSanitizer (UBSan)" OFF)

Additionally, BUILD_QJS_LIBC could be renamed QJS_BUILD_LIBC.

@bnoordhuis
Copy link
Contributor

I don't strenuously object. Pull request welcome, I suppose?

CONFIG_ASAN etc. were carried over from the old Makefile. QJS_CONFIG_ASAN reads awkward. Let's use QJS_ENABLE_ASAN or something like that instead?

@saghul
Copy link
Contributor

saghul commented Feb 7, 2025

I concur with Ben 👍

@past-due
Copy link
Contributor Author

past-due commented Feb 7, 2025

Opened PR #897, incorporating the feedback on naming (QJS_ENABLE_ASAN, etc)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants