[gentoo-dev] [PATCH] versionator.eclass: Add tests for parameter counts

[gentoo-dev] [PATCH] versionator.eclass: Add tests for parameter counts

[Michał Górny]

Ensure that proper number of parameters is passed to each versionator
function; die otherwise. This prevents the functions from proceeding
with undefined behavior when mis-called. However, it does not cover the
most common mistake of passing an empty version that implicitly gets
replaced by ${PV}.
---
 eclass/versionator.eclass | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/eclass/versionator.eclass b/eclass/versionator.eclass
index 74e676ee..e42fc4d 100644
--- a/eclass/versionator.eclass
+++ b/eclass/versionator.eclass
@@ -1,4 +1,4 @@
-# Copyright 1999-2014 Gentoo Foundation
+# Copyright 1999-2016 Gentoo Foundation
 # Distributed under the terms of the GNU General Public License v2
 # $Id$
 
@@ -46,6 +46,8 @@ get_all_version_components() {
 	local ver_str=${1:-${PV}} result
 	result=()
 
+	[[ $# -le 1 ]] || die "${FUNCNAME}: too many parameters ($#)"
+
 	# sneaky cache trick cache to avoid having to parse the same thing several
 	# times.
 	if [[ ${VERSIONATOR_CACHE_VER_STR} == ${ver_str} ]] ; then
@@ -101,6 +103,8 @@ get_all_version_components() {
 #     20040905    ->  20040905
 #     3.0c-r1     ->  3 0 c r1
 get_version_components() {
+	[[ $# -le 1 ]] || die "${FUNCNAME}: too many parameters ($#)"
+
 	local c=$(get_all_version_components "${1:-${PV}}")
 	echo ${c//[-._]/ }
 }
@@ -115,6 +119,8 @@ get_version_components() {
 #     20040905    ->  20040905
 #     3.0c-r1     ->  3
 get_major_version() {
+	[[ $# -le 1 ]] || die "${FUNCNAME}: too many parameters ($#)"
+
 	local c=($(get_all_version_components "${1:-${PV}}"))
 	echo ${c[0]}
 }
@@ -128,6 +134,9 @@ get_major_version() {
 #    1-2    1.2.3       -> 1.2
 #    2-     1.2.3       -> 2.3
 get_version_component_range() {
+	[[ $# -ge 1 ]] || die "${FUNCNAME}: no range provided"
+	[[ $# -le 2 ]] || die "${FUNCNAME}: too many parameters ($#)"
+
 	eshopts_push -s extglob
 	local c v="${2:-${PV}}" range="${1}" range_start range_end
 	local -i i=-1 j=0
@@ -161,6 +170,8 @@ get_version_component_range() {
 #     20040905    ->  (empty string)
 #     3.0c-r1     ->  0c-r1
 get_after_major_version() {
+	[[ $# -le 1 ]] || die "${FUNCNAME}: too many parameters ($#)"
+
 	echo $(get_version_component_range 2- "${1:-${PV}}")
 }
 
@@ -175,6 +186,9 @@ get_after_major_version() {
 # Rather than being a number, $1 can be a separator character such as '-', '.'
 # or '_'. In this case, the first separator of this kind is selected.
 replace_version_separator() {
+	[[ $# -ge 2 ]] || die "${FUNCNAME}: required parameters missing"
+	[[ $# -le 3 ]] || die "${FUNCNAME}: too many parameters ($#)"
+
 	eshopts_push -s extglob
 	local w c v="${3:-${PV}}"
 	declare -i i found=0
@@ -210,6 +224,9 @@ replace_version_separator() {
 # Replace all version separators in $2 (defaults to $PV) with $1.
 #     '_' 1b.2.3        -> 1b_2_3
 replace_all_version_separators() {
+	[[ $# -ge 1 ]] || die "${FUNCNAME}: no replacement provided"
+	[[ $# -le 2 ]] || die "${FUNCNAME}: too many parameters ($#)"
+
 	local c=($(get_all_version_components "${2:-${PV}}"))
 	c=${c[@]//[-._]/$1}
 	echo ${c// }
@@ -226,6 +243,9 @@ replace_all_version_separators() {
 # Rather than being a number, $1 can be a separator character such as '-', '.'
 # or '_'. In this case, the first separator of this kind is deleted.
 delete_version_separator() {
+	[[ $# -ge 1 ]] || die "${FUNCNAME}: no index provided"
+	[[ $# -le 2 ]] || die "${FUNCNAME}: too many parameters ($#)"
+
 	replace_version_separator "${1}" "" "${2}"
 }
 
@@ -235,6 +255,8 @@ delete_version_separator() {
 # Delete all version separators in $1 (defaults to $PV).
 #     1b.2.3        -> 1b23
 delete_all_version_separators() {
+	[[ $# -le 1 ]] || die "${FUNCNAME}: too many parameters ($#)"
+
 	replace_all_version_separators "" "${1}"
 }
 
@@ -245,6 +267,8 @@ delete_all_version_separators() {
 #     1.0.1       ->  3
 #     3.0c-r1     ->  4
 get_version_component_count() {
+	[[ $# -le 1 ]] || die "${FUNCNAME}: too many parameters ($#)"
+
 	local a=($(get_version_components "${1:-${PV}}"))
 	echo ${#a[@]}
 }
@@ -257,6 +281,8 @@ get_version_component_count() {
 #     1.0.1       ->  2
 #     3.0c-r1     ->  3
 get_last_version_component_index() {
+	[[ $# -le 1 ]] || die "${FUNCNAME}: too many parameters ($#)"
+
 	echo $(($(get_version_component_count "${1:-${PV}}" ) - 1))
 }
 
@@ -267,6 +293,9 @@ get_last_version_component_index() {
 # only. May not be reliable, be sure to do very careful testing before actually
 # using this.
 version_is_at_least() {
+	[[ $# -ge 1 ]] || die "${FUNCNAME}: wanted version not provided"
+	[[ $# -le 2 ]] || die "${FUNCNAME}: too many parameters ($#)"
+
 	local want_s="$1" have_s="${2:-${PVR}}" r
 	version_compare "${want_s}" "${have_s}"
 	r=$?
@@ -291,6 +320,9 @@ version_is_at_least() {
 # return 3. You probably want version_is_at_least rather than this function.
 # May not be very reliable. Test carefully before using this.
 version_compare() {
+	[[ $# -ge 2 ]] || die "${FUNCNAME}: versions not provided"
+	[[ $# -le 2 ]] || die "${FUNCNAME}: too many parameters ($#)"
+
 	eshopts_push -s extglob
 	local ver_a=${1} ver_b=${2} parts_a parts_b
 	local cur_tok_a cur_tok_b num_part_a num_part_b
@@ -468,6 +500,8 @@ version_compare() {
 # algorithm for simplicity, so don't call it with more than a few dozen items.
 # Uses version_compare, so be careful.
 version_sort() {
+	[[ $# -ge 1 ]] || die "${FUNCNAME}: version not provided"
+
 	eshopts_push -s extglob
 	local items=
 	local -i left=0
@@ -501,6 +535,9 @@ version_sort() {
 # MY_P=$(version_format_string '${PN}_source_$1_$2-$3_$4')
 # Now MY_P will be: cow-hat_source_1_2-3_p4
 version_format_string() {
+	[[ $# -ge 1 ]] || die "${FUNCNAME}: no format provided"
+	[[ $# -le 2 ]] || die "${FUNCNAME}: too many parameters ($#)"
+
 	local fstr=$1
 	shift
 	set -- $(get_version_components "$@")
-- 
2.9.2

Re: [gentoo-dev] [PATCH] versionator.eclass: Add tests for parameter counts

[Chí-Thanh Christopher Nguyễn]

Michał Górny schrieb:
> Ensure that proper number of parameters is passed to each versionator
> function; die otherwise. This prevents the functions from proceeding
> with undefined behavior when mis-called.

You are making what versionator.eclass accepts stricter. That it used to work 
when passed multiple versions may be unintentional, but I think you need to 
introduce a new eclass or new function names in this case.


Best regards,
Chí-Thanh Christopher Nguyễn

Re: [gentoo-dev] [PATCH] versionator.eclass: Add tests for parameter counts

[Michał Górny]

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

On Sun, 24 Jul 2016 00:17:21 +0200
Chí-Thanh Christopher Nguyễn <chithanh@gentoo.org> wrote:

> Michał Górny schrieb:
> > Ensure that proper number of parameters is passed to each versionator
> > function; die otherwise. This prevents the functions from proceeding
> > with undefined behavior when mis-called.  
> 
> You are making what versionator.eclass accepts stricter. That it used to work 
> when passed multiple versions may be unintentional, but I think you need to 
> introduce a new eclass or new function names in this case.

So, to summarize we shouldn't fix existing code because people did
assume accepting invalid parameters was fine.

What we should do instead is create an almost identical copy of it, ask
people to switch to new code with parameter checks. But... why?

If people already pass valid parameters, then new and old code would be
functionally identical. If they do not, then they can fix parameters
and stay with the old code. But they shouldn't have been doing that in
the first place...

So in the end, we create another copy of the functions that's used
inconsistently with the old copy, and only when people feel like
switching. Then ebuilds will fail just the same because people wouldn't
care to *ensure* they pass valid parameters while switching, as long as
they won't trigger it on their limited test case...

-- 
Best regards,
Michał Górny
<http://dev.gentoo.org/~mgorny/>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 949 bytes --]

Re: [gentoo-dev] [PATCH] versionator.eclass: Add tests for parameter counts

[Chí-Thanh Christopher Nguyễn]

Michał Górny schrieb:
> So, to summarize we shouldn't fix existing code because people did
> assume accepting invalid parameters was fine.

You are changing the API of versionator.eclass, even if it was unintentional API.

> Then ebuilds will fail just the same

No. Before, ebuilds would maybe display an upgrading message when they 
shouldn't, or vice versa. Now the eclass dies on them.


Best regards,
Chí-Thanh Christopher Nguyễn

Re: [gentoo-dev] [PATCH] versionator.eclass: Add tests for parameter counts

[Ciaran McCreesh]

On Sun, 24 Jul 2016 10:17:24 +0200
Chí-Thanh Christopher Nguyễn <chithanh@gentoo.org> wrote:
> > Then ebuilds will fail just the same  
> 
> No. Before, ebuilds would maybe display an upgrading message when
> they shouldn't, or vice versa. Now the eclass dies on them.

This attitude that invisible failures (which don't get fixed, and which
lead to occasional weirdness) are better than visible failures (which
must be fixed) is an odd one... Postel has a lot to answer for.

-- 
Ciaran McCreesh

Re: [gentoo-dev] [PATCH] versionator.eclass: Add tests for parameter counts

[Chí-Thanh Christopher Nguyễn]

Ciaran McCreesh schrieb:
> On Sun, 24 Jul 2016 10:17:24 +0200
> Chí-Thanh Christopher Nguyễn <chithanh@gentoo.org> wrote:
>>> Then ebuilds will fail just the same
>>
>> No. Before, ebuilds would maybe display an upgrading message when
>> they shouldn't, or vice versa. Now the eclass dies on them.
>
> This attitude that invisible failures (which don't get fixed, and which
> lead to occasional weirdness) are better than visible failures (which
> must be fixed) is an odd one... Postel has a lot to answer for.

I would agree with you when it comes to upstreams' -Werror but I do realize 
that I am in the minority there.

My point here is that this change will affect the behaviour of ebuilds using 
this eclass.


Best regards,
Chí-Thanh Christopher Nguyễn

Re: [gentoo-dev] [PATCH] versionator.eclass: Add tests for parameter counts

[Jason Zaman]

On Sun, Jul 24, 2016 at 10:17:24AM +0200, Chí-Thanh Christopher Nguyễn wrote:
> Michał Górny schrieb:
> > So, to summarize we shouldn't fix existing code because people did
> > assume accepting invalid parameters was fine.
> 
> You are changing the API of versionator.eclass, even if it was unintentional API.

# @FUNCTION: version_is_at_least
# @USAGE: <want> [have]
# @DESCRIPTION:
# Is $2 (defaults to $PVR) at least version $1? Intended for use in eclasses
# only. May not be reliable, be sure to do very careful testing before actually
# using this.

The API seems pretty clear to me ...

-- Jason

Re: [gentoo-dev] [PATCH] versionator.eclass: Add tests for parameter counts

[Michał Górny]

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

On Sun, 24 Jul 2016 10:17:24 +0200
Chí-Thanh Christopher Nguyễn <chithanh@gentoo.org> wrote:

> Michał Górny schrieb:
> > So, to summarize we shouldn't fix existing code because people did
> > assume accepting invalid parameters was fine.  
> 
> You are changing the API of versionator.eclass, even if it was unintentional API.

There is no such thing as 'unintentional API'. API is what's described.
If you rely on internals, random undefined behaviors or whatever, it's
not a part of the API.

Yes, I know that most of Gentoo developers are bright enough to assume
'if it is not explicitly forbidden in words I can't find a way to bend
to pretend it's allowed anyway, it's allowed even if it doesn't make
any sense'. But that's a social problem that could easily solved by more
proactive retirements, and not a good API design point.

> > Then ebuilds will fail just the same  
> 
> No. Before, ebuilds would maybe display an upgrading message when they 
> shouldn't, or vice versa. Now the eclass dies on them.

So what's the alternative? Design another eclass where ebuilds will
fail just the same because people will ignore the more explicit
requirement just the same as they do ignore the API?

The problem is not 'there is a valid case to pass useless parameters
to the function'. The problem is 'I make an invalid assumption about
what will happen based on my limited knowledge which I believe is
more correct than people who wrote package managers'.

-- 
Best regards,
Michał Górny
<http://dev.gentoo.org/~mgorny/>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 949 bytes --]

Re: [gentoo-dev] [PATCH] versionator.eclass: Add tests for parameter counts

[Chí-Thanh Christopher Nguyễn]

Michał Górny schrieb:
>> You are changing the API of versionator.eclass, even if it was unintentional API.
>
> There is no such thing as 'unintentional API'. API is what's described.
> If you rely on internals, random undefined behaviors or whatever, it's
> not a part of the API.

Well there is the API according to the docs, and then there is the API 
according to the implementation...

>>> Then ebuilds will fail just the same
>>
>> No. Before, ebuilds would maybe display an upgrading message when they
>> shouldn't, or vice versa. Now the eclass dies on them.
>
> So what's the alternative? Design another eclass where ebuilds will
> fail just the same because people will ignore the more explicit
> requirement just the same as they do ignore the API?

I don't know what is your problem here. The eclass needs not be designed 
again from the ground up.
All ebuilds which are unaffected by bug 589444 can be switched to the new 
eclass/functions immediately. The others after they are changed no longer 
make the implicit assumption that REPLACING_VERSIONS contains only a single 
version.

> The problem is not 'there is a valid case to pass useless parameters
> to the function'. The problem is 'I make an invalid assumption about
> what will happen based on my limited knowledge which I believe is
> more correct than people who wrote package managers'.

I don't say it is a correct use of versionator.eclass. I just say it has 
become (unintentionally) part of the API, and therefore is subject to the 
rules about changing APIs in eclasses. I agree that relying on unintentional 
or undocumented API is bad and needs to be addressed.


Best regards,
Chí-Thanh Christopher Nguyễn

Re: [gentoo-dev] [PATCH] versionator.eclass: Add tests for parameter counts

[Jeroen Roovers]

On Sun, 24 Jul 2016 12:17:56 +0200
Michał Górny <mgorny@gentoo.org> wrote:

> But that's a social problem that could easily solved by more
> proactive retirements, and not a good API design point.

You need some trained and payed PR people to correct your writings
before they go to press or you will never be elected president.


     jer

Re: [gentoo-dev] [PATCH] versionator.eclass: Add tests for parameter counts

[Ulrich Mueller]

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

>>>>> On Sun, 24 Jul 2016, Michał Górny wrote:

> There is no such thing as 'unintentional API'. API is what's
> described. If you rely on internals, random undefined behaviors
> or whatever, it's not a part of the API.

Like memcpy(3) direction?

Ulrich

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: [gentoo-dev] [PATCH] versionator.eclass: Add tests for parameter counts

[Chí-Thanh Christopher Nguyễn]

Chí-Thanh Christopher Nguyễn schrieb:
> I don't say it is a correct use of versionator.eclass. I just say it has
> become (unintentionally) part of the API, and therefore is subject to the
> rules about changing APIs in eclasses.

Actually, after reading those rules[1] again, it would be enough to fix all 
ebuilds in the tree and wait 30 days to be in compliance with eclass policy.

So from my side I will no longer object to your proposed change, provided 
these conditions are met.


Best regards,
Chí-Thanh Christopher Nguyễn

[1] https://devmanual.gentoo.org/eclass-writing/

Re: [gentoo-dev] [PATCH] versionator.eclass: Add tests for parameter counts

[Alexis Ballier]

On Sun, 24 Jul 2016 10:17:24 +0200
Chí-Thanh Christopher Nguyễn <chithanh@gentoo.org> wrote:
> > Then ebuilds will fail just the same  
> 
> No. Before, ebuilds would maybe display an upgrading message when
> they shouldn't, or vice versa. Now the eclass dies on them.


As a side note: die-ing in anything after merging the package
(>=pkg_postinst) is a seriously bad idea.

Re: [gentoo-dev] [PATCH] versionator.eclass: Add tests for parameter counts

[Kent Fredric]

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

On Sun, 24 Jul 2016 12:17:56 +0200
Michał Górny <mgorny@gentoo.org> wrote:

> So what's the alternative? Design another eclass where ebuilds will
> fail just the same because people will ignore the more explicit
> requirement just the same as they do ignore the API?

Sometimes you don't need a "new path", you just need to transition from
the old behaviour to the new behaviour at a slower rate with more
visibility.

Option 1: Start off with them being QA Warnings instead of fatal errors
and encourage end users to report them where they see them.

Then after a cycle of warnings, you go through and fatalise them
incrementally in order of "least likely to break the build in dangerous
ways". 

Option 2: Bind the behaviour to an EAPI version that is not yet in use.

Then, when that EAPI gets rolled out to the ebuilds getting upgrades,
the strictures come into effect when the EAPI changes, giving
maintainers fair opportunity to fix the problem before it rolls out to
users.

For me neither of these options say "don't do this thing", they're just
"manage the bleeding better"

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]