cmake: Switch from tri-state options to boolean. Stage TWO #162

hebasto · 2024-04-22T04:42:32Z

Same as #161, but USDT-specific with refactoring of the USDT search logic into its own "FindUSDT" module.

The first commit is a pure refactoring.

hebasto · 2024-04-22T04:44:10Z

0xB10C · 2024-04-22T10:19:53Z

One thought I had recently: It might make sense for us to vendor the relevant parts of systemtap's sys/std.h. If we can get this into master before merging cmake, we can drop this check?

Nonetheless, happy to take closer look here.

fanquake · 2024-04-24T09:04:51Z

cmake/module/FindUSDT.cmake

+Result Variables
+^^^^^^^^^^^^^^^^
+
+This module defines the following variables:


Why? If it's not exported / used anywhere outside of this.

It is expected from any Find module.

From CMake docs:

The primary task of a find module is to determine whether a package is available, set the <PackageName>_FOUND variable to reflect this...

It was used in the first commit. However, the second commit removed that usage.

However, the second commit removed that usage.

So then it can be removed? Or are we adding dead/unused code to our build system because of what the CMake docs say? Or are we doing things incorrectly, if we aren't using the variables we are adding, according to the docs?

fanquake · 2024-04-24T09:05:01Z

depends/Makefile

@@ -293,7 +293,7 @@ $(host_prefix)/toolchain.cmake : toolchain.cmake.in $(host_prefix)/.stamp_$(fina
            -e 's|@no_sqlite@|$(NO_SQLITE)|' \
            -e 's|@no_upnp@|$(NO_UPNP)|' \
            -e 's|@no_natpmp@|$(NO_NATPMP)|' \
-            -e 's|@no_usdt@|$(NO_USDT)|' \
+            -e 's|@no_usdt@|$(NO_USDT)|' -e 's|@usdt_packages@|$(usdt_packages_)|' \


Why do we need to do this

Considering

bitcoin/depends/packages/packages.mk

Line 28 in 19722e3

usdt_linux_packages=systemtap

the usdt_packages variable is non-empty for Linux only.

For other systems, in the absence of NO_USDT=1, the depends build system has set(WITH_USDT ON) in the toolchain file, which is wrong.

If this is only relevant for Linux, then we should really fix the depends output based on package availability, rather than working around issues that shouldn't exist, but I'm also not sure if that's true when we get macOS support, in which case, I assume it's just a bug for Windows?

... I'm also not sure if that's true when we get macOS support, in which case, I assume it's just a bug for Windows?

We can return back to this problem in the time when we actually get macOS support.

MacOS (and FreeBSD) tracepoints don't use systemtap and don't require any other dependencies.

I apologise but I feel completely lost. I still don't understand what the bug is.

Is it present in the master branch?
Is it introduced in this PR?
What options do trigger this bug when invoking make -C depends?

I just mean the fact that depends is handling anything USDT related at all, for non-linux systems. If this wasn't the case, we could have cleaner logic here. We can just track this as something to cleanup at some point, but the improvements to master would be relevant pre or post CMake (ideally before as that'll keep the build systems closer between branches).

It looks to me like the current (master) behavior for a non-linux platform is:

USDT is enabled by default

the package doesn't get built by depends

usdt is enabled by the config.site

configure fails to find the headers so it's disabled in the end

On this branch that approach won't work, because it's opt-in only. Therefore @hebasto's solution is to only enable it if it's not disabled and if it's defined for the platform. I think that's a reasonable generic approach.

Though I think we could simply with:

's|@usdt_packages@|$(usdt_packages_$(NO_USDT))|'

Then just test for:

if("@usdt_packages@") set(WITH_USDT ON CACHE BOOL "")

?

And to @fanquake's point, I think that would apply to master as well:

diff --git a/depends/config.site.in b/depends/config.site.in index 7c699087bbc..b6b38bd3148 100644 --- a/depends/config.site.in +++ b/depends/config.site.in @@ -70,7 +70,7 @@ if test -z "$enable_zmq" && test -n "@no_zmq@"; then enable_zmq=no fi -if test -z "$enable_usdt" && test -n "@no_usdt@"; then +if test -z "$enable_usdt" && test -z "@usdt_packages@"; then enable_usdt=no fi

Thanks! Reworked.

fanquake · 2024-04-24T09:06:54Z

One thought I had recently: It might make sense for us to vendor the relevant parts of systemtap's sys/std.h.

Why?

