Ban 'assert' and introduce 'pipestatus' instead in EAPI 9

[Flowdalic]

Starting with EAPI 9, we decided to phase out 'assert', because, for example, its name is confusing. Instead 'assert' is going to be replaced with 'pipestatus'.

This also introduces the portage internal __pipestatus helper that can be used in bash code for all EAPIs.

Bug: https://bugs.gentoo.org/566342

[ulm]

There should be separate commits for assert and pipestatus as well as separate feature test functions.

[Flowdalic]

There should be separate commits for assert and pipestatus as well as separate feature test functions.

I feel like banning assert and introducing pipestatus should be one atomic change. And I feel similar about the feature test functions. One could argue that the feature test function could be renamed to __eapi_has_pipestatus_and_assert_banned for clarity. How about that?

Unrelated to your comment above. How do you (and others) feel about

pipestatus() {
    __pipestatus $@
}

vs

alias pipestatus=__pipestatus

I considered using an alias for that, as it felt cleaner and clearly expresses the intent. But maybe there are downsides I am not aware of?

[ulm]

There should be separate commits for assert and pipestatus as well as separate feature test functions.

I feel like banning assert and introducing pipestatus should be one atomic change. And I feel similar about the feature test functions. One could argue that the feature test function could be renamed to __eapi_has_pipestatus_and_assert_banned for clarity. How about that?

This is definitely not one atomic change. Read the log of the 2024-12-08 council meeting where several council members asked for the assert ban to be revisited before the final approval of EAPI 9. Your commit would create a fait accompli preventing this. So please split it and also make them separate feature tests. Plus, these are separate features in PMS.

Unrelated to your comment above. How do you (and others) feel about

pipestatus() {
    __pipestatus $@
}

vs

alias pipestatus=__pipestatus

I considered using an alias for that, as it felt cleaner and clearly expresses the intent. But maybe there are downsides I am not aware of?

That would be the only alias in the Portage code and it feels brittle. As a matter of fact, all previously defined aliases (with assert being one of them) were removed or replaced by a function.

Anyway, the pipestatus function should use "$@" rather than $@ (sorry that I missed that before):

pipestatus() {
    __pipestatus "$@"
}

[ulm]

Two more comments:

1. Why do you define the functions in a separate eapi9-pipestatus.sh file instead of isolated-function.sh? It seems unnecessary.
2. Please don't define pipestatus and then unset it again in EAPIs that don't have the function. unset -f pipestatus is just horrible.

[Flowdalic]

1. Why do you define the functions in a separate eapi9-pipestatus.sh file instead of isolated-function.sh? It seems unnecessary.

Having the code isolated in a single file makes testing easier. It also follows the pattern of eapi7-ver-funcs.sh.

[ulm]

I wonder if this should be simplified and use __pipestatus internally?

Suggested change

	assert() {
	local x pipestatus=${PIPESTATUS[*]}
	for x in ${pipestatus} ; do
	[[ ${x} -eq 0 ]] || die "$@"
	done
	}
	assert() {
	__pipestatus || die "$@"
	}

[Flowdalic]

Sounds good to me, but let's hear what others think.

[ulm]

1. Why do you define the functions in a separate eapi9-pipestatus.sh file instead of isolated-function.sh? It seems unnecessary.

Having the code isolated in a single file makes testing easier. It also follows the pattern of eapi7-ver-funcs.sh.

Yes, but then follow its pattern entirely and source the file only when ___eapi_has_pipestatus is true. IMHO it makes no sense to split out a single function to its own file which is then unconditionally sourced. It is less efficient and at the same time makes maintenance more difficult (because __pipestatus and pipestatus would be defined in different places).

[Flowdalic]

Yes, but then follow its pattern entirely and source the file only when ___eapi_has_pipestatus is true. IMHO it makes no sense to split out a single function to its own file which is then unconditionally sourced.

Sourcing the file conditionally has disadvantages, as it's __pipestatus is used internally by portage as EAPI agnostic assert/pipestatus. If it would only be available in certain EAPIs, then we would need to guard every portage-internal use of assert/pipestatus with an EAPI condition.

And again, __pipestatus() resides in its own file to facilitate its unit testing.

[ulm]

Sourcing the file conditionally has disadvantages, as it's __pipestatus is used internally by portage as EAPI agnostic assert/pipestatus. If it would only be available in certain EAPIs, then we would need to guard every portage-internal use of assert/pipestatus with an EAPI condition.

And again, __pipestatus() resides in its own file to facilitate its unit testing.

Obviously we disagree here. So, I guess it is for the Portage maintainers to decide whether a function with 11 SLOC should live in its own file.