doc: rewrite OpenBSD build docs for 7.0 #24652

pull fanquake wants to merge 1 commits into bitcoin:master from fanquake:openbsd_wallet_doc_cleanupp changing 1 files +71 −61

fanquake commented at 3:49 PM on March 23, 2022: member

Removes redundant notes for setting CC &CXX now that Clang is well and truly the base compiler. See: https://www.openbsd.org/70.html

Disabled base-gcc on amd64.

Cleans up the wallet docs, i.e #23446.

Make the notes more similar to the FreeBSD notes.
fanquake added the label Docs on Mar 23, 2022

in doc/build-openbsd.md:74 in 33f946b376 outdated

  98 |  ```bash
  99 |  
 100 |  # Replace this with the autoconf version that you installed. Include only
 101 | -# the major and minor parts of the version: use "2.69" for "autoconf-2.69p2".
 102 | -export AUTOCONF_VERSION=2.69
 103 | +# the major and minor parts of the version: use "2.69" for "autoconf-2.71".

theStack commented at 5:03 PM on March 23, 2022:

should also replace the string "2.69" with "2.71" in the comment (don't know why GitHub doesn't let me add a diff-like suggestions here)

fanquake commented at 8:50 PM on March 23, 2022:

Thanks, addressed.

theStack commented at 5:08 PM on March 23, 2022: member

Concept ACK
jessebarton approved
jessebarton commented at 7:25 PM on March 23, 2022: contributor

Concept ACK 33f946b

I like the idea of keeping consistency in the markdown across build docs.
fanquake force-pushed on Mar 23, 2022
fanquake commented at 2:18 PM on March 24, 2022: member

@shaavan you might be interested in reviewing?
in doc/build-openbsd.md:73 in d7fd6cc80a outdated
```
  95 |  **Important**: Use `gmake` (the non-GNU `make` will exit with an error).
  96 |  
  97 |  Preparation:
  98 |  ```bash
  99 |  
 100 |  # Replace this with the autoconf version that you installed. Include only
```
shaavan commented at 8:00 AM on March 25, 2022:

“Replace this with autoconf version you installed.” Though it need not specifically be part of the documentation, I think we can give a redirect to a webpage explaining how to know the currently installed version of a package in OpenBSD.
fanquake commented at 8:07 AM on March 25, 2022:

I think that's something that someone can either look up, or read the OpenBSD documentation for; assuming they've installed the package previously, otherwise if they've just installed the package while running through these docs, they already know the version.

shaavan commented at 7:37 AM on March 26, 2022:

I think that's a fair point. So I think we can forgo my 2nd suggestion. @fanquake I want to understand your take on my first suggestion about removing example version numbers.

fanquake commented at 12:50 PM on March 28, 2022:

removing example version numbers. @shaavan I've pushed a change that should resolve that.
shaavan commented at 8:01 AM on March 25, 2022: contributor
Concept ACK

I like this rewriting of the OpenBSD documentation for the following reasons:
1. It effectively uses the # tags to properly segregate subsections into logical units.
2. Updates the example package versions to the latest one.
3. Removes redundant information from wherever possible.
4. Properly structure the documentation into a logical flow, from preparations to building specific wallet support.
I want to suggest some minor changes that might help improve this documentation.
1. I think we should not be giving version numbers in examples as this might incur a maintenance burden on the documentation as time passes.

doc: update OpenBSD build docs for 7.0

Removes redundant notes for setting CC & CXX now that Clang is well and
truly the base compiler.
Cleans up the wallet docs, i.e #23446.
Make the notes more similar to FreeBSD.

a2b56dcd1f

fanquake force-pushed on Mar 28, 2022
laanwj commented at 11:25 AM on March 30, 2022: member
Concept ACK.

Removes redundant notes for setting CC &CXX now that Clang is well and truly the base compiler.

Nice. I had in mind to do this as well for a while.

I think we should not be giving version numbers in examples as this might incur a maintenance burden on the documentation as time passes.

I agree in general. However not sure which version you're referring to exactly;
- mentioning the OpenBSD version is pretty important; maybe not so much going forward, but there had to be significant changes for each release. So mentioning it gives some assurance that it's up to date, and the maintenance burden is there anyway.
- automake/autoconf rarely have releases. It's some extra burden to track them, but on the other hand it's a lot easier to be able to copy/paste instead of having to check. Also, mentioning versions that work could be useful, there have been breaking changes to them for the BSDs in the past.
in doc/build-openbsd.md:37 in a2b56dcd1f
```
  52 | +###### Descriptor Wallet Support
  53 | +
  54 | +`sqlite3` is required to support [descriptor wallets](descriptors.md).
  55 | +
  56 | +``` bash
  57 | +pkg_add install sqlite3
```
laanwj commented at 11:30 AM on March 30, 2022:

Having all the pkg_add under one section (split up into subsections for different needs) might be slightly more handy. Dunno. I usually like to do it at once (as root) when following the instructions.

fanquake commented at 8:37 AM on April 5, 2022:

I agree that having everything in a single line to copy-paste and install might be more convenient. I'm just following the existing docs, which seem to have everything split out into separate sections.
fanquake commented at 8:38 AM on April 5, 2022: member

@shaavan want to take another look?
shaavan approved
shaavan commented at 12:50 PM on April 5, 2022: contributor
ACK a2b56dcd1fa102dcb3023cd2af63075525789453

Changes since my last review:
1. Removed the example version number from the pkg_add command and made subsequent minor changes.
2. Reworded commands and comments under the Building Bitcoin core subheading.
Removing the example version made the code snippet relatively clean and concise while still making the user aware of installing the latest version.

I agree with the new rewording of the Building Bitcoin core code snippet section for the following reasons:
1. It made the code snippet relatively brief, making it clear to understand.
2. Though “major.minor only” seemed quite cryptic at first, the following code lines clarified what it meant, removing the need for redundant example comments.
fanquake requested review from theStack on Apr 5, 2022

in doc/build-openbsd.md:14 in a2b56dcd1f

  22 | -pkg_add autoconf # (select highest version, e.g. 2.69)
  23 | -pkg_add automake # (select highest version, e.g. 1.16)
  24 | -pkg_add python # (select highest version, e.g. 3.8)
  25 | -pkg_add bash
  26 | +pkg_add bash git gmake libevent libtool boost
  27 | +# Select the newest version of the follow packages:

theStack commented at 6:20 PM on April 5, 2022:

nit: s/follow/following/

fanquake commented at 9:27 AM on April 6, 2022:

Thanks. I'll take care of this.

fanquake commented at 11:59 AM on April 7, 2022:

Done in #24796.

theStack approved
theStack commented at 6:27 PM on April 5, 2022: member

LGTM

ACK a2b56dcd1fa102dcb3023cd2af63075525789453
fanquake merged this on Apr 6, 2022
fanquake closed this on Apr 6, 2022
fanquake deleted the branch on Apr 6, 2022
sidhujag referenced this in commit 1b32daa442 on Apr 6, 2022
DrahtBot locked this on Apr 7, 2023

Contributors

Labels

Linked (view graph)

#23446 doc: Mention that BerkeleyDB is for legacy wallet in build-unix