0xB10C · 2024-04-24T10:33:16Z

One thought I had recently: It might make sense for us to vendor the relevant parts of systemtap's sys/std.h.

Why?

We don't use >99% of systemtap, and only need includes/sys/sdt.h. Vendoring allows us to drop systemtap as a dependency and remove the depends package definition and build related code. If done before the cmake merge, we don't have to add a FindUSDT module. sdt.h hasn't changed much over the last years since we added it as depends in 2022 and I don't expect this to change giving it's now 15 years old. It only really makes sense to do this after bitcoin#26593, when Linux tracepoints only have overhead if actually used.

There are two ways of doing this:

As sdt.h is available and tracepoints don't have overhead, compile them in every binary.
Keep a configuration/cmake option to enable/disable tracepoints.

If you have any concerns in terms of e.g. impact on maintainability, please let me know. I'm currently not aware of large arguments against it.

I briefly mentioned this to @laanwj and had the impression that he agreed with the general direction - I could be wrong.

A wip branch can be found here: https://github.com/0xB10C/bitcoin/commits/2024-04-vendor-systemtap-sys-sdt-h/

fanquake · 2024-04-24T10:37:51Z

Vendoring allows us to drop systemtap as a dependency and remove the depends package definition and build related code.

We don't drop it as a dependency, just internalize it, and have to make sure it actually stays updated / we pull fixes/changes when necessary. (in this case it may be simple enough).

Keep a configuration/cmake option to enable/disable tracepoints.

This will always be required.

hebasto · 2024-04-24T11:03:51Z

Rebased to resolve conflicts.

laanwj · 2024-04-24T11:20:41Z

mf you have any concerns in terms of e.g. impact on maintainability, please let me know. I'm currently not aware of large arguments against it.

Yes, i agree on that. It's important to realize how simple this header is, for just defining the probes. It's just a bunch of macros (that add a symbol and ELF metadata), and reducing it to just what we use allows making it even smaller. No actual function definitions, code, or data structures are defined. There's very little to bugfix or maintain, or that could even change over time.

theuni · 2024-04-29T14:49:52Z

So vendoring USDT would eliminate the search for the dependency, then it's simply a question for the builder of on or off? That sounds quite nice.

hebasto · 2024-04-29T14:53:58Z

So vendoring USDT would eliminate the search for the dependency, then it's simply a question for the builder of on or off? That sounds quite nice.

That is for the master branch, right?

This PR scope is different :)

Rework in a target-focused way.

hebasto · 2024-04-30T17:42:10Z

Rebased.

hebasto · 2024-04-30T19:14:43Z

Addressed @theuni's #162 (comment) with taking into account the CMake's if() command property:

A quoted string always evaluates to false unless the string's value is one of the true constants.

5de23c7 fixup! cmake: Add preset for MSVC build (Hennadii Stepanov) 09b1d5b fixup! ci: Test CMake edge cases (Hennadii Stepanov) 820a538 cmake [KILL 3-STATE]: Switch `WITH_QRENCODE` to boolean w/ default ON (Hennadii Stepanov) 017aeda cmake [FIXUP]: Move `find_program(brew)` out from introspection file (Hennadii Stepanov) Pull request description: This PR is a continuation of #161, #162 and #164 and tackles with the `WITH_QRENCODE` options. It becomes enabled when building GUI with the default value `ON`. Top commit has no ACKs. Tree-SHA512: 9cd923ef89e0a485330205db4389e42c001cbd6683a14b1e0c665d8e790e8bb2cae7b8b1929818443aea8eb5cbf6cafa79f4e44f41a893f54a5cdc09abc49dc1

theuni · 2024-05-01T15:58:33Z

Looks good to me. Seems to work as expected. When building for Linux it ends up on by default, but is still override-able by setting -DWITH_USDT=False.

If we decide we'd rather not have it on by default for depends build, we can make it opt-in there rather than opt-out.

theuni · 2024-05-01T16:02:02Z

src/util/trace.h

@@ -5,10 +5,6 @@
 #ifndef BITCOIN_UTIL_TRACE_H
 #define BITCOIN_UTIL_TRACE_H

-#if defined(HAVE_CONFIG_H)


I also really like this change, even if it's just incidental. It's very unlikely that out-of-tree builds would be interested in USDT (though they could of course still access it by setting the define).

One less low-level header including this file.

0xB10C

Looks good to me. Seems to work as expected. When building for Linux it ends up on by default, but is still override-able by setting -DWITH_USDT=False.

