[gentoo-dev] RFC: eutils.eclass: More reliable return status for e*_clean.

[gentoo-dev] RFC: eutils.eclass: More reliable return status for e*_clean.

[Ulrich Mueller]

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

Currently the return status of e{cvs,svn,git}_clean is not at all
reliable, because only the last command in the pipeline is tested.
In addition, there are two pipelines in ecvs_clean, and the exit
status of the first one is completely ignored.

Another issue is that xargs -0 is an extension not specified by POSIX.
See patch included below, fixing both problems. (Note that this is
close to the implementation originally proposed in bug 210708 [1].)

Should we take this as an opportunity to split off these three
functions into their own eclass, e.g. vcs-clean.eclass?

Ulrich

[1] https://bugs.gentoo.org/210708


From 83c0afbfb4ef0c501c65dcd8e0c9c8bd30cdd70a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ulrich=20M=C3=BCller?= <ulm@gentoo.org>
Date: Fri, 16 Feb 2018 07:47:46 +0100
Subject: [PATCH] eutils.eclass: More reliable return status for e*_clean.

In ecvs_clean, combine the two find commands into one, so that the
exit status of the first one won't be ignored.

Also use find -exec rather then find | xargs, so we don't have to
check the exit status of all commands in the pipeline.
---
 eclass/eutils.eclass | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/eclass/eutils.eclass b/eclass/eutils.eclass
