Skip to content

Conversation

theStack
Copy link
Contributor

Compiling C++ code with -D_XOPEN_SOURCE=600 causes problems on OpenBSD. If that define is set, the C++ standard header detection routine in BDB's configure script fails due to a missing type name for locale_t (see https://gist.github.com/theStack/b41884e31ebc5cdca3220bcaa674cb70 for the relevant config.log part).

This results in HAVE_CXX_STDHEADERS not being defined, which then it turn leads to the inclusion of <iostream.h> (rather than <iostream>), which doesn't exist, as described in #28963.

According to a mailing list post discussing a similar problem [1], "OpenBSD provides the POSIX APIs by default", so we don't need this define anyway and can remove it. This fixes the BDB build problem as described in issue #28963. See also google/flatbuffers@f87e75a for a similar fix for google's flatbuffer project.

Tested on OpenBSD 7.4 with clang 13.0.0. Fixes #28963.

[1] https://www.mail-archive.com/tech@openbsd.org/msg63386.html

Compiling C++ code with `-D_XOPEN_SOURCE=600` causes problems on
OpenBSD. If that define is set, the C++ standard header detection
routine in BDB's configure script fails. This results in
`HAVE_CXX_STDHEADERS` not being defined, which then it turn leads to
the inclusion of `<iostream.h>` (rather than `<iostream>`), which
doesn't exist.

According to a mailing list post discussing a similar problem [1],
"OpenBSD provides the POSIX APIs by default", so we don't need this
define anyway and can remove it. This fixes the BDB build problem as
described in issue bitcoin#28963.

Tested on OpenBSD 7.4 with clang 13.0.0.

[1] https://www.mail-archive.com/tech@openbsd.org/msg63386.html
@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 18, 2024

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK fanquake
Concept ACK jessebarton

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #28710 (Remove the legacy wallet and BDB dependency by achow101)

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.

@jessebarton
Copy link
Contributor

Testing this on a fresh OpenBSD 7.4 VM I had to also install gtar when running gmake -C depends NO_BOOST=1 NO_LIBEVENT=1 NO_QT=1 NO_SQLITE=1 NO_NATPMP=1 NO_UPNP=1 NO_ZMQ=1 NO_USDT=1

Confirmed when building master I got the error mentioned about <iostream.h> not existing.

After switching branches to #29443 I experienced no issues.

Might be worth updating the OpenBSD Doc to add a note about installing gtar. I can do this if its agreed that makes sense. @theStack

Copy link
Contributor

@jessebarton jessebarton left a comment

Choose a reason for hiding this comment

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

ACK

@theStack
Copy link
Contributor Author

Testing this on a fresh OpenBSD 7.4 VM I had to also install gtar when running gmake -C depends NO_BOOST=1 NO_LIBEVENT=1 NO_QT=1 NO_SQLITE=1 NO_NATPMP=1 NO_UPNP=1 NO_ZMQ=1 NO_USDT=1

Confirmed when building master I got the error mentioned about <iostream.h> not existing.

After switching branches to #29443 I experienced no issues.

Thanks for testing!

Might be worth updating the OpenBSD Doc to add a note about installing gtar. I can do this if its agreed that makes sense. @theStack

Note that the gtar dependency is mentioned in depends/README.md already:

### Install the required dependencies: OpenBSD
pkg_add bash gtar

Maybe linking to that README.md file part in doc/build-openbsd.md would be a good idea. In the course of touching this file, you could also take the chance and bump the "Updated for OpenBSD" version number from 7.3 to 7.4 if you want to test the other build instructions as well, since you are on a fresh OpenBSD 7.4 VM anyway. Feel free to ping me as reviewer if you open a PR.

Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK 0fbf051

@DrahtBot DrahtBot requested a review from jessebarton February 26, 2024 11:30
@fanquake fanquake merged commit ac19235 into bitcoin:master Feb 26, 2024
@theStack theStack deleted the 202402-depends-fix_bdb_build_on_openbsd branch February 26, 2024 11:36
jessebarton added a commit to jessebarton/bitcoin that referenced this pull request Feb 26, 2024
Tested and used all build options on OpenBSD 7.4 with no issues.

Added a note about referring to depends/README.md for detailed instructions on required dependencies.
This was added in reference to a conversation in bitcoin#29443
fanquake added a commit that referenced this pull request Feb 27, 2024
fccfdb2 doc: Update OpenBSD build docs to 7.4 (Jesse Barton)

Pull request description:

  Updated OpenBSD Build doc for 7.4 after testing all build options. No issues on my end.

  Also added a note about referring to depends/README.md for detailed instructions on required dependencies.
  This was added in reference to a conversation in #29443

ACKs for top commit:
  fanquake:
    ACK fccfdb2
  theStack:
    lgtm ACK fccfdb2

Tree-SHA512: be6d22b605140b37a71e11c5bbed54f60655832d78cd3cb221eddc77c7621a65c0d71baf436f90819be536d9b5dbf1a0b2c82b6b23d62356addc495403f2ba35
Copy link

@weezyb0893 weezyb0893 left a comment

Choose a reason for hiding this comment

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

Yeah Yeah

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 24, 2024
0fbf051 depends: fix BDB compilation on OpenBSD (Sebastian Falbesoner)

