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 not export install commands if termcolor is a subproject. #67

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cpp977
Copy link

@cpp977 cpp977 commented Jan 3, 2022

If termcolor is used as a subproject (e.g. via cpm package manager or via fetch_content()) the install calls should be hidden. This can be achieved by guarding the calls with:

if(NOT TERMCOLOR_SUBPROJECT)
  install(...)
endif()

The variable TERMCOLOR_SUBPROJECT can be determined at the beginning of the CMakeLists.txt by checking if another project() call is present.

…id of install calls when termcolor is used as a subproject.
@ikalnytskyi
Copy link
Owner

Hey @cpp977,

Thanks for the patch. My cmake-fu is quite rusty, so please help me to understand this a bit better.

  1. What implications do we have if these installs are not hidden?
  2. I wonder, is there any chance to test this as part of https://github.com/ikalnytskyi/termcolor/blob/master/.github/workflows/cmake.yml flow?

@cpp977
Copy link
Author

cpp977 commented Feb 14, 2022

Hey,

of course! I will explain how I came to the problem. Could be that they is a better solution.

I came to the problem in a project, which exports(installs) only a binary (https://github.com/cpp977/Instantiator). In this project, I include termcolor via the CPM package manager, which is very similar as using the cmake built-in fetch_content. In this approach, the cmake file is basically appended to the toplevel cmake file of the project, so that all of your targets are directly available.
However, this is also the case for the install targets and not only the build targets. So when I run make install for my project, all of the subproject's install commands will also be executed and termcolor will be installed. This is not intended when shipping binaries and in general the install commands should be managed by the toplevel cmake file. Another solution for this would be to provide a cmake variable which could be set to disable the install commands.

To the 2. question:
I guess this can be tested in the cmake-fetch. One needs to add an install target for example.cpp and then verify, that termcolor is not installed.
I could try to add it.

@ikalnytskyi
Copy link
Owner

I just removed the whole install block and it seems make install still installs the targets. 🤔

❯ make DESTDIR=/tmp/fakeroot install                                                                                                                                                                                                 
[ 50%] Building CXX object CMakeFiles/example.dir/example.cpp.o
[100%] Linking CXX executable example
[100%] Built target example
Install the project...
-- Install configuration: ""
-- Installing: /tmp/fakeroot/usr/local/lib/cmake/termcolor/termcolor-config.cmake
-- Installing: /tmp/fakeroot/usr/local/include
-- Installing: /tmp/fakeroot/usr/local/include/termcolor
-- Installing: /tmp/fakeroot/usr/local/include/termcolor/termcolor.hpp
-- Installing: /tmp/fakeroot/usr/local/lib/cmake/termcolor/termcolor-targets.cmake

So I'm not confident enough that suggested solution solves the issue.

@cpp977
Copy link
Author

cpp977 commented Oct 3, 2022

I am not sure what you did exactly but if the install target is removed, cmake shouldn't install anysthing.
Here is a blog post about projects which can be easily processed by fetch_content():
https://www.foonathan.net/2022/06/cmake-fetchcontent/#more
I changed the way of determining if termcolor is a subproject as suggested in the post.
The above solution should work: if termcolor is included via fetch_content, no install targets are defined.

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.

None yet

2 participants