Skip to content

Commit df21e50

Browse files
laanwjlaanwj
committed
Merge bitcoin/bitcoin#21427: depends: Fix id_string invocations
fa872c9 depends: Fix id_string invocations (Carl Dong) Pull request description: Closes: #21242 ``` Reproduced from depends/Makefile comment: When invoking a shell, GNU Make special-cases exit code 127 (command not found) by not capturing the output but instead passing it through. This is not done for any other exit code. Therefore, we require a "|| true" to avoid this behaviour when in an environment where the build_* or host_* may not exist yet. ``` ACKs for top commit: laanwj: Concept and light code review ACK fa872c9 Tree-SHA512: 9ce88381aec579d956572cf70c4f69dc5a3873f0d2af14a71cf24814192a89452b8280258bed8cca804e4bd2644db056d213ab733df46a10560a47079524d8ac
2 parents cdcf826 + fa872c9 commit df21e50

File tree

3 files changed

+99
-31
lines changed

3 files changed

+99
-31
lines changed

‎depends/Makefile

Lines changed: 24 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -114,37 +114,31 @@ include builders/$(build_os).mk
114114
include builders/default.mk
115115
include packages/packages.mk
116116

117-
full_env=$(shell printenv)
118-
119-
build_id_string:=$(BUILD_ID_SALT)
120-
121-
# GCC only prints COLLECT_LTO_WRAPPER when invoked with just "-v", but we want
122-
# the information from "-v -E -" as well, so just include both.
117+
# Previously, we directly invoked the well-known programs using $(shell ...)
118+
# to contruct build_id_string. However, that was problematic because:
123119
#
124-
# '3>&1 1>&2 2>&3 > /dev/null' is supposed to swap stdin and stdout and silence
125-
# stdin, since we only want the stderr output
126-
build_id_string+=$(shell $(build_CC) -v < /dev/null 3>&1 1>&2 2>&3 > /dev/null) $(shell $(build_CC) -v -E - < /dev/null 3>&1 1>&2 2>&3 > /dev/null)
127-
build_id_string+=$(shell $(build_AR) --version 2>/dev/null) $(filter AR_%,$(full_env)) ZERO_AR_DATE=$(ZERO_AR_DATE)
128-
build_id_string+=$(shell $(build_CXX) -v < /dev/null 3>&1 1>&2 2>&3 > /dev/null) $(shell $(build_CXX) -v -E - < /dev/null 3>&1 1>&2 2>&3 > /dev/null)
129-
build_id_string+=$(shell $(build_RANLIB) --version 2>/dev/null) $(filter RANLIB_%,$(full_env))
130-
build_id_string+=$(shell $(build_STRIP) --version 2>/dev/null) $(filter STRIP_%,$(full_env))
131-
132-
$(host_arch)_$(host_os)_id_string:=$(HOST_ID_SALT)
133-
$(host_arch)_$(host_os)_id_string+=$(shell $(host_CC) -v < /dev/null 3>&1 1>&2 2>&3 > /dev/null) $(shell $(host_CC) -v -E - < /dev/null 3>&1 1>&2 2>&3 > /dev/null)
134-
$(host_arch)_$(host_os)_id_string+=$(shell $(host_AR) --version 2>/dev/null) $(filter AR_%,$(full_env)) ZERO_AR_DATE=$(ZERO_AR_DATE)
135-
$(host_arch)_$(host_os)_id_string+=$(shell $(host_CXX) -v < /dev/null 3>&1 1>&2 2>&3 > /dev/null) $(shell $(host_CXX) -v -E - < /dev/null 3>&1 1>&2 2>&3 > /dev/null)
136-
$(host_arch)_$(host_os)_id_string+=$(shell $(host_RANLIB) --version 2>/dev/null) $(filter RANLIB_%,$(full_env))
137-
$(host_arch)_$(host_os)_id_string+=$(shell $(host_STRIP) --version 2>/dev/null) $(filter STRIP_%,$(full_env))
138-
139-
ifneq ($(strip $(FORCE_USE_SYSTEM_CLANG)),)
140-
# Make sure that cache is invalidated when switching between system and
141-
# depends-managed, pinned clang
142-
build_id_string+=system_clang
143-
$(host_arch)_$(host_os)_id_string+=system_clang
144-
endif
145-
146-
build_id_string+=GUIX_ENVIRONMENT=$(realpath $(GUIX_ENVIRONMENT))
147-
$(host_arch)_$(host_os)_id_string+=GUIX_ENVIRONMENT=$(realpath $(GUIX_ENVIRONMENT))
120+
# 1. When invoking a shell, GNU Make special-cases exit code 127 (command not
121+
# found) by not capturing the output but instead passing it through. This is
122+
# not done for any other exit code.
123+
#
124+
# 2. Characters like '#' (from these programs' output) would end up in make
125+
# variables like build_id_string, which would be wrongly interpreted by make
126+
# when these variables were used.
127+
#
128+
# Therefore, we should avoid having arbitrary strings in make variables where
129+
# possible. The gen_id script used here hashes the output to construct a
130+
# "make-safe" id.
131+
#
132+
# Also note that these lines need to be:
133+
#
134+
# 1. After including {hosts,builders}/*.mk, since they rely on the tool
135+
# variables (e.g. build_CC, host_STRIP, etc.) to be set.
136+
#
137+
# 2. Before including packages/*.mk (excluding packages/packages.mk), since
138+
# they rely on the build_id variables
139+
#
140+
build_id:=$(shell env CC='$(build_CC)' CXX='$(build_CXX)' AR='$(build_AR)' RANLIB='$(build_RANLIB)' STRIP='$(build_STRIP)' SHA256SUM='$(build_SHA256SUM)' DEBUG='$(DEBUG)' ./gen_id '$(BUILD_ID_SALT)' 'GUIX_ENVIRONMENT=$(realpath $(GUIX_ENVIRONMENT))')
141+
$(host_arch)_$(host_os)_id:=$(shell env CC='$(host_CC)' CXX='$(host_CXX)' AR='$(host_AR)' RANLIB='$(host_RANLIB)' STRIP='$(host_STRIP)' SHA256SUM='$(build_SHA256SUM)' DEBUG='$(DEBUG)' ./gen_id '$(HOST_ID_SALT)' 'GUIX_ENVIRONMENT=$(realpath $(GUIX_ENVIRONMENT))')
148142

