Skip to content

Add install-lib and install-bin build targets #74

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 13, 2022
Merged

Add install-lib and install-bin build targets #74

merged 3 commits into from
Sep 13, 2022

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Sep 7, 2022 •
edited
Loading

This PR adds new install-lib and install-bin build targets which allow to build and install only required artifacts during different stages of cross-building.

A demo, which uses this branch in Bitcoin Core, is here.

@hebasto
Install proxy.capnp with mpgen target
bc3d166 
This changes is required for the following two commits.
@hebasto
Copy link
Member Author

hebasto commented Sep 7, 2022

Drafted until Concept (N)ACK.

@ryanofsky
Copy link
Collaborator

Few thoughts:

  • PR title "Make build system cross-building-friendly" is vague and seems to imply that current build system does not support cross-building, which is untrue. Would suggest "Add codegen-only and library-only CMake build options"

  • I think it would be better to add new build targets, not new build options. Ideally in addition to current make install target there would be new make install-lib and make install-bin targets that automatically build and install the minimal dependencies. Current approach confuses the role of CMake and Make. Cmake is great at determining how things should be built but Make does a much better job of determining what to build. If you go down the road of using CMake to decide what to build, it is a big mistake because instead of specifying dependencies explicitly you specify them implicitly in nest of IF() statements that don't scale well and can become unmaintainable: wastefully building more dependencies than the user actually requires, or failing to rebuild needed dependencies when different combination of options are used. It is much better to let Make figure out what to build automatically, than to try to do it manually in CMake with IF() statements.

    Instead of changing the build rules, I think it would would be better to take advantage of cmake install components and add_custom_target as described https://stackoverflow.com/questions/9190098/for-cmakes-install-command-what-can-the-component-argument-do

@hebasto hebasto changed the title Make build system cross-building-friendly Add codegen-only and library-only CMake build options Sep 9, 2022
@hebasto
Add install-lib target
c4d26d2 
When cross building, using `make install-lib` allows to avoid unneeded
building of `mpgen` for a target system.
@hebasto
Add install-bin target
fa130db 
When cross building, using `make install-bin` allows to avoid unneeded
building of `libmultiprocess` for a build system.
@hebasto
Copy link
Member Author

hebasto commented Sep 9, 2022 •
edited
Loading

Updated cd9389f -> fa130db (pr74.01 -> pr74.02, diff).

@ryanofsky

Thank you for your suggestions!

Ideally in addition to current make install target there would be new make install-lib and make install-bin targets that automatically build and install the minimal dependencies.

Implemented. The PR description has been updated.

The only downside of the current implementation is that new install-lib and install-bin targets do not export the underlying targets.

@hebasto hebasto changed the title Add codegen-only and library-only CMake build options Add install-lib and install-bin build targets Sep 9, 2022
@hebasto hebasto marked this pull request as ready for review September 9, 2022 08:34
ryanofsky
ryanofsky approved these changes Sep 13, 2022
View reviewed changes
Copy link
Collaborator

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK fa130db. This does exactly what I'd expect now, so will merge it. But you do bring up an important point about no longer exporting cmake targets.

The only downside of the current implementation is that new install-lib and install-bin targets do not export the underlying targets.

Can confirm neither of the new install commands create $PREFIX/cmake/Multiprocess/Multiprocess.cmake and $PREFIX/lib/cmake/Multiprocess/Multiprocess-debug.cmake files.

Probably should update install() EXPORT Multiprocess arguments to EXPORT libmultiprocess-bin and EXPORT libmultiprocess-lib so the two installs can be independent. And maybe there is a way to get the new install-bin and install-lib commands to actually export these targets.

But I am now thinking maybe a better long-term solution is to split up the cmake project into two projects. It could also clarify the source organization to split the parts of the code that are needed for code generation from the parts that are used at runtime.

@ryanofsky ryanofsky merged commit 4992b52 into bitcoin-core:master Sep 13, 2022
@hebasto hebasto deleted the 220908-separate branch September 14, 2022 13:04
@hebasto hebasto mentioned this pull request Sep 17, 2022
Merged
@hebasto hebasto mentioned this pull request Dec 9, 2022
Merged
fanquake added a commit to bitcoin-core/gui that referenced this pull request Dec 10, 2022
@fanquake
Merge bitcoin/bitcoin#26672: build: Update libmultiprocess library
e1fb738 
1986f12 build: Update `libmultiprocess` library (Hennadii Stepanov)

Pull request description:

  This update in particular includes:
  - bitcoin-core/libmultiprocess#78 which is [required](bitcoin/bitcoin#25972 (comment)) for bitcoin/bitcoin#25972
  - bitcoin-core/libmultiprocess#74
  - bitcoin-core/libmultiprocess#70

ACKs for top commit:
  ryanofsky:
    Code review ACK 1986f12

Tree-SHA512: 2d9fa72df5de7d5be37d77d479702cba36c45e9fa9d9fc27e58aac0437c39e4a1054c0ac53b612cb43e830982e444d98c7d3e651d093ac68344e66f4734227bb
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 10, 2022
@fanquake
Merge bitcoin#26672: build: Update libmultiprocess library
e603e70 
1986f12 build: Update `libmultiprocess` library (Hennadii Stepanov)

Pull request description:

  This update in particular includes:
  - bitcoin-core/libmultiprocess#78 which is [required](bitcoin#25972 (comment)) for bitcoin#25972
  - bitcoin-core/libmultiprocess#74
  - bitcoin-core/libmultiprocess#70

ACKs for top commit:
  ryanofsky:
    Code review ACK 1986f12

