cmake: Install man pages for configured targets only

hebasto commented at 1:14 pm on January 30, 2025: member

Fixes #31762.

hebasto added the label Build system on Jan 30, 2025

hebasto added this to the milestone 29.0 on Jan 30, 2025

DrahtBot commented at 1:14 pm on January 30, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31765.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v
Concept ACK	BrandonOdiwuor

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31844 (cmake: add a component for each binary by theuni)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

hebasto renamed this:
~~cmake: Install man pages for built targets only~~
cmake: Install man pages for configured targets only
on Jan 30, 2025

hebasto force-pushed on Jan 30, 2025

in src/CMakeLists.txt:175 in 5f6680769f outdated

171@@ -172,6 +172,14 @@ target_link_libraries(bitcoin_common
172 
173 
174 set(installable_targets)
175+macro(install_manpage target)

stickies-v commented at 5:50 pm on January 31, 2025:

I think we should add an optional component parameter. This would address the issue (see below) of installing just GUI, and also aligns well with potentially adding more components (e.g. one per target) in the future (out of scope for this PR).

 0diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
 1index eb816dbacc..ea026c25be 100644
 2--- a/src/CMakeLists.txt
 3+++ b/src/CMakeLists.txt
 4@@ -172,10 +172,11 @@ target_link_libraries(bitcoin_common
 5 
 6 
 7 set(installable_targets)
 8-macro(install_manpage target)
 9+macro(install_manpage target component)
10   if(INSTALL_MAN)
11     install(FILES ${PROJECT_SOURCE_DIR}/doc/man/${target}.1
12       DESTINATION ${CMAKE_INSTALL_MANDIR}/man1
13+      COMPONENT ${component}
14     )
15   endif()
16 endmacro()
17diff --git a/src/qt/CMakeLists.txt b/src/qt/CMakeLists.txt
18index 35bfd1ecf1..0e887841af 100644
19--- a/src/qt/CMakeLists.txt
20+++ b/src/qt/CMakeLists.txt
21@@ -241,7 +241,7 @@ if(WIN32)
22   set_target_properties(bitcoin-qt PROPERTIES WIN32_EXECUTABLE TRUE)
23 endif()
24 
25-install_manpage(bitcoin-qt)
26+install_manpage(bitcoin-qt GUI)
27 
28 if(WITH_MULTIPROCESS)
29   add_executable(bitcoin-gui

On 5f6680769f017c9e12801dfa7af7d6568e53bdbe, when installing with --component GUI, the manpages are ignored:

0% cmake --install build --component GUI
1-- Install configuration: "RelWithDebInfo"
2-- Installing: /usr/local/bin/bitcoin-qt

With my diff:

0% cmake --install build --component GUI
1-- Install configuration: "RelWithDebInfo"
2-- Up-to-date: /usr/local/share/man/man1/bitcoin-qt.1
3-- Up-to-date: /usr/local/bin/bitcoin-qt

theuni commented at 6:42 pm on February 7, 2025:

Agreed. Mind addressing this @hebasto?

Edit: actually, I guess we need to actually set components for the other bins first. I’ll work on a PR for that.

stickies-v commented at 7:37 pm on February 7, 2025:

I guess we need to actually set components for the other bins first.

~~I don’t think we do? This patch picks up a component if defined, and otherwise just ignores it. So PR ordering shouldn’t matter?~~ edit: I can’t reproduce this, so I think I did something wrong when testing.

At least that’s what I got when testing on my machine

hebasto commented at 8:26 pm on February 7, 2025:

Thanks! Reworked.

stickies-v commented at 10:56 am on February 10, 2025:

Edit: actually, I guess we need to actually set components for the other bins first. I’ll work on a PR for that.

I can no longer reproduce the output I provided earlier, so I think I must have done something wrong while testing it. You’re right, it seems the components must be specified with this patch, so @hebasto’s new approach with ${ARGN} seems preferable. Sorry for the confusion!

stickies-v commented at 5:51 pm on January 31, 2025: contributor

Approach ACK 5f6680769f017c9e12801dfa7af7d6568e53bdbe, thanks for addressing my issue!

Unfamiliar enough with CMake to determine if this is the best approach, but it’s elegant and as per my testing it works well (except for my component comment).

BrandonOdiwuor commented at 8:06 pm on February 7, 2025: contributor

Concept ACK

cmake: Install man pages for configured targets only

Co-authored-by: stickies-v <stickies-v@protonmail.com>

c783833f7c

hebasto force-pushed on Feb 7, 2025

stickies-v approved

stickies-v commented at 11:40 am on February 10, 2025: contributor

tACK c783833f7c74bb184a74f3813b0bb973ea0b9e11

DrahtBot requested review from BrandonOdiwuor on Feb 10, 2025

theuni commented at 5:01 pm on February 11, 2025: member

Considering that in order to hook up COMPONENTS usage, we’d need to add an install() line for each as opposed to our current installable_targets approach, I’m not sure that install_manpage is the right abstraction.

Instead, I think we’re going to want an add_install() or so, right?

For ex:

Instead of:

0list(APPEND installable_targets bitcoind)
1...
2install(TARGETS ${installable_targets}
3  RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
4)

We’ll want:

0add_install(
1  TARGETS bitcoind
2  COMPONENT bitcoind
3  HAS_MANPAGE
4)

Then add_install() contains the if(INSTALL_MAN) logic (and sets RUNTIME DESTINATION)?

theuni commented at 8:19 pm on February 11, 2025: member

Considering that in order to hook up COMPONENTS usage, we’d need to add an install() line for each as opposed to our current installable_targets approach, I’m not sure that install_manpage is the right abstraction.

Instead, I think we’re going to want an add_install() or so, right?

See this branch for an alternative which adds install_binary_component as described above. Imo it’s more thorough/correct.

fanquake commented at 11:37 am on February 13, 2025: member

Close this given #31844 (which also solves #31745)? Or, it’d be good if you could comment either here or there on the alternative approach.

fanquake commented at 4:33 pm on February 13, 2025: member

Closing given the concept ACK in the other thread.

fanquake closed this on Feb 13, 2025

fanquake referenced this in commit 73e2ec1373 on Feb 14, 2025

cmake: Install man pages for configured targets only #31765

Code Coverage & Benchmarks

Reviews

Conflicts