149143
qrencode_packages_$(NO_QR) = $(qrencode_packages)
150144

‎depends/funcs.mk

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ define int_get_build_id
4949
$(eval $(1)_dependencies += $($(1)_$(host_arch)_$(host_os)_dependencies) $($(1)_$(host_os)_dependencies))
5050
$(eval $(1)_all_dependencies:=$(call int_get_all_dependencies,$(1),$($($(1)_type)_native_toolchain) $($($(1)_type)_native_binutils) $($(1)_dependencies)))
5151
$(foreach dep,$($(1)_all_dependencies),$(eval $(1)_build_id_deps+=$(dep)-$($(dep)_version)-$($(dep)_recipe_hash)))
52-
$(eval $(1)_build_id_long:=$(1)-$($(1)_version)-$($(1)_recipe_hash)-$(release_type) $($(1)_build_id_deps) $($($(1)_type)_id_string))
52+
$(eval $(1)_build_id_long:=$(1)-$($(1)_version)-$($(1)_recipe_hash)-$(release_type) $($(1)_build_id_deps) $($($(1)_type)_id))
5353
$(eval $(1)_build_id:=$(shell echo -n "$($(1)_build_id_long)" | $(build_SHA256SUM) | cut -c-$(HASH_LENGTH)))
5454
final_build_id_long+=$($(package)_build_id_long)
5555

‎depends/gen_id

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
#!/usr/bin/env bash
2+
3+
# Usage: env [ CC=... ] [ CXX=... ] [ AR=... ] [ RANLIB=... ] [ STRIP=... ] \
4+
# [ DEBUG=... ] ./build-id [ID_SALT]...
5+
#
6+
# Prints to stdout a SHA256 hash representing the current toolset, used by
7+
# depends/Makefile as a build id for caching purposes (detecting when the
8+
# toolset has changed and the cache needs to be invalidated).
9+
#
10+
# If the DEBUG environment variable is non-empty and the system has `tee`
11+
# available in its $PATH, the pre-image to the SHA256 hash will be printed to
12+
# stderr. This is to help developers debug caching issues in depends.
13+
14+
# This script explicitly does not `set -e` because id determination is mostly
15+
# opportunistic: it is fine that things fail, as long as they fail consistently.
16+
17+
# Command variables (CC/CXX/AR) which can be blank are invoked with `bash -c`,
18+
# because the "command not found" error message printed by shells often include
19+
# the line number, like so:
20+
#
21+
# ./depends/gen_id: line 43: --version: command not found
22+
#
23+
# By invoking with `bash -c`, we ensure that the line number is always 1
24+
25+
(
26+
# Redirect stderr to stdout
27+
exec 2>&1
28+
29+
echo "BEGIN ALL"
30+
31+
# Include any ID salts supplied via command line
32+
echo "BEGIN ID SALT"
33+
echo "$@"
34+
echo "END ID SALT"
35+
36+
# GCC only prints COLLECT_LTO_WRAPPER when invoked with just "-v", but we want
37+
# the information from "-v -E -" as well, so just include both.
38+
echo "BEGIN CC"
39+
bash -c "${CC} -v"
40+
bash -c "${CC} -v -E -xc -o /dev/null - < /dev/null"
41+
bash -c "${CC} -v -E -xobjective-c -o /dev/null - < /dev/null"
42+
echo "END CC"
43+
44+
echo "BEGIN CXX"
45+
bash -c "${CXX} -v"
46+
bash -c "${CXX} -v -E -xc++ -o /dev/null - < /dev/null"
47+
bash -c "${CXX} -v -E -xobjective-c++ -o /dev/null - < /dev/null"
48+
echo "END CXX"
49+
50+
echo "BEGIN AR"
51+
bash -c "${AR} --version"
52+
env | grep '^AR_'
53+
echo "ZERO_AR_DATE=${ZERO_AR_DATE}"
54+
echo "END AR"
55+
56+
echo "BEGIN RANLIB"
57+
bash -c "${RANLIB} --version"
58+
env | grep '^RANLIB_'
59+
echo "END RANLIB"
60+
61+
echo "BEGIN STRIP"
62+
bash -c "${STRIP} --version"
63+
env | grep '^STRIP_'
64+
echo "END STRIP"
65+
66+
echo "END ALL"
67+
) | if [ -n "$DEBUG" ] && command -v tee > /dev/null 2>&1; then
68+
# When debugging and `tee` is available, output the preimage to stderr
69+
# in addition to passing through stdin to stdout
70+
tee >(cat 1>&2)
71+
else
72+
# Otherwise, passthrough stdin to stdout
73+
cat
74+
fi | ${SHA256SUM} - | cut -d' ' -f1

0 commit comments

Comments
 (0)