Pull request description:

  Compiling C++ code with `-D_XOPEN_SOURCE=600` causes problems on OpenBSD. If that define is set, the C++ standard header detection routine in BDB's configure script fails due to a missing type name for `locale_t` (see https://gist.github.com/theStack/b41884e31ebc5cdca3220bcaa674cb70 for the relevant config.log part).

  This results in `HAVE_CXX_STDHEADERS` not being defined, which then it turn leads to the inclusion of `<iostream.h>` (rather than `<iostream>`), which doesn't exist, as described in bitcoin#28963.

  According to a mailing list post discussing a similar problem [1], "OpenBSD provides the POSIX APIs by default", so we don't need this define anyway and can remove it. This fixes the BDB build problem as described in issue bitcoin#28963. See also google/flatbuffers@f87e75a for a similar fix for google's flatbuffer project.

  Tested on OpenBSD 7.4 with clang 13.0.0. Fixes bitcoin#28963.

  [1] https://www.mail-archive.com/tech@openbsd.org/msg63386.html

ACKs for top commit:
  fanquake:
    ACK 0fbf051

Tree-SHA512: 02139e9081ed855e067bfba8c81b54c657417576e553cc1035a916ada9be049358f5e14d756d5f234c5226bd7e943f61c6ae8990c1b152f9125681b7b777c9b3
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 24, 2024
0fbf051 depends: fix BDB compilation on OpenBSD (Sebastian Falbesoner)

Pull request description:

  Compiling C++ code with `-D_XOPEN_SOURCE=600` causes problems on OpenBSD. If that define is set, the C++ standard header detection routine in BDB's configure script fails due to a missing type name for `locale_t` (see https://gist.github.com/theStack/b41884e31ebc5cdca3220bcaa674cb70 for the relevant config.log part).

  This results in `HAVE_CXX_STDHEADERS` not being defined, which then it turn leads to the inclusion of `<iostream.h>` (rather than `<iostream>`), which doesn't exist, as described in bitcoin#28963.

  According to a mailing list post discussing a similar problem [1], "OpenBSD provides the POSIX APIs by default", so we don't need this define anyway and can remove it. This fixes the BDB build problem as described in issue bitcoin#28963. See also google/flatbuffers@f87e75a for a similar fix for google's flatbuffer project.

  Tested on OpenBSD 7.4 with clang 13.0.0. Fixes bitcoin#28963.

  [1] https://www.mail-archive.com/tech@openbsd.org/msg63386.html

ACKs for top commit:
  fanquake:
    ACK 0fbf051

Tree-SHA512: 02139e9081ed855e067bfba8c81b54c657417576e553cc1035a916ada9be049358f5e14d756d5f234c5226bd7e943f61c6ae8990c1b152f9125681b7b777c9b3
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Oct 24, 2024
b70e091 Merge bitcoin#29667: fuzz: actually test garbage >64b in p2p transport test (fanquake)
6d7aa3d Merge bitcoin#29497: test: simplify test_runner.py (fanquake)
d0e15d5 Merge bitcoin#29606: refactor: Reserve memory for ToLower/ToUpper conversions (Ava Chow)
045fa5f Merge bitcoin#29514: tests: Provide more helpful assert_equal errors (Ava Chow)
bd607f0 Merge bitcoin#29393: i2p: log connection was refused due to arbitrary port (Ava Chow)
c961755 Merge bitcoin#29595: doc: Wrap flags with code in developer-notes.md (fanquake)
8d6e5e7 Merge bitcoin#29583: fuzz: Apply fuzz env (suppressions, etc.) when fetching harness list (fanquake)
4dce690 Merge bitcoin#29576: Update functional test runner to return error code when no tests are found to run (fanquake)
910a7d6 Merge bitcoin#29529: fuzz: restrict fopencookie usage to Linux & FreeBSD (fanquake)
fdac2b3 Merge bitcoin#29493: subtree: update crc32c subtree (fanquake)
a23b342 Merge bitcoin#29475: doc: Fix Broken Links (fanquake)
92bad90 Merge bitcoin#28178: fuzz: Generate with random libFuzzer settings (fanquake)
9b6a05d Merge bitcoin#29443: depends: fix BDB compilation on OpenBSD (fanquake)
9963e6b Merge bitcoin#29413: fuzz: increase length of string used for `NetWhitelist{bind}Permissions::TryParse` (fanquake)
3914745 Merge bitcoin#29425: test: fix intermittent failure in wallet_reorgrestore.py (fanquake)
b719883 Merge bitcoin#29399: test: Fix utxo set hash serialisation signedness (fanquake)
f096880 Merge bitcoin#29377: test: Add makefile target for running unit tests (Ava Chow)
03e0bd3 Merge bitcoin#27319: addrman, refactor: improve stochastic test in `AddSingle` (Ava Chow)

Pull request description:

  ## Issue being fixed or feature implemented
  Batch of trivial backports

  ## What was done?
  See commits

  ## How Has This Been Tested?
  built locally; large combined merge passed tests locally

  ## Breaking Changes
  Should be none

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK b70e091
  knst:
    utACK b70e091

Tree-SHA512: 659a931f812c8a92cf3854abb873d92885219a392d6aa8e49ac4b27fe41e8e163ef9a135050e7c2e1bd33cecd2f7dae215e05a9c29f62e837e0057d3c16746d6
@bitcoin bitcoin locked and limited conversation to collaborators Mar 12, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Building a wallet with legacy support fails on OpenBSD 7.4
5 participants