Inform user of missing dependencies #35
Conversation
MrPointer
left a comment
MrPointer
left a comment
There was a problem hiding this comment.
Haha, you shouldn't have closed the previous PR just to update the code - GitHub will handle it for you automatically and let me approve the PR.
Nevertheless, please take another look at my review since there are still some things bothering me, thanks!
| find_program(lsb_release_exec lsb_release) | ||
|
|
||
| if ("lsb_release_exec-NOTFOUND" STREQUAL "${lsb_release_exec}") | ||
| message(WARNING "Linux distribution couldn't be detected because `lsb_release' not found.") |
There was a problem hiding this comment.
There's a mismatch in the ticks here: `lsb_release' i.e The first is a back-tick, 2nd is a single-quote/tick.
I'd choose single-quotes here, so it'd become 'lsb_release'.
Also, I myself choose restrain from the word 'because' in technical information messages. IMHO a hyphen (-) is a much better choice, but I'll leave that for your judgement.
At last, I feel that saying a certain "thing" hasn't been found is not informative enough.
Correct me if I'm wrong, but I think that most of the users don't know that the lsb_release program is, so they'll just end up opening an issue here, asking for help. Instead, we could inform them that it's a package they need to install.
So I would rephrase it as such:
Linux distribution couldn't be detected - Please install 'lsb_release'.
There was a problem hiding this comment.
I agree with your suggestions!
|
I have reopened this pull request after applying the requested changes :-) |
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 :-)
2 participants
Instead of failing with a cryptic error message like
let's fail with a clean message about missing dependencies :-)