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

doc: Remove Boost build note from build-unix.md #23469

Merged
merged 2 commits into from
Nov 9, 2021

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Nov 8, 2021

We don't have build instructions for any other dependency, and in any case, this isn't something we should maintain in our docs. If someone wants to know how to build Boost, or any other dependency, they can look at the relevant website / external build documentation.

We don't do this for any other dependency, and users are better looking
at the actual Boost site/docs. This isn't something we should need to
have in our build docs.
Calling dependencies optional in a list of already optional dependencies
is redundant.
@fanquake fanquake added the Docs label Nov 8, 2021
Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

Concept ACK

It makes sense to remove Boost build notes, as:

  1. As mentioned by @fanquake, build notes are not maintained for other libraries.
  2. Building Boost yourself is not an immediate necessity to compiling bitcoin.
  3. The official documentation of building boost is quite good and easy to follow through, so we don’t need to explain it on our docs.

However, removing Boost build notes intuitively kind of seems off to me. I would like to listen to other reviewers’ opinions before making a final decision.

As for removing unnecessary “optional,” I am ACK on that. Cheers!

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

ACK ea360d6

  • b971858
    • ACK because it's not something we need in our build notes
  • ea360d6
    • Yep, this is redundant given the table title

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 9, 2021

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #23470 (doc: consolidate legacy wallet documentation by fanquake)

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.

@instagibbs
Copy link
Member

@shaavan no informed opinion myself but I'd flip the question: would you ACK the addition of these docs?

@maflcko
Copy link
Member

maflcko commented Nov 9, 2021

I think our recommended way to use dependencies is system packages or depends. I wasn't aware there is even a doc on how to build boost separately. And I am still not entirely sure why we have the build_bdb script, given that there is depends.

cr ACK

@maflcko
Copy link
Member

maflcko commented Nov 9, 2021

It looks like the note on building boost was added before the depends system.

@maflcko maflcko merged commit 4914347 into bitcoin:master Nov 9, 2021
@fanquake fanquake deleted the remove_boost_note branch November 9, 2021 09:09
@katesalazar
Copy link
Contributor

So QR is now required, right?

@fanquake
Copy link
Member Author

fanquake commented Nov 9, 2021

So QR is now required, right?

No.

@katesalazar
Copy link
Contributor

ACK.

@katesalazar
Copy link
Contributor

Unless it's a recent thing, there's no need to bootstrap Boost as root.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 9, 2021
ea360d6 doc: remove redundant optionals from build-unix.md (fanquake)
b971858 doc: remove Boost build note from build-unix.md (fanquake)

Pull request description:

  We don't have build instructions for any other dependency, and in any case, this isn't something we should maintain in our docs. If someone wants to know how to build Boost, or any other dependency, they can look at the relevant website / external build documentation.

ACKs for top commit:
  jarolrod:
    ACK ea360d6

Tree-SHA512: c7389b5f051f79c728d8ea0725143affeb2c35b1e3c20d5cd441c6ac540d230698c47bf2c57feb12e7b6bffd2f337c1c83a9cd3a928ce5c479cdbbdf8f61fba4
@bitcoin bitcoin locked and limited conversation to collaborators Nov 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants