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

Do something with CMake's GLOB_RECURSE #88

Closed
bodand opened this issue Oct 2, 2019 · 4 comments · Fixed by #91
Closed

Do something with CMake's GLOB_RECURSE #88

bodand opened this issue Oct 2, 2019 · 4 comments · Fixed by #91

Comments

@bodand
Copy link
Contributor

bodand commented Oct 2, 2019

As stated in the CMake docs, it is unwise to use GLOB_RECURSE to find source files.
And even worse if used as here, without CONFIGURE_DEPENDS, which should fix the problem of calling make ending up not recompiling some changed files.

I suggest adding at least CONFIGURE_DEPENDS, but it'd be better to add each file separately and manually when added to the project.

The latter requires some community approval, therefore, I wasn't planning on creating a PR until it is discussed which route to take if one is even taken.

@tagniam
Copy link
Owner

tagniam commented Oct 2, 2019

Interesting. Is there any alternative to manually adding new files in the CMake config files that does not depend on GLOB_RECURSE? Seems unwieldy.

@bodand
Copy link
Contributor Author

bodand commented Oct 2, 2019

Sadly, not that I know of.

@BryanDiLaura
Copy link
Contributor

The official recommendation from the CMake people is to avoid GLOB when possible.

https://cmake.org/cmake/help/v3.15/command/file.html?highlight=glob#glob

The best practice seems to be using a list of sources, so CMake can tell when there are changes and it needs to rebuild the build system. I'd recommend moving to something along the lines of what lhiny's pull request does.

@bodand
Copy link
Contributor Author

bodand commented Oct 20, 2019

@lhiny's pull request is actually perfectly done, it even uses modern CMake paradigms like target_* macros; therefore, I say we should just merge #91, even if we just have to add the new enemies in separate PR-s.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants