public inbox for gentoo-dev@lists.gentoo.org
 help / color / mirror / Atom feed
* [gentoo-dev] [PATCH 1/2] distutils-r1.eclass: teach setuptools to respect (some) build options
@ 2023-09-12 19:14 Eli Schwartz
  2023-09-12 19:14 ` [gentoo-dev] [PATCH 2/2] python-utils-r1.eclass: unconditionally warn on occluded packages in cwd Eli Schwartz
  2023-09-12 19:56 ` [gentoo-dev] [PATCH 1/2] distutils-r1.eclass: teach setuptools to respect (some) build options Ulrich Mueller
  0 siblings, 2 replies; 13+ messages in thread
From: Eli Schwartz @ 2023-09-12 19:14 UTC (permalink / raw
  To: gentoo-dev

Previously, setup.py was handled by:

- manually passing makejobs, with a heuristic to guess whether it was a
  time saver to do so.
- rm -rf'ing the build directory in between python versions to prevent
  cross-version contamination

This is because in PEP 517 mode, it doesn't accept build options
specific to a setuptools phase. So a crude hack is to just build_ext
twice, once explicitly and once internally as part of bdist_wheel, and
pray that in the latter case it detects that there's nothing to do.
Unfortunately, sometimes build_ext does NOT detect that there is nothing
to do -- e.g. for codegen tools such as mypyc, that produce *.c files
which are different every time you try building. As for build
directories, those were given up on as hopeless.

There's a better hack which is to set a magic environment variable for a
setup.cfg file which is parsed additionally to the one provided by the
project. It can contain additional settings, such as the build-base and
parallelism, which means that bdist_wheel intrinsically builds
extensions in parallel the only time it is called. And we can set the
output directory for all build artifacts to outside of the source tree,
so it is no longer necessary to delete them (which among other things,
makes debugging difficult).

This is similar to .pydistutils.cfg, but is processed later and can be
in arbitrary locations. Since we store it in the per-impl build
directory we don't need to wipe it after using it to avoid leakage.

Signed-off-by: Eli Schwartz <eschwartz93@gmail.com>
---
 eclass/distutils-r1.eclass | 48 ++++++++------------------------------
 1 file changed, 10 insertions(+), 38 deletions(-)

diff --git a/eclass/distutils-r1.eclass b/eclass/distutils-r1.eclass
index 91de144e1110..dd197a5f0693 100644
--- a/eclass/distutils-r1.eclass
+++ b/eclass/distutils-r1.eclass
@@ -1461,12 +1461,6 @@ distutils_pep517_install() {
 	[[ -n ${wheel} ]] || die "No wheel name returned"
 
 	distutils_wheel_install "${root}" "${WHEEL_BUILD_DIR}/${wheel}"
-
-	# clean the build tree; otherwise we may end up with PyPy3
-	# extensions duplicated into CPython dists
-	if [[ ${DISTUTILS_USE_PEP517:-setuptools} == setuptools ]]; then
-		rm -rf build || die
-	fi
 }
 
 # @FUNCTION: distutils-r1_python_compile
@@ -1478,9 +1472,6 @@ distutils_pep517_install() {
 #
 # If DISTUTILS_USE_PEP517 is set to any other value, builds a wheel
 # using the PEP517 backend and installs it into ${BUILD_DIR}/install.
-# May additionally call build_ext prior to that when using setuptools
-# and the eclass detects a potential benefit from parallel extension
-# builds.
 #
 # In legacy mode, runs 'esetup.py build'. Any parameters passed to this
 # function will be appended to setup.py invocation, i.e. passed
@@ -1495,40 +1486,21 @@ distutils-r1_python_compile() {
 			# call setup.py build when using setuptools (either via PEP517
 			# or in legacy mode)
 
-			if [[ ${DISTUTILS_USE_PEP517} ]]; then
-				if [[ -d build ]]; then
-					eqawarn "A 'build' directory exists already.  Artifacts from this directory may"
-					eqawarn "be picked up by setuptools when building for another interpreter."
-					eqawarn "Please remove this directory prior to building."
-				fi
-			else
-				_distutils-r1_copy_egg_info
-			fi
-
 			# distutils is parallel-capable since py3.5
 			local jobs=$(makeopts_jobs "${MAKEOPTS} ${*}")
 
 			if [[ ${DISTUTILS_USE_PEP517} ]]; then
-				# issue build_ext only if it looks like we have at least
-				# two source files to build; setuptools is expensive
-				# to start and parallel builds can only benefit us if we're
-				# compiling at least two files
-				#
-				# see extension.py for list of suffixes
-				# .pyx is added for Cython
-				#
-				# esetup.py does not respect SYSROOT, so skip it there
-				if [[ -z ${SYSROOT} && ${DISTUTILS_EXT} && 1 -ne ${jobs}
-					&& 2 -eq $(
-						find '(' -name '*.c' -o -name '*.cc' -o -name '*.cpp' \
-							-o -name '*.cxx' -o -name '*.c++' -o -name '*.m' \
-							-o -name '*.mm' -o -name '*.pyx' ')' -printf '\n' |
-							head -n 2 | wc -l
-					)
-				]]; then
-					esetup.py build_ext -j "${jobs}" "${@}"
-				fi
+				mkdir -p "${BUILD_DIR}" || die
+				local -x DIST_EXTRA_CONFIG="${BUILD_DIR}/extra-setup.cfg"
+				cat > "${DIST_EXTRA_CONFIG}" <<-EOF
+					[build]
+					build_base = ${BUILD_DIR}/build
+
+					[build_ext]
+					parallel = ${jobs}
+				EOF
 			else
+				_distutils-r1_copy_egg_info
 				esetup.py build -j "${jobs}" "${@}"
 			fi
 			;;
-- 
2.41.0



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [gentoo-dev] [PATCH 2/2] python-utils-r1.eclass: unconditionally warn on occluded packages in cwd
  2023-09-12 19:14 [gentoo-dev] [PATCH 1/2] distutils-r1.eclass: teach setuptools to respect (some) build options Eli Schwartz
@ 2023-09-12 19:14 ` Eli Schwartz
  2023-09-12 19:56 ` [gentoo-dev] [PATCH 1/2] distutils-r1.eclass: teach setuptools to respect (some) build options Ulrich Mueller
  1 sibling, 0 replies; 13+ messages in thread
From: Eli Schwartz @ 2023-09-12 19:14 UTC (permalink / raw
  To: gentoo-dev; +Cc: Michał Górny

If the current directory masks packages that would be installed and
contains different contents, it can cause testing issues that otherwise
go unnoticed. This warning can stop being experimental and opt-in

Suggested-by: Michał Górny <mgorny@gentoo.org>
Signed-off-by: Eli Schwartz <eschwartz93@gmail.com>
---
 eclass/python-utils-r1.eclass | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/eclass/python-utils-r1.eclass b/eclass/python-utils-r1.eclass
index bd30c1203180..50aeabae1c17 100644
--- a/eclass/python-utils-r1.eclass
+++ b/eclass/python-utils-r1.eclass
@@ -1242,10 +1242,6 @@ _python_check_EPYTHON() {
 _python_check_occluded_packages() {
 	debug-print-function ${FUNCNAME} "${@}"
 
-	# DO NOT ENABLE THIS unless you're going to check for false
-	# positives before filing bugs.
-	[[ ! ${PYTHON_EXPERIMENTAL_QA} ]] && return
-
 	[[ -z ${BUILD_DIR} || ! -d ${BUILD_DIR}/install ]] && return
 
 	local sitedir="${BUILD_DIR}/install$(python_get_sitedir)"
-- 
2.41.0



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [gentoo-dev] [PATCH 1/2] distutils-r1.eclass: teach setuptools to respect (some) build options
  2023-09-12 19:14 [gentoo-dev] [PATCH 1/2] distutils-r1.eclass: teach setuptools to respect (some) build options Eli Schwartz
  2023-09-12 19:14 ` [gentoo-dev] [PATCH 2/2] python-utils-r1.eclass: unconditionally warn on occluded packages in cwd Eli Schwartz
@ 2023-09-12 19:56 ` Ulrich Mueller
  2023-09-13  2:07   ` Eli Schwartz
  1 sibling, 1 reply; 13+ messages in thread
From: Ulrich Mueller @ 2023-09-12 19:56 UTC (permalink / raw
  To: Eli Schwartz; +Cc: gentoo-dev

>>>>> On Tue, 12 Sep 2023, Eli Schwartz wrote:

> +				mkdir -p "${BUILD_DIR}" || die
> +				local -x DIST_EXTRA_CONFIG="${BUILD_DIR}/extra-setup.cfg"
> +				cat > "${DIST_EXTRA_CONFIG}" <<-EOF
> +					[build]
> +					build_base = ${BUILD_DIR}/build
> +
> +					[build_ext]
> +					parallel = ${jobs}
> +				EOF

"|| die" should also be added for the cat command.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [gentoo-dev] [PATCH 1/2] distutils-r1.eclass: teach setuptools to respect (some) build options
  2023-09-12 19:56 ` [gentoo-dev] [PATCH 1/2] distutils-r1.eclass: teach setuptools to respect (some) build options Ulrich Mueller
@ 2023-09-13  2:07   ` Eli Schwartz
  2023-09-13  2:26     ` Michał Górny
  2023-09-13  9:27     ` Ulrich Mueller
  0 siblings, 2 replies; 13+ messages in thread
From: Eli Schwartz @ 2023-09-13  2:07 UTC (permalink / raw
  To: Ulrich Mueller; +Cc: gentoo-dev

On 9/12/23 3:56 PM, Ulrich Mueller wrote:
>>>>>> On Tue, 12 Sep 2023, Eli Schwartz wrote:
> 
>> +				mkdir -p "${BUILD_DIR}" || die
>> +				local -x DIST_EXTRA_CONFIG="${BUILD_DIR}/extra-setup.cfg"
>> +				cat > "${DIST_EXTRA_CONFIG}" <<-EOF
>> +					[build]
>> +					build_base = ${BUILD_DIR}/build
>> +
>> +					[build_ext]
>> +					parallel = ${jobs}
>> +				EOF
> 
> "|| die" should also be added for the cat command.


Redirecting output to a file in a directory you have just guaranteed to
exist cannot fail. (mkdir -p will return nonzero and die, if it exits
without having guaranteed its target is a directory on disk. There is
one loophole, and that is if its target already existed beforehand, but
the mkdir -p process did not have write permissions for it; mkdir will
not check permissions if it detects it doesn't have to do work. This
should not be the case inside the workdir, or something has gone
significantly wrong beforehand.)

That being said, as a style guide concern I can see why being cautious
is generally desirable, as it's much easier to review code that uses it
when it isn't strictly necessary than code that doesn't use it when it
turns out to be necessary.

I'm not very used to this yet :) so I blindly assumed while writing it
that it wasn't necessary to do some additional typing and add this for
call sites that shouldn't be able to fail. I will add this in the next
revision.



-- 
Eli Schwartz



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [gentoo-dev] [PATCH 1/2] distutils-r1.eclass: teach setuptools to respect (some) build options
  2023-09-13  2:07   ` Eli Schwartz
@ 2023-09-13  2:26     ` Michał Górny
  2023-09-13  2:52       ` Eli Schwartz
  2023-09-13  9:27     ` Ulrich Mueller
  1 sibling, 1 reply; 13+ messages in thread
From: Michał Górny @ 2023-09-13  2:26 UTC (permalink / raw
  To: gentoo-dev, Ulrich Mueller

On Tue, 2023-09-12 at 22:07 -0400, Eli Schwartz wrote:
> On 9/12/23 3:56 PM, Ulrich Mueller wrote:
> > > > > > > On Tue, 12 Sep 2023, Eli Schwartz wrote:
> > 
> > > +				mkdir -p "${BUILD_DIR}" || die
> > > +				local -x DIST_EXTRA_CONFIG="${BUILD_DIR}/extra-setup.cfg"
> > > +				cat > "${DIST_EXTRA_CONFIG}" <<-EOF
> > > +					[build]
> > > +					build_base = ${BUILD_DIR}/build
> > > +
> > > +					[build_ext]
> > > +					parallel = ${jobs}
> > > +				EOF
> > 
> > "|| die" should also be added for the cat command.
> 
> 
> Redirecting output to a file in a directory you have just guaranteed to
> exist cannot fail.

Eh, you make me prove you wrong:

# cat > dupa <<-EOF
blahblah
> EOF
cat: write error: No space left on device

-- 
Best regards,
Michał Górny



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [gentoo-dev] [PATCH 1/2] distutils-r1.eclass: teach setuptools to respect (some) build options
  2023-09-13  2:26     ` Michał Górny
@ 2023-09-13  2:52       ` Eli Schwartz
  2023-09-13  3:13         ` Sam James
                           ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Eli Schwartz @ 2023-09-13  2:52 UTC (permalink / raw
  To: gentoo-dev

On 9/12/23 10:26 PM, Michał Górny wrote:
> On Tue, 2023-09-12 at 22:07 -0400, Eli Schwartz wrote:
>> On 9/12/23 3:56 PM, Ulrich Mueller wrote:
>>>>>>>> On Tue, 12 Sep 2023, Eli Schwartz wrote:
>>>
>>>> +				mkdir -p "${BUILD_DIR}" || die
>>>> +				local -x DIST_EXTRA_CONFIG="${BUILD_DIR}/extra-setup.cfg"
>>>> +				cat > "${DIST_EXTRA_CONFIG}" <<-EOF
>>>> +					[build]
>>>> +					build_base = ${BUILD_DIR}/build
>>>> +
>>>> +					[build_ext]
>>>> +					parallel = ${jobs}
>>>> +				EOF
>>>
>>> "|| die" should also be added for the cat command.
>>
>>
>> Redirecting output to a file in a directory you have just guaranteed to
>> exist cannot fail.
> 
> Eh, you make me prove you wrong:
> 
> # cat > dupa <<-EOF
> blahblah
>> EOF
> cat: write error: No space left on device


ಠ_ಠ

Is portage generally expected to successfully complete (including
internal metadata write stages) when its workdir drive runs out of space
partway through?


-- 
Eli Schwartz



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [gentoo-dev] [PATCH 1/2] distutils-r1.eclass: teach setuptools to respect (some) build options
  2023-09-13  2:52       ` Eli Schwartz
@ 2023-09-13  3:13         ` Sam James
  2023-09-13  7:50         ` Alexey Sokolov
  2023-09-13 13:10         ` Michael Orlitzky
  2 siblings, 0 replies; 13+ messages in thread
From: Sam James @ 2023-09-13  3:13 UTC (permalink / raw
  To: gentoo-dev


Eli Schwartz <eschwartz93@gmail.com> writes:

> On 9/12/23 10:26 PM, Michał Górny wrote:
>> On Tue, 2023-09-12 at 22:07 -0400, Eli Schwartz wrote:
>>> On 9/12/23 3:56 PM, Ulrich Mueller wrote:
>>>>>>>>> On Tue, 12 Sep 2023, Eli Schwartz wrote:
>>>>
>>>>> +				mkdir -p "${BUILD_DIR}" || die
>>>>> +				local -x DIST_EXTRA_CONFIG="${BUILD_DIR}/extra-setup.cfg"
>>>>> +				cat > "${DIST_EXTRA_CONFIG}" <<-EOF
>>>>> +					[build]
>>>>> +					build_base = ${BUILD_DIR}/build
>>>>> +
>>>>> +					[build_ext]
>>>>> +					parallel = ${jobs}
>>>>> +				EOF
>>>>
>>>> "|| die" should also be added for the cat command.
>>>
>>>
>>> Redirecting output to a file in a directory you have just guaranteed to
>>> exist cannot fail.
>> 
>> Eh, you make me prove you wrong:
>> 
>> # cat > dupa <<-EOF
>> blahblah
>>> EOF
>> cat: write error: No space left on device
>
>
> ಠ_ಠ
>
> Is portage generally expected to successfully complete (including
> internal metadata write stages) when its workdir drive runs out of space
> partway through?

oh you


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [gentoo-dev] [PATCH 1/2] distutils-r1.eclass: teach setuptools to respect (some) build options
  2023-09-13  2:52       ` Eli Schwartz
  2023-09-13  3:13         ` Sam James
@ 2023-09-13  7:50         ` Alexey Sokolov
  2023-09-13 13:10         ` Michael Orlitzky
  2 siblings, 0 replies; 13+ messages in thread
From: Alexey Sokolov @ 2023-09-13  7:50 UTC (permalink / raw
  To: gentoo-dev

13.09.2023 03:52, Eli Schwartz пишет:
> On 9/12/23 10:26 PM, Michał Górny wrote:
>> On Tue, 2023-09-12 at 22:07 -0400, Eli Schwartz wrote:
>>> On 9/12/23 3:56 PM, Ulrich Mueller wrote:
>>>>>>>>> On Tue, 12 Sep 2023, Eli Schwartz wrote:
>>>>
>>>>> +				mkdir -p "${BUILD_DIR}" || die
>>>>> +				local -x DIST_EXTRA_CONFIG="${BUILD_DIR}/extra-setup.cfg"
>>>>> +				cat > "${DIST_EXTRA_CONFIG}" <<-EOF
>>>>> +					[build]
>>>>> +					build_base = ${BUILD_DIR}/build
>>>>> +
>>>>> +					[build_ext]
>>>>> +					parallel = ${jobs}
>>>>> +				EOF
>>>>
>>>> "|| die" should also be added for the cat command.
>>>
>>>
>>> Redirecting output to a file in a directory you have just guaranteed to
>>> exist cannot fail.
>>
>> Eh, you make me prove you wrong:
>>
>> # cat > dupa <<-EOF
>> blahblah
>>> EOF
>> cat: write error: No space left on device
> 
> 
> ಠ_ಠ
> 
> Is portage generally expected to successfully complete (including
> internal metadata write stages) when its workdir drive runs out of space
> partway through?
> 
> 

This is a race with the user - the user could delete some files from 
disk just in time for metadata to successfully be written

-- 
Best regards,
Alexey "DarthGandalf" Sokolov



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [gentoo-dev] [PATCH 1/2] distutils-r1.eclass: teach setuptools to respect (some) build options
  2023-09-13  2:07   ` Eli Schwartz
  2023-09-13  2:26     ` Michał Górny
@ 2023-09-13  9:27     ` Ulrich Mueller
  2023-09-13 21:06       ` Eli Schwartz
  1 sibling, 1 reply; 13+ messages in thread
From: Ulrich Mueller @ 2023-09-13  9:27 UTC (permalink / raw
  To: Eli Schwartz; +Cc: gentoo-dev

>>>>> On Wed, 13 Sep 2023, Eli Schwartz wrote:

>> "|| die" should also be added for the cat command.

> Redirecting output to a file in a directory you have just guaranteed
> to exist cannot fail.

That's a rather bold statement. I can imagine a number of possible
failures, e.g. no space left on device, quota exceeded, or a low-level
I/O error of the filesystem. Also fork or exec of the cat command could
fail (e.g. out of memory).

While either of these may be unlikely here, best practice is to check
for errors of _all_ external commands.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [gentoo-dev] [PATCH 1/2] distutils-r1.eclass: teach setuptools to respect (some) build options
  2023-09-13  2:52       ` Eli Schwartz
  2023-09-13  3:13         ` Sam James
  2023-09-13  7:50         ` Alexey Sokolov
@ 2023-09-13 13:10         ` Michael Orlitzky
  2023-09-13 21:06           ` Eli Schwartz
  2 siblings, 1 reply; 13+ messages in thread
From: Michael Orlitzky @ 2023-09-13 13:10 UTC (permalink / raw
  To: gentoo-dev

On Tue, 2023-09-12 at 22:52 -0400, Eli Schwartz wrote:
> 
> Is portage generally expected to successfully complete (including
> internal metadata write stages) when its workdir drive runs out of space
> partway through?
> 

No, but I think what everyone else is forgetting to mention is that if
it's going to fail, we want it to fail as soon as possible, i.e. as
close to the thing that actually went wrong. If we proceed past ENOSPC
or whatever, we could get five or six lines further in the ebuild, and
then some other command will fail but possibly with a crazy unrelated
error message.



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [gentoo-dev] [PATCH 1/2] distutils-r1.eclass: teach setuptools to respect (some) build options
  2023-09-13  9:27     ` Ulrich Mueller
@ 2023-09-13 21:06       ` Eli Schwartz
  2023-09-14  8:24         ` Ulrich Mueller
  0 siblings, 1 reply; 13+ messages in thread
From: Eli Schwartz @ 2023-09-13 21:06 UTC (permalink / raw
  To: Ulrich Mueller; +Cc: gentoo-dev

On 9/13/23 5:27 AM, Ulrich Mueller wrote:
>>>>>> On Wed, 13 Sep 2023, Eli Schwartz wrote:
> 
>>> "|| die" should also be added for the cat command.
> 
>> Redirecting output to a file in a directory you have just guaranteed
>> to exist cannot fail.
> 
> That's a rather bold statement. I can imagine a number of possible
> failures, e.g. no space left on device, quota exceeded, or a low-level
> I/O error of the filesystem. Also fork or exec of the cat command could
> fail (e.g. out of memory).
> 
> While either of these may be unlikely here, best practice is to check
> for errors of _all_ external commands.


The implementation of `die` performs a fork+exec of the sed command,
invokes eerror (many times!) which forks to pipe, invokes $() which
forks, and could behave abnormally due to out of memory. It is not safe
to treat `|| die` as error recovery for an out of memory condition.

The implementation of `die` performs multiple writes to files inside of
${PORTAGE_BUILDDIR}, and could behave abnormally due to write failures.
It is not safe to treat `|| die` as error recovery for a write failure
of the ${PORTAGE_BUILDDIR} drive (whatever the reason for that write
failure).

Regarding:

- no space left on device
- quota exceeded
- low-level I/O error of the filesystem

this isn't "a number of possible failures" :) it is the same failure but
elicited by different prompts. Regarding this, I have already commented...

(And the `|| die` is added to the next revision of this patch either way.)


-- 
Eli Schwartz



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [gentoo-dev] [PATCH 1/2] distutils-r1.eclass: teach setuptools to respect (some) build options
  2023-09-13 13:10         ` Michael Orlitzky
@ 2023-09-13 21:06           ` Eli Schwartz
  0 siblings, 0 replies; 13+ messages in thread
From: Eli Schwartz @ 2023-09-13 21:06 UTC (permalink / raw
  To: gentoo-dev, Michael Orlitzky

On 9/13/23 9:10 AM, Michael Orlitzky wrote:
> On Tue, 2023-09-12 at 22:52 -0400, Eli Schwartz wrote:
>>
>> Is portage generally expected to successfully complete (including
>> internal metadata write stages) when its workdir drive runs out of space
>> partway through?
>>
> 
> No, but I think what everyone else is forgetting to mention is that if
> it's going to fail, we want it to fail as soon as possible, i.e. as
> close to the thing that actually went wrong. If we proceed past ENOSPC
> or whatever, we could get five or six lines further in the ebuild, and
> then some other command will fail but possibly with a crazy unrelated
> error message.


The consequences of this particular command failing are that:

- setuptools will still build, but without spawning a ThreadPoolExecutor
  to distribute jobs; it builds single threaded

- setuptools will still build, but using its default build directory
  instead of an alternative one; setuptools itself *cannot* fail as a
  result, but other external commands hardcoding the location of
  setuptools internal files could fail to find those files


(It will still fail when setuptools reports its own ENOSPC, but this is
not a crazy unrelated error message.)

(Unless portage expects to successfully complete where possible if the
workdir drive runs out of space partway through, then quickly has more
space freed up for it as soon as monitoring warnings are triggered and
no actually-fatal ebuild errors occurred during the window of ENOSPC.)




-- 
Eli Schwartz



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [gentoo-dev] [PATCH 1/2] distutils-r1.eclass: teach setuptools to respect (some) build options
  2023-09-13 21:06       ` Eli Schwartz
@ 2023-09-14  8:24         ` Ulrich Mueller
  0 siblings, 0 replies; 13+ messages in thread
From: Ulrich Mueller @ 2023-09-14  8:24 UTC (permalink / raw
  To: Eli Schwartz; +Cc: gentoo-dev

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

>>>>> On Wed, 13 Sep 2023, Eli Schwartz wrote:

>> That's a rather bold statement. I can imagine a number of possible
>> failures, e.g. no space left on device, quota exceeded, or a low-level
>> I/O error of the filesystem. Also fork or exec of the cat command could
>> fail (e.g. out of memory).
>> 
>> While either of these may be unlikely here, best practice is to check
>> for errors of _all_ external commands.

> The implementation of `die` performs a fork+exec of the sed command,
> invokes eerror (many times!) which forks to pipe, invokes $() which
> forks, and could behave abnormally due to out of memory. It is not safe
> to treat `|| die` as error recovery for an out of memory condition.

> The implementation of `die` performs multiple writes to files inside of
> ${PORTAGE_BUILDDIR}, and could behave abnormally due to write failures.
> It is not safe to treat `|| die` as error recovery for a write failure
> of the ${PORTAGE_BUILDDIR} drive (whatever the reason for that write
> failure).

An eclass must not rely on implementation details of any specific
package manager.

"die [...] aborts the build process."
https://projects.gentoo.org/pms/8/pms.html#x1-12600012.3.6

So please report a Portage (or other package manager) bug if you can
reproduce a condition where die doesn't abort the build process.

> (And the `|| die` is added to the next revision of this patch either way.)

Thanks.

Ulrich

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2023-09-14  8:24 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-12 19:14 [gentoo-dev] [PATCH 1/2] distutils-r1.eclass: teach setuptools to respect (some) build options Eli Schwartz
2023-09-12 19:14 ` [gentoo-dev] [PATCH 2/2] python-utils-r1.eclass: unconditionally warn on occluded packages in cwd Eli Schwartz
2023-09-12 19:56 ` [gentoo-dev] [PATCH 1/2] distutils-r1.eclass: teach setuptools to respect (some) build options Ulrich Mueller
2023-09-13  2:07   ` Eli Schwartz
2023-09-13  2:26     ` Michał Górny
2023-09-13  2:52       ` Eli Schwartz
2023-09-13  3:13         ` Sam James
2023-09-13  7:50         ` Alexey Sokolov
2023-09-13 13:10         ` Michael Orlitzky
2023-09-13 21:06           ` Eli Schwartz
2023-09-13  9:27     ` Ulrich Mueller
2023-09-13 21:06       ` Eli Schwartz
2023-09-14  8:24         ` Ulrich Mueller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox