Conversation
Contributor
Author
|
@josephgarnier Can you pull this branch down and verify that it still works for you, please? |
ryanmrichard
approved these changes
Jul 28, 2025
|
Wow, great test coverage. With my fix, I had already noticed a display problem when the exception was thrown. I imagine that the problem also occurs with this PR. The code: message(FATAL_ERROR "REMOVE requires exactly 2 arguments, got ${nb_args}!")displays the status in text: [cmake] CMake Error at build/x64-Release-Win-GCC/_deps/cmake_test-src/cmake/cmake_test/overrides.cmake:113 (_message):
[cmake] Testing forcibly stopped, no further tests will be executed. Details:
[cmake] Uncaught exception: FATAL_ERROR;REMOVE requires exactly 2 arguments, got 1! # <================
[cmake] Call Stack (most recent call first):
[cmake] build/x64-Release-Win-GCC/_deps/cmake_test-src/cmake/cmake_test/overrides.cmake:48 (ct_exit)
[cmake] cmake/modules/Map.cmake:495 (message)
[cmake] cmake/modules/Map.cmake:192 (_map_remove)
[cmake] CMakeLists.txt:163 (map)I don't know enough about the internal mechanics of CMakeTest to have been able to fix it. |
Contributor
Author
|
Thanks for the heads up, I didn't think to check when the exception is thrown! I'll check it out. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Is this pull request associated with an issue(s)?
Closes #116
Description
Implements @josephgarnier's suggested fix for the issues described in #116.
TODOs
Testing
It is quite difficult to test this override to message functionality programmatically. However, I have tested this implementation manually with a slightly more extensive set than set forth in the issue and seen no issues. I have also only tested with
message(andmessage(STATUS, though, not the other possible message modes. Here is my testing method (on a Linux system):mkdir test_116_fix cd test_116_fixtest_116_fix/CMakeLists.txtfor the testing with the same contents as this file: https://github.com/user-attachments/files/21456416/CMakeLists.txt (contents are pasted below)cmake -H. -Bbuild -DFETCHCONTENT_SOURCE_DIR_CMAKE_TEST="$PWD/cmaketest"[cmaketest]output matches the[vanilla]output. (I just ran it in two side-by-side terminals and manually compared, but I'm sure you could usediffas well)Testing Project CMakeLists.txt contents
I understand potential hesitancy to download files from the Internet that you will need to run (even if it is uploaded on GitHub like the link above). I have included the contents of the the testing project
CMakeLists.txtbelow so you can see the file contents before downloading it or for reference if the link above stops working, but without interrupting the flow of the testing steps with a massive file.Contents of
CMakeLists.txt: