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.
ACK - looks good to me
This PR reduces clarity of the build instructions, IMO. Slightly inclined to NACK.
@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.
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 deployminiupnpc can be avoided if you use ./configure --with-miniupnpc=noberkeley-db4 can be avoided if you compile without wallet --disable-wallet, or you just use berkeley-db if you compile with --with-incompatible-bdbprotobuf can be avoided with --disable-bip70qt with --disable-guiqrencode is absent, it won't add QR support, to explicitly complain in such a case: --with-qrencodebrew 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.
This PR reduces clarity of the build instructions, IMO. Slightly inclined to NACK.
Agree, it slightly reduces the information for no good reason IMO.
28 | -| xkbcommon | | | | | [Yes](https://github.com/bitcoin/bitcoin/blob/master/depends/packages/qt.mk#L86) (Linux only) | 29 | -| ZeroMQ | [4.3.1](https://github.com/zeromq/libzmq/releases) | 4.0.0 | No | | | 30 | -| zlib | [1.2.11](https://zlib.net/) | | | | No | 31 | +| Dependency | Version used | Minimum required | CVEs | Shared | [Bundled Qt library](https://doc.qt.io/qt-5/configure-options.html#third-party-libraries) |Notes| 32 | +| --- | --- | --- | --- | --- | --- | --- | 33 | +| 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`|
Building with --with-incompatible-bdb option does not make Berkeley DB dependency unneeded.
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-bdbto configure.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
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).
Still tend toward NACK.I think it's better to keep this information where it is and can be easily found.
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.
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.
37 | +* **miniupnpc** is not needed with `--with-miniupnpc=no`. 38 | +* **berkeley-db4** is not needed with `--disable-wallet`. 39 | +* **protobuf** is not needed with `--disable-bip70`. 40 | +* **qt** is not needed with `--without-gui`. 41 | +* If the **qrencode** dependency is absent, QR support won't be added. To force an error when that happens, pass `--with-qrencode`. 42 | +* **zeromq** is needed only with the `--with-zmq` option.
Trailing whitespace
fixed, thanks.
40 | +* **qt** is not needed with `--without-gui`. 41 | +* If the **qrencode** dependency is absent, QR support won't be added. To force an error when that happens, pass `--with-qrencode`. 42 | +* **zeromq** is needed only with the `--with-zmq` option. 43 | + 44 | +#### Other 45 | +* **librsvg** is only needed if you need to run `make deploy`.
Nit, add: "on (cross-compliation to) macOS."
good.
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).
ACK a59529e
utACK a59529ed2c579d015e7867eb23ba353b7a616bec looks good to me now
32 | +Controlling dependencies 33 | +------------------------ 34 | +Some dependencies are not needed in all configurations. The following are some factors that affect the dependency list. 35 | + 36 | +#### Options passed to `./configure` 37 | +* **miniupnpc** is not needed with `--with-miniupnpc=no`.
Nit: would be nice if the dependency names used the same capitalization as in the table above (MiniUPnPc, ZeroMQ, Berkeley DB, Qt).
Agreed! Will fix.
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
40 | +* **qt** is not needed with `--without-gui`. 41 | +* If the **qrencode** dependency is absent, QR support won't be added. To force an error when that happens, pass `--with-qrencode`. 42 | +* **zeromq** is needed only with the `--with-zmq` option. 43 | + 44 | +#### Other 45 | +* **librsvg** is only needed if you need to run `make deploy` on (cross-compliation to) macOS.
librsvg is not listed in the table. Should it be added?
Good catch. It was added in an earlier commit, but hence forgotten... Will update.
16 | @@ -17,6 +17,7 @@ These are the dependencies currently used by Bitcoin Core. You can find instruct 17 | | libevent | [2.1.8-stable](https://github.com/libevent/libevent/releases) | 2.0.22 | No | | | 18 | | libjpeg | | | | | [Yes](https://github.com/bitcoin/bitcoin/blob/master/depends/packages/qt.mk#L65) | 19 | | libpng | | | | | [Yes](https://github.com/bitcoin/bitcoin/blob/master/depends/packages/qt.mk#L64) | 20 | +| libsrvg | | | | | |
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.
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.
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.
re-ACK 55e05a8
This is pretty useful info now, thanks! utACK 55e05a82cdc347400e4e6f8df2b0aba690b835ea