Hm, that differs from my result on 598eda8 (cmake [KILL 3-STATE]: Switch WITH_USDT to boolean w/ default OFF). It ends up being off by default for me. I can enable it with -DWITH_USDT=ON.

edit: maybe that's a issue on the NixOS side?

0xB10C · 2024-05-02T08:12:00Z

CMakeLists.txt

-  "if sys/sdt.h is found."
-  AUTO
-)
+option(WITH_USDT "Enable tracepoints for Userspace, Statically Defined Tracing." OFF)


Do we want to have this OFF by default? Since v23.0 (specifically bitcoin#23724), all release builds have had tracepoints compiled in by default as the systemtap headers are available.

Do we want to have this OFF by default?

Every optional dependency become opt-in.

Since v23.0 (specifically bitcoin#23724), all release builds have had tracepoints compiled in by default as the systemtap headers are available.

Release functionality must be preserved. There should be WITH_USDT=ON.

hebasto · 2024-05-02T09:07:53Z

Looks good to me. Seems to work as expected. When building for Linux it ends up on by default, but is still override-able by setting -DWITH_USDT=False.

Hm, that differs from my result on 598eda8 (cmake [KILL 3-STATE]: Switch WITH_USDT to boolean w/ default OFF). It ends up being off by default for me. I can enable it with -DWITH_USDT=ON.

I've just checked the staging branch on Ubuntu 23.10. It works:

...
-- Performing Test HAVE_USDT_H
-- Performing Test HAVE_USDT_H - Success
-- Found USDT: /home/hebasto/git/bitcoin/depends/x86_64-pc-linux-gnu/include
...

edit: maybe that's a issue on the NixOS side?

It seems so. Let's continue the further discussion in #126.

0xB10C · 2024-05-02T10:53:53Z

I've just checked the staging branch on Ubuntu 23.10. It works:

What exactly did you run?

For me cmake -S .. -DWITH_USDT=ON works as well. I don't think it's an NixOS issue.

-- Performing Test HAVE_USDT_H
-- Performing Test HAVE_USDT_H - Success
-- Found USDT: /nix/store/rvdnwsi7jw1jvims79gi6cb9s3gfn2y4-libsystemtap-4.6/include

Plain cmake -S .. results in

Configure summary
=================
Executables:
  bitcoind ............................ ON
  bitcoin-cli ......................... ON
...
  USDT tracing ........................ OFF
...

My confusion is more about @theuni's "When building for Linux it ends up on by default" - which is not the case for me and I wouldn't expect it from the code. (but I think we might want it to be ON by default #162 (comment)?)

hebasto · 2024-05-02T11:26:07Z

I've just checked the staging branch on Ubuntu 23.10. It works:

What exactly did you run?

$ make -C depends
$ cmake -B build --toolchain depends/x86_64-pc-linux-gnu/toolchain.cmake

hebasto · 2024-05-02T11:27:19Z

My confusion is more about @theuni's "When building for Linux it ends up on by default"

It is about building with depends, I guess.

UPD. As it has been said before all optional package (USDT, ZMQ, Qt etc) are to be opt-in.

2b4ad50 cmake [KILL 3-STATE]: Switch `WITH_{SQLITE,BDB}` to boolean (Hennadii Stepanov) Pull request description: This PR is a continuation of #161 and #162 and tackles with the `WITH_SQLITE` and `WITH_BDB` options. For the latter the default value `OFF` is suggested. The #83 (comment): > Shouldn't `WITH_SQLITE=ON` imply `ENABLE_WALLET`? What (extra) does `ENABLE_WALLET` do here? I'm not sure we'll actually want to keep it as an option. has been addressed. ACKs for top commit: TheCharlatan: ACK 2b4ad50 Tree-SHA512: 13d5bd16709ab557ee736696fe1d5d3fe64bae8cb77c000401c26b8be2deab0473474f7fa9bb958884ba3ab9c4d0b66e44766ca5c1c86d0fab3957642ec996b9

hebasto added the enhancement New feature or request label Apr 22, 2024

hebasto force-pushed the 240422-cmake-DW branch from d22142e to 4d8e8f0 Compare April 22, 2024 09:45

hebasto force-pushed the 240422-cmake-DW branch from 4d8e8f0 to 43c9bc3 Compare April 22, 2024 18:10

hebasto mentioned this pull request Apr 22, 2024

cmake: Switch from tri-state options to boolean. Stage THREE #164

Merged

fanquake reviewed Apr 24, 2024

View reviewed changes

hebasto force-pushed the 240422-cmake-DW branch from 43c9bc3 to ffef539 Compare April 24, 2024 11:03

hebasto force-pushed the 240422-cmake-DW branch from ffef539 to 8214ef1 Compare April 24, 2024 11:56

This was referenced Apr 24, 2024

cmake: Switch from tri-state options to boolean. Stage FOUR #169

Merged

[FUTURE] cmake: Switch from tri-state options to ON/OFF #83

Closed

hebasto added this to the User Interface milestone Apr 27, 2024

hebasto force-pushed the cmake-staging branch from 4561f76 to 101f3ef Compare April 30, 2024 12:34

fixup! cmake: Add systemtap-sdt optional package support

f3c2fea

Rework in a target-focused way.

hebasto force-pushed the 240422-cmake-DW branch from 8214ef1 to ab5992a Compare April 30, 2024 17:41

cmake [KILL 3-STATE]: Switch WITH_USDT to boolean w/ default OFF

598eda8

hebasto force-pushed the 240422-cmake-DW branch from ab5992a to 598eda8 Compare April 30, 2024 19:11

theuni reviewed May 1, 2024

View reviewed changes

hebasto merged commit c6261b9 into cmake-staging May 1, 2024
28 checks passed

0xB10C reviewed May 2, 2024

View reviewed changes

This was referenced Nov 11, 2024

tracing: Only prepare tracepoint arguments when actually tracing bitcoin/bitcoin#26593

Merged

Tracepoint Interface Tracking Issue bitcoin/bitcoin#31274

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmake: Switch from tri-state options to boolean. Stage TWO #162

cmake: Switch from tri-state options to boolean. Stage TWO #162

hebasto commented Apr 22, 2024 •

edited

Loading

hebasto commented Apr 22, 2024

0xB10C commented Apr 22, 2024

fanquake Apr 24, 2024

hebasto Apr 24, 2024

fanquake Apr 24, 2024

fanquake Apr 24, 2024

hebasto Apr 24, 2024

fanquake Apr 24, 2024

hebasto Apr 24, 2024

0xB10C Apr 24, 2024

hebasto Apr 25, 2024

fanquake Apr 25, 2024

theuni Apr 30, 2024

theuni Apr 30, 2024 •

edited

Loading

hebasto Apr 30, 2024

fanquake commented Apr 24, 2024

0xB10C commented Apr 24, 2024

fanquake commented Apr 24, 2024 •

edited

Loading

hebasto commented Apr 24, 2024

laanwj commented Apr 24, 2024 •

edited

Loading

theuni commented Apr 29, 2024

hebasto commented Apr 29, 2024

hebasto commented Apr 30, 2024

hebasto commented Apr 30, 2024

theuni commented May 1, 2024

theuni May 1, 2024

0xB10C left a comment •

edited

Loading

0xB10C May 2, 2024

hebasto May 2, 2024

hebasto commented May 2, 2024

0xB10C commented May 2, 2024

hebasto commented May 2, 2024

hebasto commented May 2, 2024 •

edited

Loading

cmake: Switch from tri-state options to boolean. Stage TWO #162

cmake: Switch from tri-state options to boolean. Stage TWO #162

Conversation

hebasto commented Apr 22, 2024 • edited Loading

hebasto commented Apr 22, 2024

0xB10C commented Apr 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theuni Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanquake commented Apr 24, 2024

0xB10C commented Apr 24, 2024

fanquake commented Apr 24, 2024 • edited Loading

hebasto commented Apr 24, 2024

laanwj commented Apr 24, 2024 • edited Loading

theuni commented Apr 29, 2024

hebasto commented Apr 29, 2024

hebasto commented Apr 30, 2024

hebasto commented Apr 30, 2024

theuni commented May 1, 2024

Choose a reason for hiding this comment

0xB10C left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto commented May 2, 2024

0xB10C commented May 2, 2024

hebasto commented May 2, 2024

hebasto commented May 2, 2024 • edited Loading

hebasto commented Apr 22, 2024 •

edited

Loading

theuni Apr 30, 2024 •

edited

Loading

fanquake commented Apr 24, 2024 •

edited

Loading

laanwj commented Apr 24, 2024 •

edited

Loading

0xB10C left a comment •

edited

Loading

hebasto commented May 2, 2024 •

edited

Loading