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

cmake: Switch from tri-state options to boolean. Stage FIVE. The GUI #173

Merged
merged 2 commits into from
May 8, 2024

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented Apr 27, 2024

Replaces multi-value WITH_GUI with boolean BUILD_GUI as discussed during the recent call.

The default value of the BUILD_GUI option is OFF.

Documentation has been adjusted.

While touching build docs, they were amended to use modern CMake invocations according to discussion in #167 (should be trivial to review though).

@hebasto hebasto changed the title cmake: Switch from tri-state options to boolean. Stage FOUR. GUI cmake: Switch from tri-state options to boolean. Stage FIVE. The GUI Apr 27, 2024
@hebasto hebasto added enhancement New feature or request documentation Docs and manuals labels Apr 27, 2024
@hebasto hebasto added this to the User Interface milestone Apr 27, 2024
@hebasto
Copy link
Owner Author

hebasto commented Apr 30, 2024

Rebased.

@hebasto hebasto force-pushed the 240427-cmake-EG branch 2 times, most recently from f60968f to ed3c58e Compare May 1, 2024 12:45
@hebasto
Copy link
Owner Author

hebasto commented May 1, 2024

Rebased to resolve conflicts.

@theuni
Copy link

theuni commented May 1, 2024

Would you mind splitting this up into 2 commits: 1 for the logical change, and 1 for the rename to BUILD_GUI? That would make it easier to review imo.

@hebasto
Copy link
Owner Author

hebasto commented May 1, 2024

@theuni

Would you mind splitting this up into 2 commits: 1 for the logical change, and 1 for the rename to BUILD_GUI? That would make it easier to review imo.

Done.

@hebasto
Copy link
Owner Author

hebasto commented May 4, 2024

Rebased.

if(NOT WITH_GUI AND "@no_qt@" STREQUAL "1")
set(WITH_GUI OFF CACHE STRING "Build GUI.")
# Set configuration options for the main build system.
if("@no_qt@")
Copy link

Choose a reason for hiding this comment

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

I wonder if we should depart from upstream/master behavior for all of these options to be more CMake-like?

(I'm not just referring to qt here, but all depends options)

What if depends doesn't turn anything on by default? Meaning.. quit forwarding any meaning from built packages into Core's build. So, you can build depends with any set of packages/options, but that doesn't turn anything just on by using the generated toolchain file.

That way, if I build depends with qt, I still have to specify WITH_GUI in order to get bitcoin-qt. Conversely, if I build depends with NO_QT and build with WITH_GUI, that would be an error.

Thoughts? @hebasto @fanquake @laanwj

Copy link
Owner Author

Choose a reason for hiding this comment

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

... to be more CMake-like?

In the future, CMake can manage all dependencies using its own modules like ExternalProject or FetchContent.

hebasto added a commit that referenced this pull request May 7, 2024
448132a fixup! cmake, doc: Update `build-freebsd.md` (Hennadii Stepanov)
9bafc55 fixup! cmake, doc: Update `build-osx.md` (Hennadii Stepanov)
a0a98ff fixup! cmake, docs: Update MSVC build docs (Hennadii Stepanov)

Pull request description:

  This PR amends build docs according the discussion in #167 where there were agreed to use modern cmake invocations.

  Simplifies #173 and #177.

ACKs for top commit:
  vasild:
    ACK 448132a

Tree-SHA512: cf49f258b6f65098603a0e33ec2a3f11665d83ee87efe3fc11ff59a2a92ba798c4e127896052cf934b0fcabbbfa6a87c880341458b0c12cfbc7b7a4465f9c413
@hebasto
Copy link
Owner Author

hebasto commented May 7, 2024

Rebased, which made the last commit simpler.

Copy link

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

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

lgtm

@hebasto hebasto merged commit a6efc04 into cmake-staging May 8, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Docs and manuals enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants