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

docs: Add info about factors that affect dependency list #15222

Merged
merged 1 commit into from Feb 21, 2019

Conversation

Projects
None yet
8 participants
@merland
Copy link
Contributor

commented Jan 21, 2019

To simplify build instructions, the librsvg formula should be moved to the main brew install ... command, in my opinion.
It is not a big problem to install a single extra formula, and it will only be unused for some users.

An additional reason for this change is that I would like to add a comment (in a future PR) about making sure you have the latest version of all deps (in the case of preexisting formulae). That comment can be authored more clearly if this simplification PR is merged.

@merland merland changed the title Simplification of macOS build instructions docs: Simplification of macOS build instructions Jan 21, 2019

@matt-auckland

This comment has been minimized.

Copy link

commented Jan 21, 2019

ACK - looks good to me

@hebasto

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

This PR reduces clarity of the build instructions, IMO.
Slightly inclined to NACK.

@merland

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2019

@hebasto Granted, some information is lost, but I felt it was not crucial to tell the user for what reason librsvg is needed. Do you think it is important? If so, why?

@fanquake fanquake added the Docs label Jan 21, 2019

@hebasto

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

@merland

Do you think it is important?

Yes.

If so, why?

The task of any docs is to provide a user with clear and relevant info.

@merland

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2019

@hebasto Then maybe the instructions should state exactly why every dependency is needed? :)
I think good instructions should tell you all you need to know, but nothing more. There is a link to dependencies.md for the more curious reader.

@Sjors

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

I'm not a fan of removing this information. Currently https://github.com/bitcoin/bitcoin/blob/master/doc/dependencies.md points back to the OS specific build instructions for more information. macOS instructions are far less detailed than Linux at the moment, and I've often just looked there.

Maybe we can add a section where we briefly mention which dependencies can be avoided or added:

  • librsvg can be avoided if you don't need make deploy
  • miniupnpc can be avoided if you use ./configure --with-miniupnpc=no
  • berkeley-db4 can be avoided if you compile without wallet --disable-wallet, or you just use berkeley-db if you compile with --with-incompatible-bdb
  • protobuf can be avoided with --disable-bip70
  • qt with --disable-gui
  • when qrencode is absent, it won't add QR support, to explicitly complain in such a case: --with-qrencode
  • missing from the instructions: brew install zeromq is needed for --with-zmq

(not sure about the others)

I'm fine with including librsvg in the default instruction, since it's non-trivial to install the GUI without that, and I'm assuming the main target audience for these instructions are users, not developers.

@merland

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2019

@Sjors
This makes a lot of sense, thank you! Hope it's ok if I update this PR with your suggestions, adding you as co-author.

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

This PR reduces clarity of the build instructions, IMO.
Slightly inclined to NACK.

Agree, it slightly reduces the information for no good reason IMO.

@merland merland force-pushed the merland:patch-2 branch 3 times, most recently Jan 22, 2019

@merland

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

Updated. I added the info provided by @Sjors as a new column in doc/dependencies.md.
What do you think?

@merland merland force-pushed the merland:patch-2 branch Jan 22, 2019

doc/dependencies.md Outdated
| zlib | [1.2.11](https://zlib.net/) | | | | No |
| Dependency | Version used | Minimum required | CVEs | Shared | [Bundled Qt library](https://doc.qt.io/qt-5/configure-options.html#third-party-libraries) |Notes|
| --- | --- | --- | --- | --- | --- | --- |
| Berkeley DB | [4.8.30](https://www.oracle.com/technetwork/database/database-technologies/berkeleydb/downloads/index.html) | 4.8.x | No | | |Not needed if you compile with `--disable-wallet`, or `--with-incompatible-bdb`|

This comment has been minimized.

Copy link
@hebasto

hebasto Jan 22, 2019

Member

Building with --with-incompatible-bdb option does not make Berkeley DB dependency unneeded.

This comment has been minimized.

Copy link
@merland

merland Jan 22, 2019

Author Contributor

Ok, thanks @hebasto. Did I misunderstand this bullet point, @Sjors ?
What is the best phrasing for this cell?

This comment has been minimized.

Copy link
@hebasto

hebasto Jan 22, 2019

Member

@merland

From Ubuntu & Debian Dependency Build Instructions:

Ubuntu and Debian have their own libdb-dev and libdb++-dev packages, but these will install BerkeleyDB 5.1 or later. This will break binary wallet compatibility with the distributed executables, which are based on BerkeleyDB 4.8. If you do not care about wallet compatibility, pass --with-incompatible-bdb to configure.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14066 (gitian-linux: Build binaries for 64-bit POWER by luke-jr)

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.

@Sjors

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

I don't think adding the extra column to the table in dependencies is the right approach.

Try adding a new section at the bottom instead "Configuration options". There you can explain the effect of each --disable option on which dependencies you need.

Regarding Berkeley DB: it's used by the wallet, so you don't need it at all if you compile with --disable-wallet. In addition, which often confuses people, the wallet is super picky about which BDB version it wants, which is why we still use 4.8. If you don't mind a potentially incompatible wallet file, then you can use newer versions too. That's when you need --with-incompatible-bdb. Again, too subtle to squeeze into a column.

P.S. I didn't know Github integrated so well with the Co-authored-by tag (no need by the way, though it's always nice).

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

Still tend toward NACK.I think it's better to keep this information where it is and can be easily found.

@merland

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

Thanks for weighing in, @laanwj. I see your point.
However, having a history as a technical writer (but also developer for many years), I tend to think mainly in terms of readability and simplicity for the majority of the readers.

As @Sjors writes above, the main target audience for these instructions is probably users, not developers. As a result, I think it makes a lot of sense to think along the lines of "convention over configuration" and hide complexities wherever possible. Taking away non-crucial info can greatly improve docs like these, sometimes.

Having said this, I admit that this PR (that started as a small simplification) may be growing over my head. I may not see the full implications of centralizing dependency info for several platforms into one place. Nonetheless, @Sjors' suggestion to extend dependencies.md sure seemed like an elegant and de-duplicating doc refactoring.

@merland merland force-pushed the merland:patch-2 branch Jan 23, 2019

@merland merland changed the title docs: Simplification of macOS build instructions docs: Add info about factors that affect dependency list Jan 23, 2019

@merland

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

Since I got some pushback against the change to simplicification of the build instructions, this PR has now "pivoted" into being about the added dependency info only.

@Sjors, I intentionally omitted the info about --with-incompatible-bdb, since I think it may confuse more than it helps the majority of the readers. Also, it is documented elsewhere.

@Sjors
Copy link
Member

left a comment

Ok, so now it's strictly adding information, which should not be controversial.

Travis ran into a linter issue (some trailing whitespace, most code editors have an option to prevent that).

Show resolved Hide resolved doc/dependencies.md Outdated
Show resolved Hide resolved doc/dependencies.md Outdated

@merland merland force-pushed the merland:patch-2 branch Jan 23, 2019

@Sjors

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

ACK a59529e

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

utACK a59529ed2c579d015e7867eb23ba353b7a616bec
looks good to me now

Show resolved Hide resolved doc/dependencies.md Outdated
Show resolved Hide resolved doc/dependencies.md Outdated
Added some factors that affect the dependency list
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>

@merland merland force-pushed the merland:patch-2 branch to 55e05a8 Jan 23, 2019

@merland

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

Updated after comments from @flack.
If anyone knows if a certain version of librsvg is reqiured, please comment.

@Sjors

Sjors approved these changes Jan 23, 2019

Copy link
Member

left a comment

re-ACK 55e05a8

@@ -17,6 +17,7 @@ These are the dependencies currently used by Bitcoin Core. You can find instruct
| libevent | [2.1.8-stable](https://github.com/libevent/libevent/releases) | 2.0.22 | No | | |
| libjpeg | | | | | [Yes](https://github.com/bitcoin/bitcoin/blob/master/depends/packages/qt.mk#L65) |
| libpng | | | | | [Yes](https://github.com/bitcoin/bitcoin/blob/master/depends/packages/qt.mk#L64) |
| libsrvg | | | | | |

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 23, 2019

Member

There's a bunch of CVE's for librsvg, though afaik they all involve a specially crafted SVG file. Since we don't let users open arbitrary images, I doubt they matter. So that also means we don't have to recommend a minimum version.

@laanwj thoughts on what to put in the CVE column in this case (current PR leaves it blank)? Either way I think that can wait for another PR.

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 23, 2019

Member

Out of an abundance of caution, I would just set the minimum to 2.41.2 which fixes the most recent CVE. It's almost a year old, which is ancient for most macOS users :-)

However it seems our macOS Gitian build would then be in violation of that, since Bionic is still at 2.40, and they don't even list this CVE in their tracker.

This comment has been minimized.

Copy link
@laanwj

laanwj Jan 24, 2019

Member

Right—if it's linked into bitcoin-qt itself—and not a side dependency used for tooling only—these kind of indirect vulnerabilities could still be a problem. In many cases exploitation involves multiple stages, where one exploit is able to insert something into memory which another bug will stumble over, eventually resulting in RCE. So in that case it should be mentioned.

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

This is pretty useful info now, thanks!
utACK 55e05a8

@laanwj laanwj merged commit 55e05a8 into bitcoin:master Feb 21, 2019

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

laanwj added a commit that referenced this pull request Feb 21, 2019

Merge #15222: docs: Add info about factors that affect dependency list
55e05a8 Added some factors that affect the dependency list (Martin Erlandsson)

Pull request description:

  To simplify build instructions, the librsvg formula should be moved to the main `brew install ...` command, in my opinion.
  It is not a big problem to install a single extra formula, and it will only be unused for some users.

  An additional reason for this change is that I would like to add a comment (in a future PR) about making sure you have the latest version of all deps (in the case of preexisting formulae). That comment can be authored more clearly if this simplification PR is merged.

Tree-SHA512: e63284a4e0584f071a920f6b8ac46694de38e7b1df1e0dc2b00262c1487a2f2851fae721e8f4907a4aad0335f287e881974df6f9d05fe9b26f0ba71033dce145

@merland merland deleted the merland:patch-2 branch Apr 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.