build: configure.ac and netbsd-build.md for NetBSD #19430

midnightmagic · 2020-07-02T11:26:35Z

At the moment these changes are not functional on master as of a few minutes ago,
failing with an Abort trap in gmake check, but this can be fixed in another
PR. These particular changes should be otherwise correct and back-portable to
v0.20.0, and will end up with a working gmake check there.

Hopefully these changes will be of some use to the, apparently, other NetBSD
user out there who is by now on an outdated version.

working on NetBSD. At the moment they are not functional on master as of a few minutes ago, failing with an Abort trap in gmake check, but this can be fixed in another PR. These particular changes should be correct and back-portable to v0.20.0, and will end up with a working gmake check there. Hopefully these changes will be of some use to the, apparently, other NetBSD user out there who is by now on an outdated version.

midnightmagic · 2020-07-03T03:30:23Z

At commit a24806c, gmake check is now working just fine on my NetBSD test machine with the configure.ac changes in this PR..

laanwj · 2020-07-09T12:42:21Z

doc/build-netbsd.md

+MAKE="gmake" \
+CPPFLAGS="-I/usr/pkg/include" LDFLAGS="-L/usr/pkg/lib -Wl,-R/usr/pkg/lib" \
+BDB_LIBS="-L${BDB_PREFIX}/lib -ldb_cxx-4.8" BDB_CFLAGS="-I${BDB_PREFIX}/include" \
+./configure --with-gui=no --with-boost=/usr/pkg


Why are you canging this around? In general we prefer putting the environment variables after the ./configure because then they are retained if configure is automatically re-run by the build system (e.g. after a git pul lthat changes one of the build system files).

I'm not stuck on why particular way, or order, of anything. It's entirely a habit born of porting stuff to NetBSD, idea being to keep changes as unobtrusive and light as possible, and to leave the onus on the NetBSD side to maintain/make changes that are specific to NetBSD. I can switch it up if you think that's better. I don't think any of these things will (presumably) ever be run on anything but an end-user's NetBSD machine who is doing a manual one-off build and carefully testing it by hand.

I'd prefer to keep it as-is, it's the convention that we use everywhere and if there is no need to deviate from it on NetBSD, it's better to keep it more or less the same. This makes maintenance easier for us.

Yeah sure thing. I'm completely fine with that. Technically no need whatsoever. Purely habit.

luke-jr · 2020-07-23T23:44:43Z

configure.ac

@@ -1015,11 +1015,18 @@ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <stdint.h>

 dnl LevelDB platform checks
 AC_MSG_CHECKING(for fdatasync)
+case $host in
+  *netbsd*)
+AC_MSG_RESULT(no because NetBSD will not fdatasync on directories and this breaks leveldb); HAVE_FDATASYNC=0


Isn't there a better way to detect this condition?

Yes; we can detect it by running fdatasync() on a directory that we know exists and observing the result. I'd be glad to rewrite it into an autoconf run-program, but things like includes are disparate across platforms for this and a generic program would likely only be functional on tested platforms. A check for the name of the platform (programmed logic) rather than detected behaviour was simpler and—I figured—perhaps less effort and less prone to failure modes. I'd hate to have corrected this behaviour on NetBSD and then made configure non-functional on a platform I don't currently have access to. :-)

I considered that, but it won't work if we're cross-compiling.

BTW, how does it break LevelDB?

LevelDB, if HAVE_FDATASYNC is enabled, attempts it on full directories. When bitcoind attempts to commit data to (any) leveldb it issues fdatasync on a directory, which fails and generates an incomprehensible error for the user. bitcoind then exits. Without invasive changes in the leveldb code which special-case NetBSD (or.. platforms where fdatasync on a directory won't work) then addressing this "properly" won't happen. Easier to just disable it.

luke-jr · 2020-07-23T23:46:13Z

configure.ac

@@ -1015,11 +1015,18 @@ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <stdint.h>

 dnl LevelDB platform checks
 AC_MSG_CHECKING(for fdatasync)
+case $host in
+  *netbsd*)
+AC_MSG_RESULT(no because NetBSD will not fdatasync on directories and this breaks leveldb); HAVE_FDATASYNC=0


We use fdatasync on files in FileCommit. Currently, it checks __linux__ || __NetBSD__, but it really should be using HAVE_FDATASYNC defined here.

Some other solution seems desirable...

Is this a NetBSD bug?

