cmake: Install man pages for configured targets only #31765

hebasto · 2025-01-30T13:14:08Z

DrahtBot · 2025-01-30T13:14:12Z

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.

stickies-v

Approach ACK 5f66807, 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).

stickies-v · 2025-01-31T17:50:18Z

src/CMakeLists.txt



 set(installable_targets)
+macro(install_manpage target)


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

git diff on 5f66807

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index eb816dbacc..ea026c25be 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -172,10 +172,11 @@ target_link_libraries(bitcoin_common set(installable_targets) -macro(install_manpage target) +macro(install_manpage target component) if(INSTALL_MAN) install(FILES ${PROJECT_SOURCE_DIR}/doc/man/${target}.1 DESTINATION ${CMAKE_INSTALL_MANDIR}/man1 + COMPONENT ${component} ) endif() endmacro() diff --git a/src/qt/CMakeLists.txt b/src/qt/CMakeLists.txt index 35bfd1ecf1..0e887841af 100644 --- a/src/qt/CMakeLists.txt +++ b/src/qt/CMakeLists.txt @@ -241,7 +241,7 @@ if(WIN32) set_target_properties(bitcoin-qt PROPERTIES WIN32_EXECUTABLE TRUE) endif() -install_manpage(bitcoin-qt) +install_manpage(bitcoin-qt GUI) if(WITH_MULTIPROCESS) add_executable(bitcoin-gui

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

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

With my diff:

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

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.

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

Thanks! Reworked.

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!

BrandonOdiwuor

Concept ACK

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

stickies-v

tACK c783833

theuni · 2025-02-11T17:01:58Z

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:

list(APPEND installable_targets bitcoind)
...
install(TARGETS ${installable_targets}
  RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
)

We'll want:

add_install(
  TARGETS bitcoind
  COMPONENT bitcoind
  HAS_MANPAGE
)

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

theuni · 2025-02-11T20:19:31Z

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 · 2025-02-13T11:37:39Z

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 · 2025-02-13T16:33:57Z

Closing given the concept ACK in the other thread.

9b033be cmake: rename Kernel component to bitcoinkernel for consistency (Cory Fields) 2e0c925 cmake: add and use install_binary_component (Cory Fields) 0264c5d cmake: use per-target components for bitcoin-qt and bitcoin-gui (Cory Fields) fb0546b ci: don't try to install for a fuzz build (Cory Fields) Pull request description: This makes it possible to build/install only the desired binaries regardless of the configuration. For consistency, the component names match the binary names. `Kernel` and `GUI` have been renamed. Additionally it fixes #31762 by installing only the manpages for the configured targets (and includes them in the component installs for each). Also fixes #31745. Alternative to #31765 which is (imo) more correct/thorough. Can be tested using (for ex): ```bash $ cmake -B build $ cmake --build build -t bitcoind -t bitcoin-cli $ cmake --install build --component bitcoind $ cmake --install build --component bitcoin-cli ``` ACKs for top commit: hebasto: ACK 9b033be. TheCharlatan: Re-ACK 9b033be stickies-v: re-ACK 9b033be Tree-SHA512: fd4818e76f190dbeafbf0c246b466f829771902c9d6d7111ed917093b811c8a5536a4a45e20708f73e7f581d6cb77c8e61cfa69e065788dcf0886792f553a355

hebasto added the Build system label Jan 30, 2025

hebasto added this to the 29.0 milestone Jan 30, 2025

hebasto changed the title ~~cmake: Install man pages for built targets only~~ cmake: Install man pages for configured targets only Jan 30, 2025

hebasto force-pushed the 250130-man-inst branch from cd28d5e to 5f66807 Compare January 30, 2025 13:34

This was referenced Jan 30, 2025

multiprocess: Add libmultiprocess git subtree #31741

Merged

cmake: Install internal binaries to <prefix>/libexec/ #31679

Merged

ci: build multiprocess on most jobs #30975

Closed

stickies-v reviewed Jan 31, 2025

View reviewed changes

DrahtBot mentioned this pull request Feb 5, 2025

Add bitcoin-{node,gui} to release binaries for IPC #31802

Merged

BrandonOdiwuor reviewed Feb 7, 2025

View reviewed changes

cmake: Install man pages for configured targets only

c783833

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

hebasto force-pushed the 250130-man-inst branch from 5f66807 to c783833 Compare February 7, 2025 20:25

stickies-v approved these changes Feb 10, 2025

View reviewed changes

DrahtBot requested a review from BrandonOdiwuor February 10, 2025 11:40

theuni mentioned this pull request Feb 11, 2025

cmake: add a component for each binary #31844

Merged

fanquake closed this Feb 13, 2025

cmake: Install man pages for configured targets only #31765

cmake: Install man pages for configured targets only #31765

Uh oh!

Conversation

hebasto commented Jan 30, 2025

Uh oh!

DrahtBot commented Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

stickies-v Jan 31, 2025

Choose a reason for hiding this comment

Uh oh!

theuni Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stickies-v Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Feb 7, 2025

Choose a reason for hiding this comment

Uh oh!

stickies-v Feb 10, 2025

Choose a reason for hiding this comment

Uh oh!

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

theuni commented Feb 11, 2025

Uh oh!

theuni commented Feb 11, 2025

Uh oh!

fanquake commented Feb 13, 2025

Uh oh!

fanquake commented Feb 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DrahtBot commented Jan 30, 2025 •

edited

Loading

theuni Feb 7, 2025 •

edited

Loading

stickies-v Feb 7, 2025 •

edited

Loading