Tree-SHA512: 2d9fa72df5de7d5be37d77d479702cba36c45e9fa9d9fc27e58aac0437c39e4a1054c0ac53b612cb43e830982e444d98c7d3e651d093ac68344e66f4734227bb
ryanofsky added a commit that referenced this pull request Feb 14, 2023
@ryanofsky
Merge #79: Install Exports for custom install-{lib,bin} targets
917877a 
2dda753 Install Exports for custom `install-{lib,bin}` targets (Hennadii Stepanov)
54bd57f Use `GNUInstallDirs` module (Hennadii Stepanov)

Pull request description:

  This is a follow up of #74 and addresses #74 (review):
  > > The only downside of the current implementation is that new `install-lib` and `install-bin` targets do not export the underlying targets.
  >
  > Can confirm neither of the new install commands create `$PREFIX/cmake/Multiprocess/Multiprocess.cmake` and `$PREFIX/lib/cmake/Multiprocess/Multiprocess-debug.cmake` files.
  >
  > Probably should update `install()` `EXPORT Multiprocess` arguments to `EXPORT libmultiprocess-bin` and `EXPORT libmultiprocess-lib` so the two installs can be independent. And maybe there is a way to get the new `install-bin` and `install-lib` commands to actually export these targets.

ACKs for top commit:
  ryanofsky:
    Code review ACK 2dda753. Only change since last review is changing namespace prefix to match project name as suggested (thanks!)

Tree-SHA512: 4f41148e6a95f95790f9b4c3cf7694244f15f6100d2b860f182d6b14262ade320daf246bac1393419b04a0ef73b9da4291be70637038ed54b0484611a9aa8656
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 27, 2024
@fanquake @PastaPastaPasta
Merge bitcoin#26672: build: Update libmultiprocess library
571fd37 
1986f12 build: Update `libmultiprocess` library (Hennadii Stepanov)

Pull request description:

  This update in particular includes:
  - bitcoin-core/libmultiprocess#78 which is [required](bitcoin#25972 (comment)) for bitcoin#25972
  - bitcoin-core/libmultiprocess#74
  - bitcoin-core/libmultiprocess#70

ACKs for top commit:
  ryanofsky:
    Code review ACK 1986f12

Tree-SHA512: 2d9fa72df5de7d5be37d77d479702cba36c45e9fa9d9fc27e58aac0437c39e4a1054c0ac53b612cb43e830982e444d98c7d3e651d093ac68344e66f4734227bb
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 27, 2024
@fanquake @PastaPastaPasta
Merge bitcoin#26672: build: Update libmultiprocess library
04d0d67 
1986f12 build: Update `libmultiprocess` library (Hennadii Stepanov)

Pull request description:

  This update in particular includes:
  - bitcoin-core/libmultiprocess#78 which is [required](bitcoin#25972 (comment)) for bitcoin#25972
  - bitcoin-core/libmultiprocess#74
  - bitcoin-core/libmultiprocess#70

ACKs for top commit:
  ryanofsky:
    Code review ACK 1986f12

Tree-SHA512: 2d9fa72df5de7d5be37d77d479702cba36c45e9fa9d9fc27e58aac0437c39e4a1054c0ac53b612cb43e830982e444d98c7d3e651d093ac68344e66f4734227bb
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 27, 2024
@fanquake @PastaPastaPasta
Merge bitcoin#26672: build: Update libmultiprocess library
10acc2a 
1986f12 build: Update `libmultiprocess` library (Hennadii Stepanov)

Pull request description:

  This update in particular includes:
  - bitcoin-core/libmultiprocess#78 which is [required](bitcoin#25972 (comment)) for bitcoin#25972
  - bitcoin-core/libmultiprocess#74
  - bitcoin-core/libmultiprocess#70

ACKs for top commit:
  ryanofsky:
    Code review ACK 1986f12

Tree-SHA512: 2d9fa72df5de7d5be37d77d479702cba36c45e9fa9d9fc27e58aac0437c39e4a1054c0ac53b612cb43e830982e444d98c7d3e651d093ac68344e66f4734227bb
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 28, 2024
@fanquake @PastaPastaPasta
Merge bitcoin#26672: build: Update libmultiprocess library
a2cc616 
1986f12 build: Update `libmultiprocess` library (Hennadii Stepanov)

Pull request description:

  This update in particular includes:
  - bitcoin-core/libmultiprocess#78 which is [required](bitcoin#25972 (comment)) for bitcoin#25972
  - bitcoin-core/libmultiprocess#74
  - bitcoin-core/libmultiprocess#70

ACKs for top commit:
  ryanofsky:
    Code review ACK 1986f12

Tree-SHA512: 2d9fa72df5de7d5be37d77d479702cba36c45e9fa9d9fc27e58aac0437c39e4a1054c0ac53b612cb43e830982e444d98c7d3e651d093ac68344e66f4734227bb
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 1, 2024
@fanquake @PastaPastaPasta
Merge bitcoin#26672: build: Update libmultiprocess library
ad0c279 
1986f12 build: Update `libmultiprocess` library (Hennadii Stepanov)

Pull request description:

  This update in particular includes:
  - bitcoin-core/libmultiprocess#78 which is [required](bitcoin#25972 (comment)) for bitcoin#25972
  - bitcoin-core/libmultiprocess#74
  - bitcoin-core/libmultiprocess#70

ACKs for top commit:
  ryanofsky:
    Code review ACK 1986f12

Tree-SHA512: 2d9fa72df5de7d5be37d77d479702cba36c45e9fa9d9fc27e58aac0437c39e4a1054c0ac53b612cb43e830982e444d98c7d3e651d093ac68344e66f4734227bb
@ryanofsky ryanofsky mentioned this pull request Jan 31, 2025
Merged
@bitcoin-core bitcoin-core locked and limited conversation to collaborators Jun 25, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hebasto @ryanofsky