index 8bbd561015ad..bce8cf1c2610 100644
--- a/eclass/eutils.eclass
+++ b/eclass/eutils.eclass
@@ -44,8 +44,8 @@ fi
 # internal CVS directories.  Defaults to $PWD.
 ecvs_clean() {
 	[[ $# -eq 0 ]] && set -- .
-	find "$@" -type d -name 'CVS' -prune -print0 | xargs -0 rm -rf
-	find "$@" -type f -name '.cvs*' -print0 | xargs -0 rm -rf
+	find "$@" \( -type d -name 'CVS' -prune -o -type f -name '.cvs*' \) \
+		-exec rm -rf '{}' \+
 }
 
 # @FUNCTION: esvn_clean
@@ -55,7 +55,7 @@ ecvs_clean() {
 # internal Subversion directories.  Defaults to $PWD.
 esvn_clean() {
 	[[ $# -eq 0 ]] && set -- .
-	find "$@" -type d -name '.svn' -prune -print0 | xargs -0 rm -rf
+	find "$@" -type d -name '.svn' -prune -exec rm -rf '{}' \+
 }
 
 # @FUNCTION: egit_clean
@@ -65,7 +65,7 @@ esvn_clean() {
 # contains internal Git directories.  Defaults to $PWD.
 egit_clean() {
 	[[ $# -eq 0 ]] && set -- .
-	find "$@" -type d -name '.git*' -prune -print0 | xargs -0 rm -rf
+	find "$@" -type d -name '.git*' -prune -exec rm -rf '{}' \+
 }
 
 # @FUNCTION: emktemp
-- 
2.16.1

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

Re: [gentoo-dev] RFC: eutils.eclass: More reliable return status for e*_clean.

[Michał Górny]

Dnia 16 lutego 2018 09:46:33 CET, Ulrich Mueller <ulm@gentoo.org> napisał(a):
>Currently the return status of e{cvs,svn,git}_clean is not at all
>reliable, because only the last command in the pipeline is tested.
>In addition, there are two pipelines in ecvs_clean, and the exit
>status of the first one is completely ignored.
>
>Another issue is that xargs -0 is an extension not specified by POSIX.
>See patch included below, fixing both problems. (Note that this is
>close to the implementation originally proposed in bug 210708 [1].)
>
>Should we take this as an opportunity to split off these three
>functions into their own eclass, e.g. vcs-clean.eclass?
>
>Ulrich
>
>[1] https://bugs.gentoo.org/210708
>
>
>From 83c0afbfb4ef0c501c65dcd8e0c9c8bd30cdd70a Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?Ulrich=20M=C3=BCller?= <ulm@gentoo.org>
>Date: Fri, 16 Feb 2018 07:47:46 +0100
>Subject: [PATCH] eutils.eclass: More reliable return status for
>e*_clean.
>
>In ecvs_clean, combine the two find commands into one, so that the
>exit status of the first one won't be ignored.
>
>Also use find -exec rather then find | xargs, so we don't have to
>check the exit status of all commands in the pipeline.
>---
> eclass/eutils.eclass | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/eclass/eutils.eclass b/eclass/eutils.eclass
>index 8bbd561015ad..bce8cf1c2610 100644
>--- a/eclass/eutils.eclass
>+++ b/eclass/eutils.eclass
>@@ -44,8 +44,8 @@ fi
> # internal CVS directories.  Defaults to $PWD.
> ecvs_clean() {
> 	[[ $# -eq 0 ]] && set -- .
>-	find "$@" -type d -name 'CVS' -prune -print0 | xargs -0 rm -rf
>-	find "$@" -type f -name '.cvs*' -print0 | xargs -0 rm -rf
>+	find "$@" \( -type d -name 'CVS' -prune -o -type f -name '.cvs*' \) \
>+		-exec rm -rf '{}' \+

Could you please use quotes instead of backslash escapes all the way? They're generally less confusing since escapes trigger different behavior in different contexts in bash.

> }
> 
> # @FUNCTION: esvn_clean
>@@ -55,7 +55,7 @@ ecvs_clean() {
> # internal Subversion directories.  Defaults to $PWD.
> esvn_clean() {
> 	[[ $# -eq 0 ]] && set -- .
>-	find "$@" -type d -name '.svn' -prune -print0 | xargs -0 rm -rf
>+	find "$@" -type d -name '.svn' -prune -exec rm -rf '{}' \+
> }
> 
> # @FUNCTION: egit_clean
>@@ -65,7 +65,7 @@ esvn_clean() {
> # contains internal Git directories.  Defaults to $PWD.
> egit_clean() {
> 	[[ $# -eq 0 ]] && set -- .
>-	find "$@" -type d -name '.git*' -prune -print0 | xargs -0 rm -rf
>+	find "$@" -type d -name '.git*' -prune -exec rm -rf '{}' \+
> }
> 
> # @FUNCTION: emktemp


-- 
Best regards,
Michał Górny (by phone)

Re: [gentoo-dev] RFC: eutils.eclass: More reliable return status for e*_clean.

[Michael Orlitzky]

On 02/16/2018 03:46 AM, Ulrich Mueller wrote:
>
> Should we take this as an opportunity to split off these three
> functions into their own eclass, e.g. vcs-clean.eclass?

I think this is a good direction to go in. Changing a popular eclass is
always scary, and the more unrelated stuff it contains, the harder it
gets. It's not easy to tell which ebuilds use the part of the eclass
that you're touching, so you wind up testing (or at least worrying
about) them all. There's the metadata regen, too.

To make maintenance easier, I would go one step further and say that
unless two functions need the same variables or call one another, they
belong in separate eclasses. Since ecvs_clean, esvn_clean, and
egit_clean are completely independent of one another, they could go in
separate eclasses -- it's not like you'll need more than one of them in
your ebuild. Then in the future if we need to change egit_clean, we will
know precisely which ebuilds are affected.

Re: [gentoo-dev] RFC: eutils.eclass: More reliable return status for e*_clean.

[Michał Górny]

W dniu pią, 16.02.2018 o godzinie 08∶06 -0500, użytkownik Michael
Orlitzky napisał:
> On 02/16/2018 03:46 AM, Ulrich Mueller wrote:
> > 
> > Should we take this as an opportunity to split off these three
> > functions into their own eclass, e.g. vcs-clean.eclass?
> 
> I think this is a good direction to go in. Changing a popular eclass is
> always scary, and the more unrelated stuff it contains, the harder it
> gets. It's not easy to tell which ebuilds use the part of the eclass
> that you're touching, so you wind up testing (or at least worrying
> about) them all. There's the metadata regen, too.
> 
> To make maintenance easier, I would go one step further and say that
> unless two functions need the same variables or call one another, they
> belong in separate eclasses. Since ecvs_clean, esvn_clean, and
> egit_clean are completely independent of one another, they could go in
> separate eclasses -- it's not like you'll need more than one of them in
> your ebuild. Then in the future if we need to change egit_clean, we will
> know precisely which ebuilds are affected.
> 

When you reach the point of having one eclass per one-command function,
you should have already figured out it's easier to inline the command
in ebuilds.

-- 
Best regards,
Michał Górny

[gentoo-dev] RFC v2: eutils.eclass: More reliable return status for e*_clean.

[Ulrich Mueller]

[-- Attachment #1.1: message body text --]
[-- Type: text/plain, Size: 1322 bytes --]

>>>>> On Fri, 16 Feb 2018, Michał Górny wrote:

> Could you please use quotes instead of backslash escapes all the
> way? They're generally less confusing since escapes trigger
> different behavior in different contexts in bash.

Done for the parentheses, and removed escaping of the plus sign which
is no special character in bash.

>>>>> On Fri, 16 Feb 2018, Michael Orlitzky wrote:

> To make maintenance easier, I would go one step further and say that
> unless two functions need the same variables or call one another,
> they belong in separate eclasses. Since ecvs_clean, esvn_clean, and
> egit_clean are completely independent of one another, they could go
> in separate eclasses -- it's not like you'll need more than one of
> them in your ebuild. Then in the future if we need to change
> egit_clean, we will know precisely which ebuilds are affected.

If these were 200 line functions then I would agree. However, they
consist of one line each, are all about the same theme, and too much
fragmentation of eclasses should be avoided. In fact, I am already
reluctant about splitting off these tiny functions from eutils.

Anyway, updated patches are attached. First patch changes the code,
second patch splits the functions off into their own eclass (with no
code changes).

Ulrich


[-- Attachment #1.2: 0001-eutils.eclass-More-reliable-return-status-for-e-_cle.patch --]
[-- Type: text/plain, Size: 1739 bytes --]

From 5cd1763df2dc385524da0163bdcadecfaf93675f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ulrich=20M=C3=BCller?= <ulm@gentoo.org>
Date: Fri, 16 Feb 2018 07:47:46 +0100
Subject: [PATCH 1/2] eutils.eclass: More reliable return status for e*_clean
 functions.

In ecvs_clean, combine the two find commands into one, so that the
exit status of the first one won't be ignored.

Also use find -exec rather then find | xargs, so we don't have to
check the exit status of all commands in the pipeline.
---
 eclass/eutils.eclass | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/eclass/eutils.eclass b/eclass/eutils.eclass
index 8bbd561015ad..0a5bf3853582 100644
--- a/eclass/eutils.eclass
+++ b/eclass/eutils.eclass
@@ -44,8 +44,8 @@ fi
 # internal CVS directories.  Defaults to $PWD.
 ecvs_clean() {
 	[[ $# -eq 0 ]] && set -- .
-	find "$@" -type d -name 'CVS' -prune -print0 | xargs -0 rm -rf
-	find "$@" -type f -name '.cvs*' -print0 | xargs -0 rm -rf
+	find "$@" '(' -type d -name 'CVS' -prune -o -type f -name '.cvs*' ')' \
+		-exec rm -rf '{}' +
 }
 
 # @FUNCTION: esvn_clean
@@ -55,7 +55,7 @@ ecvs_clean() {
 # internal Subversion directories.  Defaults to $PWD.
 esvn_clean() {
 	[[ $# -eq 0 ]] && set -- .
-	find "$@" -type d -name '.svn' -prune -print0 | xargs -0 rm -rf
+	find "$@" -type d -name '.svn' -prune -exec rm -rf '{}' +
 }
 
 # @FUNCTION: egit_clean
@@ -65,7 +65,7 @@ esvn_clean() {
 # contains internal Git directories.  Defaults to $PWD.
 egit_clean() {
 	[[ $# -eq 0 ]] && set -- .
-	find "$@" -type d -name '.git*' -prune -print0 | xargs -0 rm -rf
+	find "$@" -type d -name '.git*' -prune -exec rm -rf '{}' +
 }
 
 # @FUNCTION: emktemp
-- 
2.16.1


[-- Attachment #1.3: 0002-vcs-clean.eclass-Split-off-clean-helpers-from-eutils.patch --]
[-- Type: text/plain, Size: 3808 bytes --]

From 5056a7a455bac8a5b5bdadc357cc67cf0f6865fd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ulrich=20M=C3=BCller?= <ulm@gentoo.org>
Date: Fri, 16 Feb 2018 18:48:13 +0100
Subject: [PATCH 2/2] vcs-clean.eclass: Split off clean helpers from
 eutils.eclass.

Split off functions ecvs_clean, esvn_clean, and egit_clean into
a dedicated vcs-clean.eclass. No code changes.

For backwards compatibility, eutils inherits the new eclass in
existing EAPIs.
---
 eclass/eutils.eclass    | 34 ++--------------------------------
 eclass/vcs-clean.eclass | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 32 deletions(-)
 create mode 100644 eclass/vcs-clean.eclass

diff --git a/eclass/eutils.eclass b/eclass/eutils.eclass
index 0a5bf3853582..7840afbb77b9 100644
--- a/eclass/eutils.eclass
+++ b/eclass/eutils.eclass
@@ -20,7 +20,8 @@ _EUTILS_ECLASS=1
 # implicitly inherited (now split) eclasses
 case ${EAPI:-0} in
 0|1|2|3|4|5|6)
-	inherit desktop epatch estack ltprune multilib preserve-libs toolchain-funcs
+	inherit desktop epatch estack ltprune multilib preserve-libs \
+		toolchain-funcs vcs-clean
 	;;
 esac
 
@@ -37,37 +38,6 @@ if ! declare -F eqawarn >/dev/null ; then
 	}
 fi
 
-# @FUNCTION: ecvs_clean
-# @USAGE: [list of dirs]
-# @DESCRIPTION:
-# Remove CVS directories recursiveley.  Useful when a source tarball contains
-# internal CVS directories.  Defaults to $PWD.
-ecvs_clean() {
-	[[ $# -eq 0 ]] && set -- .
-	find "$@" '(' -type d -name 'CVS' -prune -o -type f -name '.cvs*' ')' \
-		-exec rm -rf '{}' +
-}
-
-# @FUNCTION: esvn_clean
-# @USAGE: [list of dirs]
-# @DESCRIPTION:
-# Remove .svn directories recursiveley.  Useful when a source tarball contains
-# internal Subversion directories.  Defaults to $PWD.
-esvn_clean() {
-	[[ $# -eq 0 ]] && set -- .
-	find "$@" -type d -name '.svn' -prune -exec rm -rf '{}' +
-}
-
-# @FUNCTION: egit_clean
-# @USAGE: [list of dirs]
-# @DESCRIPTION:
-# Remove .git* directories/files recursiveley.  Useful when a source tarball
-# contains internal Git directories.  Defaults to $PWD.
-egit_clean() {
-	[[ $# -eq 0 ]] && set -- .
-	find "$@" -type d -name '.git*' -prune -exec rm -rf '{}' +
-}
-
 # @FUNCTION: emktemp
 # @USAGE: [temp dir]
 # @DESCRIPTION:
diff --git a/eclass/vcs-clean.eclass b/eclass/vcs-clean.eclass
new file mode 100644
index 000000000000..649a9e3039b1
--- /dev/null
+++ b/eclass/vcs-clean.eclass
@@ -0,0 +1,40 @@
+# Copyright 1999-2018 Gentoo Foundation
+# Distributed under the terms of the GNU General Public License v2
+
+# @ECLASS: vcs-clean.eclass
+# @MAINTAINER:
+# base-system@gentoo.org
+# @AUTHOR:
+# Benedikt Böhm <hollow@gentoo.org>
+# @BLURB: helper functions to remove VCS directories
+
+# @FUNCTION: ecvs_clean
+# @USAGE: [list of dirs]
+# @DESCRIPTION:
+# Remove CVS directories and .cvs* files recursively.  Useful when a
+# source tarball contains internal CVS directories.  Defaults to ${PWD}.
+ecvs_clean() {
+	[[ $# -eq 0 ]] && set -- .
+	find "$@" '(' -type d -name 'CVS' -prune -o -type f -name '.cvs*' ')' \
+		-exec rm -rf '{}' +
+}
+
+# @FUNCTION: esvn_clean
+# @USAGE: [list of dirs]
+# @DESCRIPTION:
+# Remove .svn directories recursively.  Useful when a source tarball
+# contains internal Subversion directories.  Defaults to ${PWD}.
+esvn_clean() {
+	[[ $# -eq 0 ]] && set -- .
+	find "$@" -type d -name '.svn' -prune -exec rm -rf '{}' +
+}
+
+# @FUNCTION: egit_clean
+# @USAGE: [list of dirs]
+# @DESCRIPTION:
+# Remove .git* directories recursively.  Useful when a source tarball
+# contains internal Git directories.  Defaults to ${PWD}.
+egit_clean() {
+	[[ $# -eq 0 ]] && set -- .
+	find "$@" -type d -name '.git*' -prune -exec rm -rf '{}' +
+}
-- 
2.16.1


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

Re: [gentoo-dev] RFC v2: eutils.eclass: More reliable return status for e*_clean.

[Michał Górny]

W dniu pią, 16.02.2018 o godzinie 19∶12 +0100, użytkownik Ulrich Mueller
napisał:
> > > > > > On Fri, 16 Feb 2018, Michał Górny wrote:
> > Could you please use quotes instead of backslash escapes all the
> > way? They're generally less confusing since escapes trigger
> > different behavior in different contexts in bash.
> 
> Done for the parentheses, and removed escaping of the plus sign which
> is no special character in bash.

Thanks. Looks good to me now.


-- 
Best regards,
Michał Górny