Re: [gentoo-dev] [PATCH 3/3] distutils-r1.eclass: fix src_configure to handle flag-o-matic correctly

["Michał Górny" <mgorny@gentoo.org>]

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

On Mon, 2024-02-19 at 23:26 -0500, Eli Schwartz wrote:
> Use of python_configure_all is a bit broken, because distutils-r1 is a
> bit broken. It has the intriguing value-claim that *FLAGS shall be
> modified with local -x as part of run-phase, which means that all
> modifications are reset as soon as any given phase exits. Including
> sub-phases. If you make changes in python_configure or
> python_configure_all, as you are expected to do instead of defining
> src_configure and then running the eclass phases manually, your changes
> inherently get erased before you actually *get* to running builds (i.e.
> reading the variables).
> 
> For an example of how this works, consider dev-python/scipy. It builds
> with filter-lto, for the standard reasons. However, filter-lto gets run
> inside python_configure_all, so it gets erased and ignored.
> 
> Note: it "worked" anyways prior to reworking meson.eclass to pass
> -Db_lto based on `is-flagq flto`. This is because filter-lto removed it
> from LDFLAGS, and since distutils-r1 doesn't localize that variable, the
> changes stuck around. As a result:
> 
> - the previous fix to scipy caused scipy to be compiled with LTO, but
>   not linked with LTO
> - meson.eclass changes caused scipy to be built with -Db_lto=true, which
>   affects both compiling and linking
> 
> It's absolutely unsafe to have flag-o-matic be ignored like this, and it
> is fascinating to end up with LTO restored into compile flags while
> still being filtered from LDFLAGS... simply as a side effect of
> distutils-r1 not modifying the latter.
> 
> - this patch causes scipy to be built with -Db_lto=false
> 
> Consequences of the change make no difference to standard dev-python/
> packages, but affect packages that use both distutils-r1 and other
> packaging infrastructure:
> 
> - global variables for tools are exported. This is supposed to be a
>   safe, albeit unnecessary for better-than-setuptools build systems,
>   option.
> 
> - the "debug" option applies the DEBUG preprocessor macro to more than
>   just python code. This was already the case for python projects that
>   built non-CPython API C/C++ code and then linked them with thin python
>   wrappers, so projects with python bindings shouldn't be any different.
>   Also, it is a "debug" use flag so it's pretty silly if it only toggles
>   debug on python bindings but not the rest of the package, so just...
>   deal with it I guess?
> 
> - any project that uses cython, now ignores the Modern C Porting flag
>   "incompatible-pointer-types". This is terrible, because code which
>   should trigger that error is terrible. But it's also terrible for
>   python projects with mixed Cython and handwritten C, to squelch that
>   error just because one file happens to use Cython. Yet, we do it
>   because we don't have a way to make extremely specific files built
>   with different CFLAGS compared to the rest of the project. There's no
>   actual reason to treat handwritten C python modules different from
>   non-distutils phases.
> 
> Signed-off-by: Eli Schwartz <eschwartz93@gmail.com>
> ---
>  eclass/distutils-r1.eclass | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/eclass/distutils-r1.eclass b/eclass/distutils-r1.eclass
> index 35825d4c3aa6..873421bbcee8 100644
> --- a/eclass/distutils-r1.eclass
> +++ b/eclass/distutils-r1.eclass
> @@ -1813,16 +1813,15 @@ distutils-r1_run_phase() {
>  	fi
>  
>  	# Set up build environment, bug #513664.
> -	local -x AR=${AR} CC=${CC} CPP=${CPP} CXX=${CXX}
>  	tc-export AR CC CPP CXX

Sigh.  While I see your point, this feels like simultaneously breaking
and half-assed change.  If we were to remove it, I'd rather move
the logic somewhere where it is actually called once.

>  
>  	if [[ ${DISTUTILS_EXT} ]]; then
>  		if [[ ${BDEPEND} == *dev-python/cython* ]] ; then
>  			# Workaround for https://github.com/cython/cython/issues/2747 (bug #918983)
> -			local -x CFLAGS="${CFLAGS} $(test-flags-CC -Wno-error=incompatible-pointer-types)"
> +			append-cflags $(test-flags-CC -Wno-error=incompatible-pointer-types)

Sounds like it will be appended multiple times.

>  		fi
>  
> -		local -x CPPFLAGS="${CPPFLAGS} $(usex debug '-UNDEBUG' '-DNDEBUG')"
> +		append-cppflags $(usex debug '-UNDEBUG' '-DNDEBUG')
>  		# always generate .c files from .pyx files to ensure we get latest
>  		# bug fixes from Cython (this works only when setup.py is using
>  		# cythonize() but it's better than nothing)

Likewise.

-- 
Best regards,
Michał Górny


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