-
Notifications
You must be signed in to change notification settings - Fork 39
Inform user of missing dependencies instead #33
Conversation
Instead of failing with a cryptic error message like ``` CMake Error at /tmp/Arduino-CMake-NG/cmake/Platform/System/LinuxDistDetector.cmake:12 (string): string no output variable specified Call Stack (most recent call first): /tmp/Arduino-CMake-NG/cmake/Platform/System/BuildSystemInitializer.cmake:25 (detect_host_linux_distribution) /tmp/Arduino-CMake-NG/cmake/Platform/Arduino.cmake:34 (initialize_build_system) /usr/share/cmake-3.10/Modules/CMakeSystemSpecificInformation.cmake:26 (include) CMakeLists.txt:2 (project) ``` let's fail with a clean message about missing dependencies :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ooxi Welcome to the project and thank you very much for this contribution!
You've noticed a very subtle bug and I really appreciate the effort to fix it yourself!
It looks good, I just have a few notes for you to look at in the review comments 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FATAL_ERROR fails the whole CMake process, which is something we don't really want to do here since the dist-detection module is not part of the framework's core, just a nice-to-have feature 😃
Instead, use WARNING to inform the user that it won't benefit from this module until it installs missing dependencies.
Besides, I think it'd be more informative if the message also stated that "Linux distribution couldn't be detected", then the exact reason why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fast feedback, I'll update my pull request!
Instead of failing with a cryptic error message like
let's fail with a clean message about missing dependencies :-)