Skip to content

Commit fa872c9

Browse files
dongcarldongcarl
committed
depends: Fix id_string invocations
We now use a script named gen_id to generate the base build_id/host_id. This solves 2 problems: 1. GNU Make special-casing exit code 127 (command not found) meant that warnings about missing tools would propagate to the user's terminal and broke our opportunistic build_id construction. 2. This change ensures that we don't have arbitrary characters in our make variables that would be misinterpreted by Make. See comments in depends/Makefile and depends/gen_id for more information.
1 parent 6664211 commit fa872c9

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
@@ -112,37 +112,31 @@ include builders/$(build_os).mk
112112
include builders/default.mk
113113
include packages/packages.mk
114114

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

147141
qrencode_packages_$(NO_QR) = $(qrencode_packages)
148142

‎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)