build: enhance FYPP error messaging and add defensive checks in stdlib.cmake#1137
Conversation
|
Thank you for the detailed feedback, @jalvesz. I completely agree with your point regarding the separation of responsibilities; the build system should provide diagnostic tools rather than assuming a specific installation path, especially given the variety of environments like Conda, MSYS2, or standard Python installs. I will fix the accidental logic flip in cmake/stdlib.cmake to ensure the message correctly displays 'Disable' when a module is not compiled. I will update the CMakeLists.txt error message to remove the specific Windows roaming paths. Instead, I will include the where (Windows) and which (Linux/macOS) commands you suggested to help users verify their own environments. I will keep the instruction for the -DFYPP_EXECUTABLE flag, as it remains the standard way for users to manually point to the preprocessor if the automatic search fails. Please let me know @jalvesz if this plan aligns with your expectations. Once you confirm, I will push the updated commits to this PR. |
|
Sorry by mistake i made a commit with a wrong comment written in it , I changed that and the commit named"Final Changes" is the correct one .Kindly please review this one @jalvesz |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1137 +/- ##
=======================================
Coverage 67.99% 67.99%
=======================================
Files 404 404
Lines 12935 12935
Branches 1392 1392
=======================================
Hits 8795 8795
Misses 4140 4140 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jvdp1
left a comment
There was a problem hiding this comment.
Overall it looks good to me. Here are some suggestions.
| # Detect ILP64 (common function) | ||
| function(detect_ilp64 lib_name) | ||
| set(${lib_name}_ILP64 False PARENT_SCOPE) | ||
| # Prefer checking BLA_SIZEOF_INTEGER (available in CMake >= 3.22) |
There was a problem hiding this comment.
Why did you remove these two comments? They seem useful to me.
| if(NOT FYPP) | ||
| message(FATAL_ERROR "Preprocessor fypp not found! Please install fypp following the instructions in https://fypp.readthedocs.io/en/stable/fypp.html#installing") | ||
| if(NOT FYPP OR "${FYPP}" MATCHES "NOTFOUND") | ||
| message(STATUS "Checking for fypp...") |
There was a problem hiding this comment.
| message(STATUS "Checking for fypp...") |
What is the goal of this message? I propose to remove it
…fix-fypp-error-message :qw :wq
There was a problem hiding this comment.
Pull request overview
This PR improves the CMake configuration experience when the fypp preprocessor is missing by adding clearer diagnostics and a defensive guard in the preprocessing pipeline to avoid later-stage “empty command” build failures.
Changes:
- Enhance the top-level CMake failure message for missing
fyppwith actionable diagnostic steps. - Add a defensive check in
cmake/stdlib.cmaketo fail early if the preprocessor command/path is invalid. - Minor formatting/comment cleanup in CMake files.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
CMakeLists.txt |
Improves fypp-missing fatal error messaging and adjusts formatting. |
cmake/stdlib.cmake |
Adds a guard in preprocess() to prevent invalid/empty custom command invocations; minor whitespace/comment tweaks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| " Windows: 'where fypp' and 'where python'\n" | ||
| " Linux/macOS: 'which fypp' and 'which python'\n\n" | ||
| " Ensure the executable is in your PATH or set it manually during configuration:\n" | ||
| " cmake -B build -DFYPP_EXECUTABLE=/path/to/fypp\n" |
There was a problem hiding this comment.
The error message instructs users to pass -DFYPP_EXECUTABLE=..., but this project actually uses the FYPP variable from find_program(FYPP fypp) (and cmake/stdlib.cmake calls preprocess("${FYPP}" ...)). As written, the suggested flag won’t have any effect and may further confuse users; align the hint with the actual cache variable (e.g., -DFYPP=/path/to/fypp) or switch the build to consistently use a FYPP_EXECUTABLE cache variable.
| " cmake -B build -DFYPP_EXECUTABLE=/path/to/fypp\n" | |
| " cmake -B build -DFYPP=/path/to/fypp\n" |
| set(LIB_MOD_DIR ${CMAKE_CURRENT_BINARY_DIR}/mod_files/${target_name}/) | ||
| #set(INSTALL_MOD_DIR "${CMAKE_INSTALL_MODULEDIR}/${target_name}") | ||
| set(INSTALL_MOD_DIR "${CMAKE_INSTALL_MODULEDIR}") | ||
|
|
There was a problem hiding this comment.
There’s trailing whitespace on this blank line (the file has a whitespace-only line after set(INSTALL_MOD_DIR ...)). .editorconfig enforces trim_trailing_whitespace = true for *.cmake, so this should be removed to avoid style/lint issues.
jvdp1
left a comment
There was a problem hiding this comment.
Thank you @srinjoy933 . I left only one minor comment. Otherwise, I think it is ready to be merged.
| # Prefer checking BLA_SIZEOF_INTEGER (available in CMake >= 3.22) | ||
| if(DEFINED BLA_SIZEOF_INTEGER AND BLA_SIZEOF_INTEGER EQUAL 8) | ||
| set(${lib_name}_ILP64 True PARENT_SCOPE) | ||
| # Fallback: Check BLA_VENDOR manually for signs of ILP64 |
There was a problem hiding this comment.
Please re-insert this comment. It seems usefull to me
|
hey @jvdp1 I made the change that you have suggested .Please have a look is it fine now. |
Solves #1136
This PR enhances the developer onboarding experience by replacing generic build failures with actionable diagnostic messages. While setting up the environment on Windows 11, I found that a missing fypp in the system PATH (common with pip installs) leads to a "silent failure" .
Changes:
Actionable Errors: Updated the root CMakeLists.txt to suggest specific fixes, including checking the Python Scripts folder and using the -DFYPP_EXECUTABLE flag.
Defensive Programming: Added a guard clause in cmake/stdlib.cmake to validate the preprocessor path before generating build commands, preventing "empty command" errors.