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, doc: Update build-unix.md #208

Merged
merged 1 commit into from
May 22, 2024
Merged

cmake, doc: Update build-unix.md #208

merged 1 commit into from
May 22, 2024

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented May 22, 2024

No description provided.

@hebasto hebasto added the documentation Docs and manuals label May 22, 2024
doc/build-unix.md Outdated Show resolved Hide resolved
Copy link

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

utACK 52400af

Checked consistency also against #201.

@hebasto
Copy link
Owner Author

hebasto commented May 22, 2024

@pablomartin4btc's #208 (comment) has been addressed.

@pablomartin4btc
Copy link

re-ACK a11cb27

doc/build-unix.md Show resolved Hide resolved
@@ -134,7 +133,7 @@ GUI dependencies:

If you want to build bitcoin-qt, make sure that the required packages for Qt development
are installed. Qt 5 is necessary to build the GUI.
To build without GUI pass `--without-gui`.
To build with GUI pass `-DBUILD_GUI=ON`.

To build with Qt 5 you need the following:

Choose a reason for hiding this comment

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

"Once these are installed, they will be found by configure and a bitcoin-qt executable will be
built by default."

This is no longer true?

Copy link
Owner Author

Choose a reason for hiding this comment

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

This is no longer true?

Please join the discussion in #196 :)

TL;DR:

During our calls, we discussed the concept of build options. We have reached a rough consensus to make them "opt-in" rather than "opt-out" as much as possible.

Choose a reason for hiding this comment

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

So this being the case, shouldn't we delete the line: "Once these are installed, they will be found by configure and a bitcoin-qt executable will be built by default."

Copy link
Owner Author

@hebasto hebasto May 22, 2024

Choose a reason for hiding this comment

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

Thanks! Deleted.

doc/build-unix.md Show resolved Hide resolved
@m3dwards
Copy link

Added some nits, two unrelated to the change to cmake.

@hebasto
Copy link
Owner Author

hebasto commented May 22, 2024

@m3dwards's #208 (comment) has been addressed.

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, just left some comments.

./configure
make # use "-j N" for N parallel jobs
make install # optional
cmake -B build

Choose a reason for hiding this comment

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

I think we talked previously about adding -S to the docs whenever we are configuring, but I can't remember anymore what the verdict was.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Indeed. There was -S in the past. See:

We agreed to use modern CMake invocation style. Configuring in the source root makes the -S option redundant.

doc/build-unix.md Show resolved Hide resolved
@hebasto hebasto merged commit da4fefa into cmake-staging May 22, 2024
2 of 34 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants