[gentoo-dev] [PATCH 0/2] cargo.eclass: Change RUSTFLAGS approach following recent build failures

[gentoo-dev] [PATCH 0/2] cargo.eclass: Change RUSTFLAGS approach following recent build failures

[James Le Cuirot]

Unfortunately, my recent cargo.eclass changes didn't fare so well, despite me
testing it on a good handful of packages. I've had to sharply change tack to
work around Cargo's shortcomings.

The first commit has the main change. I'd like to merge that very soon to put
out the immediate fire.

The second commit tries to address a lingering downside where
project-specific flags may get overridden by our own. This was happening,
even before I changed anything for users or ebuilds setting RUSTFLAGS, but
now it is more likely to happen. I'm unsure of this change because it is a
little controverial, using app-misc/yq's tomlq (a jq wrapper) to read flags
from the project's config files. On the plus side, it should avoid upstreams
complaining that we're not using their flags. See what you think. I've kept
it in a separate commit so that it can easily be backed out.

James Le Cuirot (2):
  cargo.eclass: Change RUSTFLAGS approach following recent build
    failures
  cargo.eclass: Preserve project-specific [build] flags by reading
    config

 eclass/cargo.eclass | 73 ++++++++++++++++++++++++++++-----------------
 1 file changed, 46 insertions(+), 27 deletions(-)

--
2.45.2

[gentoo-dev] [PATCH 1/2] cargo.eclass: Change RUSTFLAGS approach following recent build failures

[James Le Cuirot]

Cargo turned out to be even worse at handling flags than I thought.
Target-specific flags set by projects were overriding our own, and Cargo
was bailing out when faced with merging a string of flags with an array
of flags.

After weighing up the poor set of options, I've found that it is better
to always set flags via a target-specific environment variable for
reasons set out in comments added here. This approach was previously
just used for cross-compiling, but now we do it unconditionally.

It has the downside of overriding generic [build] flags set by projects,
but these were already being overridden by users and ebuilds setting
RUSTFLAGS themselves.

Closes: https://bugs.gentoo.org/937453
Closes: https://bugs.gentoo.org/937470
Signed-off-by: James Le Cuirot <chewi@gentoo.org>
---
 eclass/cargo.eclass | 65 +++++++++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 26 deletions(-)

