Skip to content
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

Merged
merged 4 commits into from
Oct 17, 2021
Merged

Conversation

DominicD
Copy link
Contributor

resolves #133

Copy link
Owner

@TheLartians TheLartians left a 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?

Comment on lines 22 to 24
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}/..)
Copy link
Owner

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?

Copy link
Contributor

@ClausKlein ClausKlein Sep 29, 2021

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?

Copy link
Contributor Author

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

Copy link
Owner

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.

@DominicD
Copy link
Contributor Author

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?

Currently I am not able to run cmake-format I will first have to figure out how that works.

@ClausKlein
Copy link
Contributor

ClausKlein commented Oct 1, 2021

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?

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 ...

@TheLartians
Copy link
Owner

TheLartians commented Oct 2, 2021

Currently I am not able to run cmake-format I will first have to figure out how that works.

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.

@TheLartians
Copy link
Owner

@DominicD is the PR still active? I would be happy to merge once CI checks pass. IMO the best approach would be to change CMAKE_PROJECT_NAME back to Greeter and run the formatting target as mentioned above.

@DominicD
Copy link
Contributor Author

@DominicD is the PR still active? I would be happy to merge once CI checks pass. IMO the best approach would be to change CMAKE_PROJECT_NAME back to Greeter and run the formatting target as mentioned above.

@TheLartians I am very sorry but I had no time yet to do it. I hope I will find some time this week.

Copy link
Owner

@TheLartians TheLartians left a 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!

test/CMakeLists.txt Outdated Show resolved Hide resolved
@TheLartians TheLartians enabled auto-merge (squash) October 17, 2021 19:10
@TheLartians TheLartians merged commit e0bf3f5 into TheLartians:master Oct 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use ${PROJECT_NAME} instead of writing projectname multiple times
4 participants