This is not a NetBSD bug. The POSIX/SUSvX whatever it is now is being more liberally interpreted by Linux as supporting directory fdatasync, but arguably this is incorrect. NetBSD interprets conditions and/or functionality like this quite a bit more strictly than Linux does, and does so (somewhat) independently and without working to be compatible with Linux. (At least, it has in the past. I have no idea if this is "intended" behaviour but it's been around for a really long time and they tend to be reluctant to change POSIX-y behaviour without e.g. rulings from on high..)

I can fix whatever you''d like me to fix, in whatever capacity you'd prefer, including fiddling with the logic inside leveldb but "The NetBSD Way" is to absolutely minimize the patches required to get software going on NetBSD in order to reduce load and etc on upstream and make patches palatable. I'm fine with whatever you guys think is best.

Rather than just using it on Linux and NetBSD, use `fdatasync()` based on whether it's available. i.e it is available in newer versions of FreeBSD (11.1 and later). This also aligns our code more closely with what is being done in leveldb. Was pointed out by Luke in bitcoin#19430.

1d8338d util: use HAVE_FDATASYNC to determine fdatasync() use (fanquake) Pull request description: Rather than just using on Linux and NetBSD, use `fdatasync()` based on whether it's available. i.e `fdatasync` is available in newer versions of FreeBSD. This also aligns more closely with what is being done in leveldb. Was pointed out by Luke in #19430. ACKs for top commit: practicalswift: ACK 1d8338d -- patch looks correct laanwj: ACK 1d8338d hebasto: ACK 1d8338d Tree-SHA512: 7dd6d87f5dc0c0ba21ae42f96b63fc12b34806cd536457fc4284f14bb8c235765344be228b000c6adf4cd1e8c4e6a03a18ca18ab22599c42cc3b706e0bcd1a17

…) use 1d8338d util: use HAVE_FDATASYNC to determine fdatasync() use (fanquake) Pull request description: Rather than just using on Linux and NetBSD, use `fdatasync()` based on whether it's available. i.e `fdatasync` is available in newer versions of FreeBSD. This also aligns more closely with what is being done in leveldb. Was pointed out by Luke in bitcoin#19430. ACKs for top commit: practicalswift: ACK 1d8338d -- patch looks correct laanwj: ACK 1d8338d hebasto: ACK 1d8338d Tree-SHA512: 7dd6d87f5dc0c0ba21ae42f96b63fc12b34806cd536457fc4284f14bb8c235765344be228b000c6adf4cd1e8c4e6a03a18ca18ab22599c42cc3b706e0bcd1a17

Rather than just using it on Linux and NetBSD, use `fdatasync()` based on whether it's available. i.e it is available in newer versions of FreeBSD (11.1 and later). This also aligns our code more closely with what is being done in leveldb. Was pointed out by Luke in bitcoin#19430.

DrahtBot · 2020-08-20T22:04:12Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Bugfix: Define and use HAVE_FDATASYNC correctly outside LevelDB #19803 (Bugfix: Define and use HAVE_FDATASYNC correctly outside LevelDB by luke-jr)
Fix possible data race when committing block files #14501 (Fix possible data race when committing block files 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.

DrahtBot · 2020-08-31T06:08:01Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

fanquake

@midnightmagic are you planning on following up here? Looks like after some revertions, the changes here are essentially adding devel/ to the packages list, and the build system change, assuming it's still required.

fanquake · 2021-04-01T14:06:04Z

doc/build-netbsd.md

 ```

-Build and run the tests:
+Build and run the tests (very important!):


I think you can drop this addition as well.

midnightmagic · 2021-04-01T15:51:42Z

It was required at the time, yes. I no longer have machines on which this can be tested. It will have to wait until I have rebuilt that section of my infrastructure.

fanquake · 2021-05-31T06:31:37Z

@midnightmagic if you get back to testing this, let us know, and this can be reopened. I'm going to close this for now.

…) use 1d8338d util: use HAVE_FDATASYNC to determine fdatasync() use (fanquake) Pull request description: Rather than just using on Linux and NetBSD, use `fdatasync()` based on whether it's available. i.e `fdatasync` is available in newer versions of FreeBSD. This also aligns more closely with what is being done in leveldb. Was pointed out by Luke in bitcoin#19430. ACKs for top commit: practicalswift: ACK 1d8338d -- patch looks correct laanwj: ACK 1d8338d hebasto: ACK 1d8338d Tree-SHA512: 7dd6d87f5dc0c0ba21ae42f96b63fc12b34806cd536457fc4284f14bb8c235765344be228b000c6adf4cd1e8c4e6a03a18ca18ab22599c42cc3b706e0bcd1a17

fanquake added the Build system label Jul 2, 2020

laanwj reviewed Jul 9, 2020

View reviewed changes

fanquake added the Waiting for author label Jul 21, 2020

luke-jr suggested changes Jul 23, 2020

View reviewed changes

fanquake mentioned this pull request Jul 28, 2020

util: use HAVE_FDATASYNC to determine fdatasync() use #19614

Merged

This was referenced Aug 21, 2020

Fix possible data race when committing block files #14501

Merged

Bugfix: Define and use HAVE_FDATASYNC correctly outside LevelDB #19803

Merged

DrahtBot added the Needs rebase label Aug 31, 2020

fanquake reviewed Apr 1, 2021

View reviewed changes

fanquake closed this May 31, 2021

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

build: configure.ac and netbsd-build.md for NetBSD #19430

build: configure.ac and netbsd-build.md for NetBSD #19430

Uh oh!

Conversation

midnightmagic commented Jul 2, 2020

Uh oh!

midnightmagic commented Jul 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

midnightmagic Jul 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

midnightmagic Jul 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

DrahtBot commented Aug 31, 2020

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

midnightmagic commented Apr 1, 2021

Uh oh!

fanquake commented May 31, 2021

Uh oh!

Uh oh!

midnightmagic Jul 24, 2020 •

edited

Loading

midnightmagic Jul 24, 2020 •

edited

Loading

DrahtBot commented Aug 20, 2020 •

edited

Loading