diff --git a/eclass/cargo.eclass b/eclass/cargo.eclass
index c360c2a6c419..6d4cf1b425eb 100644
--- a/eclass/cargo.eclass
+++ b/eclass/cargo.eclass
@@ -259,19 +259,6 @@ cargo_crate_uris() {
 cargo_gen_config() {
 	debug-print-function ${FUNCNAME} "$@"
 
-	# The default linker is "cc" so override by setting linker to CC in the
-	# RUSTFLAGS. The given linker cannot include any arguments, so split these
-	# into link-args along with LDFLAGS. Also include external RUSTFLAGS.
-	# Note that as of Rust 1.80, the build host RUSTFLAGS are ignored when
-	# cross-compiling unless you use the unstable host-config feature available
-	# with USE=nightly. There is no simple way around this.
-	tc-export_build_env
-	local LD_A=( $(tc-getBUILD_CC) ${BUILD_LDFLAGS} )
-	local MY_BUILD_RUSTFLAGS="-C strip=none -C linker=${LD_A[0]}"
-	[[ ${#LD_A[@]} -gt 1 ]] && MY_BUILD_RUSTFLAGS+="$(printf -- ' -C link-arg=%s' "${LD_A[@]:1}")"
-	MY_BUILD_RUSTFLAGS+=" ${RUSTFLAGS} ${BUILD_RUSTFLAGS}"
-	tc-is-cross-compiler || MY_BUILD_RUSTFLAGS+=" ${TARGET_RUSTFLAGS}"
-
 	mkdir -p "${ECARGO_HOME}" || die
 
 	cat > "${ECARGO_HOME}/config.toml" <<- _EOF_ || die "Failed to create cargo config"
@@ -286,7 +273,6 @@ cargo_gen_config() {
 	offline = true
 
 	[build]
-	rustflags = "${MY_BUILD_RUSTFLAGS}"
 	jobs = $(makeopts_jobs)
 	incremental = false
 
@@ -541,10 +527,11 @@ cargo_src_configure() {
 # @USAGE: Command with its arguments
 # @DESCRIPTION:
 # Run the given command under an environment needed for performing tasks with
-# Cargo such as building. RUSTFLAGS is used for both the build and target host.
-# BUILD_RUSTFLAGS and TARGET_RUSTFLAGS are used for just the build host and
-# target host respectively. Ensure these are set consistently between Cargo
-# invocations, otherwise rebuilds will occur.
+# Cargo such as building. RUSTFLAGS are appended to additional flags set here.
+# Ensure these are set consistently between Cargo invocations, otherwise
+# rebuilds will occur. Project-specific rustflags set against [build] will not
+# take affect due to Cargo limitations, so add these to your ebuild's RUSTFLAGS
+# if they seem important.
 cargo_env() {
 	[[ ${_CARGO_GEN_CONFIG_HAS_RUN} ]] || \
 		die "FATAL: please call cargo_gen_config before using ${FUNCNAME}"
@@ -569,14 +556,40 @@ cargo_env() {
 		HOST_CFLAGS=${BUILD_CFLAGS}
 		HOST_CXXFLAGS=${BUILD_CXXFLAGS}
 
-	if tc-is-cross-compiler; then
-		local -x CARGO_BUILD_TARGET=$(rust_abi)
-		local TRIPLE=${CARGO_BUILD_TARGET//-/_}
-		local TRIPLE=${TRIPLE^^} LD_A=( $(tc-getCC) ${LDFLAGS} )
-		local -x CARGO_TARGET_"${TRIPLE}"_RUSTFLAGS="-C strip=none -C linker=${LD_A[0]}"
-		[[ ${#LD_A[@]} -gt 1 ]] && local CARGO_TARGET_"${TRIPLE}"_RUSTFLAGS+="$(printf -- ' -C link-arg=%s' "${LD_A[@]:1}")"
-		local CARGO_TARGET_"${TRIPLE}"_RUSTFLAGS+=" ${RUSTFLAGS} ${TARGET_RUSTFLAGS}"
-	fi
+	# Unfortunately, Cargo is *really* bad at handling flags. In short, it uses
+	# the first of the RUSTFLAGS env var, any target-specific config, and then
+	# any generic [build] config. It can merge within the latter two types from
+	# different sources, but it will not merge across these different types, so
+	# if a project sets flags under [target.'cfg(all())'], it will override any
+	# flags we set under [build] and vice-versa.
+	#
+	# It has been common for users and ebuilds to set RUSTFLAGS, which would
+	# have overridden whatever a project sets anyway, so the least-worst option
+	# is to include those RUSTFLAGS in target-specific config here, which will
+	# merge with any the project sets. Only flags in generic [build] config set
+	# by the project will be lost, and ebuilds will need to add those to
+	# RUSTFLAGS themselves if they are important.
+	#
+	# We could potentially inspect a project's generic [build] config and
+	# reapply those flags ourselves, but that would require a proper toml parser
+	# like tomlq, it might lead to confusion where projects also have
+	# target-specific config, and converting arrays to strings may not work
+	# well. Nightly features to inspect the config might help here in future.
+	#
+	# As of Rust 1.80, it is not possible to set separate flags for the build
+	# host and the target host when cross-compiling. The flags given are applied
+	# to the target host only with no flags being applied to the build host. The
+	# nightly host-config feature will improve this situation later.
+	#
+	# The default linker is "cc" so override by setting linker to CC in the
+	# RUSTFLAGS. The given linker cannot include any arguments, so split these
+	# into link-args along with LDFLAGS.
+	local -x CARGO_BUILD_TARGET=$(rust_abi)
+	local TRIPLE=${CARGO_BUILD_TARGET//-/_}
+	local TRIPLE=${TRIPLE^^} LD_A=( $(tc-getCC) ${LDFLAGS} )
+	local -x CARGO_TARGET_"${TRIPLE}"_RUSTFLAGS="-C strip=none -C linker=${LD_A[0]}"
+	[[ ${#LD_A[@]} -gt 1 ]] && local CARGO_TARGET_"${TRIPLE}"_RUSTFLAGS+="$(printf -- ' -C link-arg=%s' "${LD_A[@]:1}")"
+	local CARGO_TARGET_"${TRIPLE}"_RUSTFLAGS+=" ${RUSTFLAGS}"
 
 	(
 		# These variables will override the above, even if empty, so unset them
-- 
2.45.2

[gentoo-dev] [PATCH 2/2] cargo.eclass: Preserve project-specific [build] flags by reading config

[James Le Cuirot]

The flags we set an a target-specific environment variable override any
generic [build] flags set by the project, requiring ebuilds to set these
themselves, which is undesirable. Work around this by using tomlq to
read the flags from the config files checked by Cargo and prepending
them to our environment variable.

Signed-off-by: James Le Cuirot <chewi@gentoo.org>
---
 eclass/cargo.eclass | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/eclass/cargo.eclass b/eclass/cargo.eclass
index 6d4cf1b425eb..dea8c49e4585 100644
--- a/eclass/cargo.eclass
+++ b/eclass/cargo.eclass
@@ -36,7 +36,8 @@ esac
 
 inherit flag-o-matic multiprocessing rust-toolchain toolchain-funcs
 
-[[ ! ${CARGO_OPTIONAL} ]] && BDEPEND="${RUST_DEPEND}"
+# app-misc/yq is needed for tomlq.
+[[ ! ${CARGO_OPTIONAL} ]] && BDEPEND="${RUST_DEPEND} app-misc/yq"
 
 IUSE="${IUSE} debug"
 
@@ -566,21 +567,26 @@ cargo_env() {
 	# It has been common for users and ebuilds to set RUSTFLAGS, which would
 	# have overridden whatever a project sets anyway, so the least-worst option
 	# is to include those RUSTFLAGS in target-specific config here, which will
-	# merge with any the project sets. Only flags in generic [build] config set
-	# by the project will be lost, and ebuilds will need to add those to
-	# RUSTFLAGS themselves if they are important.
-	#
-	# We could potentially inspect a project's generic [build] config and
-	# reapply those flags ourselves, but that would require a proper toml parser
-	# like tomlq, it might lead to confusion where projects also have
-	# target-specific config, and converting arrays to strings may not work
-	# well. Nightly features to inspect the config might help here in future.
+	# merge with any the project sets. Flags in generic [build] config set by
+	# the project would then be overridden, but we preserve them by reading them
+	# from config files and prepending them to RUSTFLAGS.
 	#
 	# As of Rust 1.80, it is not possible to set separate flags for the build
 	# host and the target host when cross-compiling. The flags given are applied
 	# to the target host only with no flags being applied to the build host. The
 	# nightly host-config feature will improve this situation later.
-	#
+
+	# Iterate over all the config.toml files that Cargo will read except for our
+	# own, prepending any build.rustflags to our environment variable so that
+	# they don't get overridden by our other flags. Skip any flags with an
+	# explicit space in them because we cannot handle those via our variable.
+	local DIR=${PWD}
+	while true; do
+		RUSTFLAGS="$(tomlq -r '.build.rustflags | if type == "array" then map(select(contains(" ") | not)) else [.] end | join(" ")' "${DIR}"/.cargo/config.toml 2>/dev/null) ${RUSTFLAGS# }"
+		[[ -d ${DIR} ]] || break
+		DIR=${DIR%/*}
+	done
+
 	# The default linker is "cc" so override by setting linker to CC in the
 	# RUSTFLAGS. The given linker cannot include any arguments, so split these
 	# into link-args along with LDFLAGS.
-- 
2.45.2

Re: [gentoo-dev] [PATCH 2/2] cargo.eclass: Preserve project-specific [build] flags by reading config

[Michał Górny]

[-- Attachment #1: Type: text/plain, Size: 1153 bytes --]

On Wed, 2024-08-07 at 16:52 +0100, James Le Cuirot wrote:
> The flags we set an a target-specific environment variable override any
> generic [build] flags set by the project, requiring ebuilds to set these
> themselves, which is undesirable. Work around this by using tomlq to
> read the flags from the config files checked by Cargo and prepending
> them to our environment variable.
> 
> Signed-off-by: James Le Cuirot <chewi@gentoo.org>
> ---
>  eclass/cargo.eclass | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/eclass/cargo.eclass b/eclass/cargo.eclass
> index 6d4cf1b425eb..dea8c49e4585 100644
> --- a/eclass/cargo.eclass
> +++ b/eclass/cargo.eclass
> @@ -36,7 +36,8 @@ esac
>  
>  inherit flag-o-matic multiprocessing rust-toolchain toolchain-funcs
>  
> -[[ ! ${CARGO_OPTIONAL} ]] && BDEPEND="${RUST_DEPEND}"
> +# app-misc/yq is needed for tomlq.
> +[[ ! ${CARGO_OPTIONAL} ]] && BDEPEND="${RUST_DEPEND} app-misc/yq"
>  

Doesn't this imply that all ebuilds using CARGO_OPTIONAL will now have
to explicitly depend on yq?

-- 
Best regards,
Michał Górny


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [gentoo-dev] [PATCH 2/2] cargo.eclass: Preserve project-specific [build] flags by reading config

[James Le Cuirot]

[-- Attachment #1: Type: text/plain, Size: 1753 bytes --]

On Wed, 2024-08-07 at 19:39 +0200, Michał Górny wrote:
> On Wed, 2024-08-07 at 16:52 +0100, James Le Cuirot wrote:
> > The flags we set an a target-specific environment variable override any
> > generic [build] flags set by the project, requiring ebuilds to set these
> > themselves, which is undesirable. Work around this by using tomlq to
> > read the flags from the config files checked by Cargo and prepending
> > them to our environment variable.
> > 
> > Signed-off-by: James Le Cuirot <chewi@gentoo.org>
> > ---
> >  eclass/cargo.eclass | 28 +++++++++++++++++-----------
> >  1 file changed, 17 insertions(+), 11 deletions(-)
> > 
> > diff --git a/eclass/cargo.eclass b/eclass/cargo.eclass
> > index 6d4cf1b425eb..dea8c49e4585 100644
> > --- a/eclass/cargo.eclass
> > +++ b/eclass/cargo.eclass
> > @@ -36,7 +36,8 @@ esac
> >  
> >  inherit flag-o-matic multiprocessing rust-toolchain toolchain-funcs
> >  
> > -[[ ! ${CARGO_OPTIONAL} ]] && BDEPEND="${RUST_DEPEND}"
> > +# app-misc/yq is needed for tomlq.
> > +[[ ! ${CARGO_OPTIONAL} ]] && BDEPEND="${RUST_DEPEND} app-misc/yq"
> >  
> 
> Doesn't this imply that all ebuilds using CARGO_OPTIONAL will now have
> to explicitly depend on yq?

Good catch, thanks. I can update the eclass docs accordingly. I count 23
ebuilds across 8 packages, and this can be done without a revbump, so it's not
too bad. Even if it were missing, it would just continue without applying
these extra flags.

I have also noticed that yq will need some keywording first.

I considered some inline Python with just tomli instead, but then you get into
the mess of micro-managing the Python versions. I don't think there's a way to
say "use whatever Python tomli is installed for".

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 858 bytes --]

Re: [gentoo-dev] [PATCH 2/2] cargo.eclass: Preserve project-specific [build] flags by reading config

[Ionen Wolkens]

[-- Attachment #1: Type: text/plain, Size: 2486 bytes --]

On Wed, Aug 07, 2024 at 09:19:36PM +0100, James Le Cuirot wrote:
> On Wed, 2024-08-07 at 19:39 +0200, Michał Górny wrote:
> > On Wed, 2024-08-07 at 16:52 +0100, James Le Cuirot wrote:
> > > The flags we set an a target-specific environment variable override any
> > > generic [build] flags set by the project, requiring ebuilds to set these
> > > themselves, which is undesirable. Work around this by using tomlq to
> > > read the flags from the config files checked by Cargo and prepending
> > > them to our environment variable.
> > > 
> > > Signed-off-by: James Le Cuirot <chewi@gentoo.org>
> > > ---
> > >  eclass/cargo.eclass | 28 +++++++++++++++++-----------
> > >  1 file changed, 17 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/eclass/cargo.eclass b/eclass/cargo.eclass
> > > index 6d4cf1b425eb..dea8c49e4585 100644
> > > --- a/eclass/cargo.eclass
> > > +++ b/eclass/cargo.eclass
> > > @@ -36,7 +36,8 @@ esac
> > >  
> > >  inherit flag-o-matic multiprocessing rust-toolchain toolchain-funcs
> > >  
> > > -[[ ! ${CARGO_OPTIONAL} ]] && BDEPEND="${RUST_DEPEND}"
> > > +# app-misc/yq is needed for tomlq.
> > > +[[ ! ${CARGO_OPTIONAL} ]] && BDEPEND="${RUST_DEPEND} app-misc/yq"
> > >  
> > 
> > Doesn't this imply that all ebuilds using CARGO_OPTIONAL will now have
> > to explicitly depend on yq?
> 
> Good catch, thanks. I can update the eclass docs accordingly. I count 23
> ebuilds across 8 packages, and this can be done without a revbump, so it's not
> too bad. Even if it were missing, it would just continue without applying
> these extra flags.

Probably no reason to not just add it to RUST_DEPEND, some ebuilds
already use that variable with OPTIONAL. Albeit yeah, it's not
documented nor required to use it at the moment.

> 
> I have also noticed that yq will need some keywording first.
> 
> I considered some inline Python with just tomli instead, but then you get into
> the mess of micro-managing the Python versions. I don't think there's a way to
> say "use whatever Python tomli is installed for".

If want to use python, I think simplest would the equivalent of
meson.eclass' dev-build/meson-format-array. Then the ebuild could
handle all the dependencies rather than the eclass.

Could be handy to pull less dependencies, with >=python3.11 can
use the stdlib's tomllib rather than tomli too, be very low footprint.

But is it really worth it over just using yq? I don't know :)
-- 
ionen

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [gentoo-dev] [PATCH 2/2] cargo.eclass: Preserve project-specific [build] flags by reading config

[Michał Górny]

[-- Attachment #1: Type: text/plain, Size: 1770 bytes --]

On Wed, 2024-08-07 at 21:19 +0100, James Le Cuirot wrote:
> On Wed, 2024-08-07 at 19:39 +0200, Michał Górny wrote:
> > On Wed, 2024-08-07 at 16:52 +0100, James Le Cuirot wrote:
> > > The flags we set an a target-specific environment variable override any
> > > generic [build] flags set by the project, requiring ebuilds to set these
> > > themselves, which is undesirable. Work around this by using tomlq to
> > > read the flags from the config files checked by Cargo and prepending
> > > them to our environment variable.
> > > 
> > > Signed-off-by: James Le Cuirot <chewi@gentoo.org>
> > > ---
> > >  eclass/cargo.eclass | 28 +++++++++++++++++-----------
> > >  1 file changed, 17 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/eclass/cargo.eclass b/eclass/cargo.eclass
> > > index 6d4cf1b425eb..dea8c49e4585 100644
> > > --- a/eclass/cargo.eclass
> > > +++ b/eclass/cargo.eclass
> > > @@ -36,7 +36,8 @@ esac
> > >  
> > >  inherit flag-o-matic multiprocessing rust-toolchain toolchain-funcs
> > >  
> > > -[[ ! ${CARGO_OPTIONAL} ]] && BDEPEND="${RUST_DEPEND}"
> > > +# app-misc/yq is needed for tomlq.
> > > +[[ ! ${CARGO_OPTIONAL} ]] && BDEPEND="${RUST_DEPEND} app-misc/yq"
> > >  
> > 
> > Doesn't this imply that all ebuilds using CARGO_OPTIONAL will now have
> > to explicitly depend on yq?
> 
> Good catch, thanks. I can update the eclass docs accordingly. I count 23
> ebuilds across 8 packages, and this can be done without a revbump, so it's not
> too bad. Even if it were missing, it would just continue without applying
> these extra flags.
> 

Still, this sounds like hardcoding implementation details into ebuilds,
and implementation details can change over time.

-- 
Best regards,
Michał Górny


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 512 bytes --]