-
Notifications
You must be signed in to change notification settings - Fork 388
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
Use ${PROJECT_NAME} instead of writing projectname multiple times #134
Conversation
…ECT_NAME} variable Update CMakeLists.txt
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.
Hey, thanks for the PR! It definitely makes adapting the template easier. There seem to be some failed CI tests - could you run cmake-format
(or the CMake target fix-format
) and check if the use of ${CMAKE_PROJECT_NAME}
is correct?
test/CMakeLists.txt
Outdated
find_package(${CMAKE_PROJECT_NAME} REQUIRED) | ||
else() | ||
CPMAddPackage(NAME Greeter SOURCE_DIR ${CMAKE_CURRENT_LIST_DIR}/..) | ||
CPMAddPackage(NAME ${CMAKE_PROJECT_NAME} SOURCE_DIR ${CMAKE_CURRENT_LIST_DIR}/..) |
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.
Not sure why this would work, isn't ${CMAKE_PROJECT_NAME}
the same as the project name "GreeterTests"
in this context, as it's the top-level CMakeLists?
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.
IMHO you should only use ${PROJECT_NAME}
in the top level project.
Only then it would work as standalone project and as subproject used with CPM.cmake
in other CMake
projects.
The test project is a standalone project and an subproject. Being explizit would be better, or not?
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.
@ClausKlein If test project is designed to be a standalone project then yes it may make sense to remove ${CMAKE_PROJECT_NAME}.
@TheLartians
And regarding how it works this answer helped me to understand:
https://stackoverflow.com/questions/38938315/difference-between-cmake-project-name-and-project-name
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.
@DominicD yeah as I understand it, ${CMAKE_PROJECT_NAME}
should always refers to the most recent top-level project name. So it is context-dependent and we cannot know which value it holds. So if you run cmake -Stest -Bbuild
, then ${CMAKE_PROJECT_NAME}
should be GreeterTests
, but if you run cmake -Sall -Bbuild
, it should be BuildAll
etc.
I think this is also why the "Install" CI test is failing.
Currently I am not able to run cmake-format I will first have to figure out how that works. |
pip install cmakelang
cmake-format -i *.cmake ... |
The easiest way to format all the code (cmake /C++) should be: # configure tests (or "all" targets using `cmake -S all -B build`)
cmake -S test -B build/test
# run the `fix-format` target
cmake --build build/test --target fix-format Of course you should have clang-format and cmake-format installed as shown above. |
@DominicD is the PR still active? I would be happy to merge once CI checks pass. IMO the best approach would be to change |
@TheLartians I am very sorry but I had no time yet to do it. I hope I will find some time this week. |
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.
Super, thanks for the changes!
resolves #133