build: don't append valgrind CPPFLAGS if not installed (macOS) #1019

pull fanquake wants to merge 1 commits into bitcoin-core:master from fanquake:dont_cpp_if_no_valgrind changing 1 files +2 −2

fanquake commented at 12:10 AM on November 24, 2021: member

Valgrinds CPPFLAGS, i.e -I/usr/local/opt/valgrind/include, are currently added to CPPFLAGS, regardless of whether valgrind is installed. This changes configure so that they are only added if valgrind is available. i.e the output of brew list --versions valgrind is non-null.
build: don't append valgrind CPPFLAGS if not installed 214042a170
real-or-random approved
real-or-random commented at 9:36 AM on November 24, 2021: contributor

ACK 214042a170c860523b7aad2a251b0dbfbaefb235

Same code works in Core https://github.com/bitcoin/bitcoin/commit/d3ef947524a07f8d7fbad5b95781ab6cacb1cb49
real-or-random commented at 6:01 PM on November 24, 2021: contributor

@hebasto It's a trivial change but can you test this on MacOS so that we have at least one ACK from somehow who can actually run this on MacOS? :)
fanquake commented at 6:42 PM on November 24, 2021: member

@real-or-random fwiw I also tested this on a macOS machine.
real-or-random commented at 7:08 PM on November 24, 2021: contributor

@real-or-random fwiw I also tested this on a macOS machine.

Okay sure. I'm fine merging as-is, and almost did, but then also remembered that @hebasto is quick usually.

in configure.ac:47 in 214042a170

  42 | @@ -43,8 +43,8 @@ case $host_os in
  43 |           # These Homebrew packages may be keg-only, meaning that they won't be found
  44 |           # in expected paths because they may conflict with system files. Ask
  45 |           # Homebrew where each one is located, then adjust paths accordingly.
  46 | -         valgrind_prefix=`$BREW --prefix valgrind 2>/dev/null`
  47 | -         if test x$valgrind_prefix != x; then
  48 | +         if $BREW list --versions valgrind >/dev/null; then
  49 | +           valgrind_prefix=`$BREW --prefix valgrind 2>/dev/null`

hebasto commented at 7:18 PM on November 24, 2021:

           valgrind_prefix=$($BREW --prefix valgrind 2>/dev/null)

See https://github.com/koalaman/shellcheck/wiki/SC2006

real-or-random commented at 7:35 PM on November 24, 2021:

Yeah, the Bash FAQ says " ... is the legacy syntax required by only the very oldest of non-POSIX-compatible bourne-shells." But then if we want to perform that cleanup, we should do this change in the entire file...

fanquake commented at 10:55 PM on November 24, 2021:

Yep. I wasn't going to make other random cleanups.

hebasto approved
hebasto commented at 7:20 PM on November 24, 2021: member

ACK 214042a170c860523b7aad2a251b0dbfbaefb235, tested on macOS Big Sur 11.6.1 (20G224, Intel).
real-or-random merged this on Nov 24, 2021
real-or-random closed this on Nov 24, 2021
fanquake deleted the branch on Nov 25, 2021
sipa referenced this in commit d057eae556 on Dec 2, 2021
sipa cross-referenced this on Dec 2, 2021 from issue Update libsecp256k1 subtree to current master by sipa
fanquake referenced this in commit 2b7c7497ef on Dec 3, 2021
fanquake referenced this in commit c4a1e09a8c on Dec 3, 2021
fanquake cross-referenced this on Dec 3, 2021 from issue build: replace backtick command substitution with $() by fanquake
real-or-random referenced this in commit 4900227451 on Dec 3, 2021
sipa referenced this in commit 86dbc4d075 on Dec 15, 2021
jonasnick cross-referenced this on Jan 2, 2022 from issue Sync Upstream by jonasnick
real-or-random referenced this in commit 21e2d65b79 on Jan 5, 2022
gwillen referenced this in commit 35d6112a72 on May 25, 2022
janus referenced this in commit 879a9a27b9 on Jul 10, 2022
patricklodder referenced this in commit 21badcf9d2 on Jul 25, 2022
patricklodder referenced this in commit 03002a9013 on Jul 28, 2022
backpacker69 referenced this in commit 77186f4a04 on Jan 18, 2023
str4d referenced this in commit 6de4698bf9 on Apr 21, 2023
dderjoel referenced this in commit fc13f6d3a4 on May 23, 2023
vmta referenced this in commit e1120c94a1 on Jun 4, 2023
vmta referenced this in commit 8f03457eed on Jul 1, 2023

Contributors

fanquake

real-or-random

hebasto

Linked (view graph)

#1025 build: replace backtick command substitution with $()