[gentoo-dev] [PATCH eutils] Introduce run_in_build_dir() used in a few ebuilds.

[gentoo-dev] [PATCH eutils] Introduce run_in_build_dir() used in a few ebuilds.

[Michał Górny]

The run_in_build_dir() command simply runs given command
in the directory stated as BUILD_DIR. This variable is used commonly
by autotools-utils, cmake-utils and python-r1 eclasses, therefore I'm
proposing adding the relevant function to eutils.
---
 gx86/eclass/eutils.eclass | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/gx86/eclass/eutils.eclass b/gx86/eclass/eutils.eclass
index 6588792..bb3c1e3 100644
--- a/gx86/eclass/eutils.eclass
+++ b/gx86/eclass/eutils.eclass
@@ -1495,6 +1495,25 @@ prune_libtool_files() {
 	fi
 }
 
+# @FUNCTION: run_in_build_dir
+# @USAGE: <argv>...
+# @DESCRIPTION:
+# Run the given command in the directory pointed by BUILD_DIR.
+run_in_build_dir() {
+	debug-print-function ${FUNCNAME} "$@"
+	local ret
+
+	[[ ${#} -ne 0 ]] || die "${FUNCNAME}: no command specified."
+	[[ ${BUILD_DIR} ]] || die "${FUNCNAME}: BUILD_DIR not set."
+
+	pushd "${BUILD_DIR}" &>/dev/null || die
+	"${@}"
+	ret=${?}
+	popd &>/dev/null || die
+
+	return ${ret}
+}
+
 check_license() { die "you no longer need this as portage supports ACCEPT_LICENSE itself"; }
 
 fi
-- 
1.8.1

Re: [gentoo-dev] [PATCH eutils] Introduce run_in_build_dir() used in a few ebuilds.

[William Hubbs]

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

On Sun, Jan 13, 2013 at 02:29:43PM +0100, Michał Górny wrote:
> The run_in_build_dir() command simply runs given command
> in the directory stated as BUILD_DIR. This variable is used commonly
> by autotools-utils, cmake-utils and python-r1 eclasses, therefore I'm
> proposing adding the relevant function to eutils.
> ---
>  gx86/eclass/eutils.eclass | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/gx86/eclass/eutils.eclass b/gx86/eclass/eutils.eclass
> index 6588792..bb3c1e3 100644
> --- a/gx86/eclass/eutils.eclass
> +++ b/gx86/eclass/eutils.eclass
> @@ -1495,6 +1495,25 @@ prune_libtool_files() {
>  	fi
>  }
>  
> +# @FUNCTION: run_in_build_dir
> +# @USAGE: <argv>...
> +# @DESCRIPTION:
> +# Run the given command in the directory pointed by BUILD_DIR.

I think I would make this more generic if it is going in eutiles,
e.g. rename it something like run_in_dir and pass in the directory as the
first argument.

William


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

Re: [gentoo-dev] [PATCH eutils] Introduce run_in_build_dir() used in a few ebuilds.

[Michał Górny]

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

On Sun, 13 Jan 2013 09:05:31 -0600
William Hubbs <williamh@gentoo.org> wrote:

> On Sun, Jan 13, 2013 at 02:29:43PM +0100, Michał Górny wrote:
> > The run_in_build_dir() command simply runs given command
> > in the directory stated as BUILD_DIR. This variable is used commonly
> > by autotools-utils, cmake-utils and python-r1 eclasses, therefore I'm
> > proposing adding the relevant function to eutils.
> > ---
> >  gx86/eclass/eutils.eclass | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/gx86/eclass/eutils.eclass b/gx86/eclass/eutils.eclass
> > index 6588792..bb3c1e3 100644
> > --- a/gx86/eclass/eutils.eclass
> > +++ b/gx86/eclass/eutils.eclass
> > @@ -1495,6 +1495,25 @@ prune_libtool_files() {
> >  	fi
> >  }
> >  
> > +# @FUNCTION: run_in_build_dir
> > +# @USAGE: <argv>...
> > +# @DESCRIPTION:
> > +# Run the given command in the directory pointed by BUILD_DIR.
> 
> I think I would make this more generic if it is going in eutiles,
> e.g. rename it something like run_in_dir and pass in the directory as the
> first argument.

That's not going to work for us since the command is subject to a loop
which sets BUILD_DIR, e.g.:

  python_foreach_impl run_in_build_dir ...

with python_foreach_impl setting BUILD_DIR.

-- 
Best regards,
Michał Górny

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

Re: [gentoo-dev] [PATCH eutils] Introduce run_in_build_dir() used in a few ebuilds.

[Alec Warner]

On Sun, Jan 13, 2013 at 5:29 AM, Michał Górny <mgorny@gentoo.org> wrote:
> The run_in_build_dir() command simply runs given command
> in the directory stated as BUILD_DIR. This variable is used commonly
> by autotools-utils, cmake-utils and python-r1 eclasses, therefore I'm
> proposing adding the relevant function to eutils.
> ---
>  gx86/eclass/eutils.eclass | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/gx86/eclass/eutils.eclass b/gx86/eclass/eutils.eclass
> index 6588792..bb3c1e3 100644
> --- a/gx86/eclass/eutils.eclass
> +++ b/gx86/eclass/eutils.eclass
> @@ -1495,6 +1495,25 @@ prune_libtool_files() {
>         fi
>  }
>
> +# @FUNCTION: run_in_build_dir
> +# @USAGE: <argv>...
> +# @DESCRIPTION:
> +# Run the given command in the directory pointed by BUILD_DIR.
> +run_in_build_dir() {
> +       debug-print-function ${FUNCNAME} "$@"
> +       local ret

local -i ret
?

> +
> +       [[ ${#} -ne 0 ]] || die "${FUNCNAME}: no command specified."
> +       [[ ${BUILD_DIR} ]] || die "${FUNCNAME}: BUILD_DIR not set."
> +
> +       pushd "${BUILD_DIR}" &>/dev/null || die
> +       "${@}"
> +       ret=${?}
> +       popd &>/dev/null || die
> +
> +       return ${ret}
> +}
> +
>  check_license() { die "you no longer need this as portage supports ACCEPT_LICENSE itself"; }
>
>  fi
> --
> 1.8.1
>
>

Re: [gentoo-dev] [PATCH eutils] Introduce run_in_build_dir() used in a few ebuilds.

[Gilles Dartiguelongue]

Le dimanche 13 janvier 2013 à 16:08 +0100, Michał Górny a écrit :
> On Sun, 13 Jan 2013 09:05:31 -0600
> William Hubbs <williamh@gentoo.org> wrote:
> 
> > On Sun, Jan 13, 2013 at 02:29:43PM +0100, Michał Górny wrote:
> > > The run_in_build_dir() command simply runs given command
> > > in the directory stated as BUILD_DIR. This variable is used commonly
> > > by autotools-utils, cmake-utils and python-r1 eclasses, therefore I'm
> > > proposing adding the relevant function to eutils.
> > > ---
> > >  gx86/eclass/eutils.eclass | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > > 
> > > diff --git a/gx86/eclass/eutils.eclass b/gx86/eclass/eutils.eclass
> > > index 6588792..bb3c1e3 100644
> > > --- a/gx86/eclass/eutils.eclass
> > > +++ b/gx86/eclass/eutils.eclass
> > > @@ -1495,6 +1495,25 @@ prune_libtool_files() {
> > >  	fi
> > >  }
> > >  
> > > +# @FUNCTION: run_in_build_dir
> > > +# @USAGE: <argv>...
> > > +# @DESCRIPTION:
> > > +# Run the given command in the directory pointed by BUILD_DIR.
> > 
> > I think I would make this more generic if it is going in eutiles,
> > e.g. rename it something like run_in_dir and pass in the directory as the
> > first argument.
> 
> That's not going to work for us since the command is subject to a loop
> which sets BUILD_DIR, e.g.:
> 
>   python_foreach_impl run_in_build_dir ...
> 
> with python_foreach_impl setting BUILD_DIR.

FTR, this function is used as-is in quite a few gnome ebuilds that use
python-r1 eclass. We thought that it could probably be used in other
places but it would be nice if we could have changes to would make it
not suitable for this purpose.

-- 
Gilles Dartiguelongue <eva@gentoo.org>
Gentoo

Re: [gentoo-dev] [PATCH eutils] Introduce run_in_build_dir() used in a few ebuilds.

[William Hubbs]

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

On Sun, Jan 13, 2013 at 04:08:18PM +0100, Michał Górny wrote:
> On Sun, 13 Jan 2013 09:05:31 -0600
> William Hubbs <williamh@gentoo.org> wrote:
> 
> > On Sun, Jan 13, 2013 at 02:29:43PM +0100, Michał Górny wrote:
> > > The run_in_build_dir() command simply runs given command
> > > in the directory stated as BUILD_DIR. This variable is used commonly
> > > by autotools-utils, cmake-utils and python-r1 eclasses, therefore I'm
> > > proposing adding the relevant function to eutils.
> > > ---
> > >  gx86/eclass/eutils.eclass | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > > 
> > > diff --git a/gx86/eclass/eutils.eclass b/gx86/eclass/eutils.eclass
> > > index 6588792..bb3c1e3 100644
> > > --- a/gx86/eclass/eutils.eclass
> > > +++ b/gx86/eclass/eutils.eclass
> > > @@ -1495,6 +1495,25 @@ prune_libtool_files() {
> > >  	fi
> > >  }
> > >  
> > > +# @FUNCTION: run_in_build_dir
> > > +# @USAGE: <argv>...
> > > +# @DESCRIPTION:
> > > +# Run the given command in the directory pointed by BUILD_DIR.
> > 
> > I think I would make this more generic if it is going in eutiles,
> > e.g. rename it something like run_in_dir and pass in the directory as the
> > first argument.
> 
> That's not going to work for us since the command is subject to a loop
> which sets BUILD_DIR, e.g.:
> 
>   python_foreach_impl run_in_build_dir ...

Can you not change the logic so it doesn't die if build_dir isn't set,
but uses the value of $1 and calls shift?

Thanks,

William

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

Re: [gentoo-dev] [PATCH eutils] Introduce run_in_build_dir() used in a few ebuilds.

[Michał Górny]

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

On Sun, 13 Jan 2013 09:52:09 -0600
William Hubbs <williamh@gentoo.org> wrote:

> On Sun, Jan 13, 2013 at 04:08:18PM +0100, Michał Górny wrote:
> > On Sun, 13 Jan 2013 09:05:31 -0600
> > William Hubbs <williamh@gentoo.org> wrote:
> > 
> > > On Sun, Jan 13, 2013 at 02:29:43PM +0100, Michał Górny wrote:
> > > > The run_in_build_dir() command simply runs given command
> > > > in the directory stated as BUILD_DIR. This variable is used commonly
> > > > by autotools-utils, cmake-utils and python-r1 eclasses, therefore I'm
> > > > proposing adding the relevant function to eutils.
> > > > ---
> > > >  gx86/eclass/eutils.eclass | 19 +++++++++++++++++++
> > > >  1 file changed, 19 insertions(+)
> > > > 
> > > > diff --git a/gx86/eclass/eutils.eclass b/gx86/eclass/eutils.eclass
> > > > index 6588792..bb3c1e3 100644
> > > > --- a/gx86/eclass/eutils.eclass
> > > > +++ b/gx86/eclass/eutils.eclass
> > > > @@ -1495,6 +1495,25 @@ prune_libtool_files() {
> > > >  	fi
> > > >  }
> > > >  
> > > > +# @FUNCTION: run_in_build_dir
> > > > +# @USAGE: <argv>...
> > > > +# @DESCRIPTION:
> > > > +# Run the given command in the directory pointed by BUILD_DIR.
> > > 
> > > I think I would make this more generic if it is going in eutiles,
> > > e.g. rename it something like run_in_dir and pass in the directory as the
> > > first argument.
> > 
> > That's not going to work for us since the command is subject to a loop
> > which sets BUILD_DIR, e.g.:
> > 
> >   python_foreach_impl run_in_build_dir ...
> 
> Can you not change the logic so it doesn't die if build_dir isn't set,
> but uses the value of $1 and calls shift?

Where? What? I don't follow really. And since I don't follow, I don't
think that's something reasonable to do.

-- 
Best regards,
Michał Górny

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

Re: [gentoo-dev] [PATCH eutils] Introduce run_in_build_dir() used in a few ebuilds.

[Michał Górny]

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

On Sun, 13 Jan 2013 07:36:59 -0800
Alec Warner <antarus@gentoo.org> wrote:

> On Sun, Jan 13, 2013 at 5:29 AM, Michał Górny <mgorny@gentoo.org> wrote:
> > The run_in_build_dir() command simply runs given command
> > in the directory stated as BUILD_DIR. This variable is used commonly
> > by autotools-utils, cmake-utils and python-r1 eclasses, therefore I'm
> > proposing adding the relevant function to eutils.
> > ---
> >  gx86/eclass/eutils.eclass | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/gx86/eclass/eutils.eclass b/gx86/eclass/eutils.eclass
> > index 6588792..bb3c1e3 100644
> > --- a/gx86/eclass/eutils.eclass
> > +++ b/gx86/eclass/eutils.eclass
> > @@ -1495,6 +1495,25 @@ prune_libtool_files() {
> >         fi
> >  }
> >
> > +# @FUNCTION: run_in_build_dir
> > +# @USAGE: <argv>...
> > +# @DESCRIPTION:
> > +# Run the given command in the directory pointed by BUILD_DIR.
> > +run_in_build_dir() {
> > +       debug-print-function ${FUNCNAME} "$@"
> > +       local ret
> 
> local -i ret
> ?

Looks good. I didn't even know bash has something like that.

-- 
Best regards,
Michał Górny

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

Re: [gentoo-dev] [PATCH eutils] Introduce run_in_build_dir() used in a few ebuilds.

[Gilles Dartiguelongue]

Le dimanche 13 janvier 2013 à 09:52 -0600, William Hubbs a écrit :
> On Sun, Jan 13, 2013 at 04:08:18PM +0100, Michał Górny wrote:
> > On Sun, 13 Jan 2013 09:05:31 -0600
> > William Hubbs <williamh@gentoo.org> wrote:
> > 
> > > On Sun, Jan 13, 2013 at 02:29:43PM +0100, Michał Górny wrote:
> > > > The run_in_build_dir() command simply runs given command
> > > > in the directory stated as BUILD_DIR. This variable is used commonly
> > > > by autotools-utils, cmake-utils and python-r1 eclasses, therefore I'm
> > > > proposing adding the relevant function to eutils.
> > > > ---
> > > >  gx86/eclass/eutils.eclass | 19 +++++++++++++++++++
> > > >  1 file changed, 19 insertions(+)
> > > > 
> > > > diff --git a/gx86/eclass/eutils.eclass b/gx86/eclass/eutils.eclass
> > > > index 6588792..bb3c1e3 100644
> > > > --- a/gx86/eclass/eutils.eclass
> > > > +++ b/gx86/eclass/eutils.eclass
> > > > @@ -1495,6 +1495,25 @@ prune_libtool_files() {
> > > >  	fi
> > > >  }
> > > >  
> > > > +# @FUNCTION: run_in_build_dir
> > > > +# @USAGE: <argv>...
> > > > +# @DESCRIPTION:
> > > > +# Run the given command in the directory pointed by BUILD_DIR.
> > > 
> > > I think I would make this more generic if it is going in eutiles,
> > > e.g. rename it something like run_in_dir and pass in the directory as the
> > > first argument.
> > 
> > That's not going to work for us since the command is subject to a loop
> > which sets BUILD_DIR, e.g.:
> > 
> >   python_foreach_impl run_in_build_dir ...
> 
> Can you not change the logic so it doesn't die if build_dir isn't set,
> but uses the value of $1 and calls shift?

I guess an explicit option would be less error prone, eg. :

run_in_dir -d foodir barfunc arg1 arg2
or
run_in_dir --directory foodir barfunc arg1 arg2

documentation could then mention that the function defaults to BUILD_DIR
if this option is not set and fails if nothing is set.

-- 
Gilles Dartiguelongue <eva@gentoo.org>
Gentoo

Re: [gentoo-dev] [PATCH eutils] Introduce run_in_build_dir() used in a few ebuilds.

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 276 bytes --]

On Sunday 13 January 2013 08:29:43 Michał Górny wrote:
> +	[[ ${BUILD_DIR} ]] || die "${FUNCNAME}: BUILD_DIR not set."

really should use -n there

> +	pushd "${BUILD_DIR}" &>/dev/null || die
> +	popd &>/dev/null || die

sending errors to /dev/null is wrong
-mike

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

Re: [gentoo-dev] [PATCH eutils] Introduce run_in_build_dir() used in a few ebuilds.

[Dan Douglas]

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

On Sunday, January 13, 2013 06:02:26 PM Michał Górny wrote:
> On Sun, 13 Jan 2013 07:36:59 -0800
> Alec Warner <antarus@gentoo.org> wrote:
> > local -i ret
> > ?
> 
> Looks good. I didn't even know bash has something like that.
> 

Useless use of -i is usually unhelpful. In Bash (but not all shells), it's 
slower, because it's still just storing the value as a string and marking the 
variable with the integer attribute which modifies the way certain assignments 
behave.

It doesn't do any harm in this specific case, but there are a lot of gotchas 
with -i to be aware of if you're going to use it.
-- 
Dan Douglas

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

Re: [gentoo-dev] [PATCH eutils] Introduce run_in_build_dir() used in a few ebuilds.

[Dan Douglas]

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

On Sunday, January 13, 2013 01:28:42 PM Mike Frysinger wrote:
> On Sunday 13 January 2013 08:29:43 Michał Górny wrote:
> > +	[[ ${BUILD_DIR} ]] || die "${FUNCNAME}: BUILD_DIR not set."
> 
> really should use -n there
> 
Doesn't matter. Only zsh requires it (and pointlessly disagrees with every 
other implementation).

There are many possibilities.
${BUILD_DIR:+:} die "${FUNCNAME}: BUILD_DIR unset or empty."

-- 
Dan Douglas

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

Re: [gentoo-dev] [PATCH eutils] Introduce run_in_build_dir() used in a few ebuilds.

[Alec Warner]

On Mon, Jan 14, 2013 at 9:30 AM, Dan Douglas <ormaaj@gmail.com> wrote:
> On Sunday, January 13, 2013 06:02:26 PM Michał Górny wrote:
>> On Sun, 13 Jan 2013 07:36:59 -0800
>> Alec Warner <antarus@gentoo.org> wrote:
>> > local -i ret
>> > ?
>>
>> Looks good. I didn't even know bash has something like that.
>>
>
> Useless use of -i is usually unhelpful. In Bash (but not all shells), it's
> slower, because it's still just storing the value as a string and marking the
> variable with the integer attribute which modifies the way certain assignments
> behave.
>
> It doesn't do any harm in this specific case, but there are a lot of gotchas
> with -i to be aware of if you're going to use it.
> --
> Dan Douglas

It is bash...there are lots of gotchas with almost *all* of bash's features ;)

-A

Re: [gentoo-dev] [PATCH eutils] Introduce run_in_build_dir() used in a few ebuilds.

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 515 bytes --]

On Monday 14 January 2013 12:55:29 Dan Douglas wrote:
> On Sunday, January 13, 2013 01:28:42 PM Mike Frysinger wrote:
> > On Sunday 13 January 2013 08:29:43 Michał Górny wrote:
> > > +	[[ ${BUILD_DIR} ]] || die "${FUNCNAME}: BUILD_DIR not set."
> > 
> > really should use -n there
> 
> Doesn't matter. Only zsh requires it (and pointlessly disagrees with every
> other implementation).

the point wasn't "will it work".  it's more "how easy is it to glance at code 
and know what it is doing".
-mike

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

[gentoo-dev] Re: [PATCH eutils] Introduce run_in_build_dir() used in a few ebuilds.

[Duncan]

Mike Frysinger posted on Mon, 14 Jan 2013 17:09:51 -0500 as excerpted:

>>> +	[[ ${BUILD_DIR} ]] || die "${FUNCNAME}: BUILD_DIR not set."
>> 
>> really should use -n there
> 
>> Doesn't matter.
> 
> the point wasn't "will it work".  it's more "how easy is it to glance at
> code and know what it is doing".

Indeed.  But arguably standalone [[ ${var} ]] tests ARE easier to "know 
what it doing."  

Consider:

1) The [[ $var ]] case is exactly one of one.  There's no misinterpreting 
it.  Even if you don't know the rule by rote, given that it's a boolean 
test on a string, there's logically only one way to parse it.  No 
mistakes to make.

2) By contrast, -n is only one of a whole list of -X style tests, and one 
must stop and think, "Let's see... Oh, yes, -n was non-null."

3) The situation in #2 is even worse than it might be due to the  
standalone alternative, making -n (and -z, #4) far less common than they 
would be otherwise.

4) You are arguing the "at a glance" position, but didn't even MENTION 
the needlessly negated logic of [[ -n $string ]] || ... , which could be 
rewritten to avoid the || negation as [[ -z $string ]] && ...

5) [[ ]] is already a bashism while the standalone string test is common 
shell.  Surely you're not arguing that people familiar enough with the 
[[ ]] || construct to parse it at a glance can't equally capably parse 
the a standalone string test, given its use in non-bash shell context as 
well.

6) If you're arguing for explicit, be consistent, with explicit
if/then, test, positive -z instead of negated -n logic... ::

if test -z "${BUILD_DIR}"; then die "${FUNCTNAME}: BUILD_DIR not set."



Obviously the example in #6 is taking it a bit far, but my point is, an 
explicit (if one-of-many) -z/-n test vs. the standalone (single-case) 
$string test really is personal preference.  You obviously prefer it one 
way, others (including me) prefer the other... for the very same reason, 
to us, our preferred way is instantly parsable, while the other way 
triggers "extra processing".

As such, IMO, "He who codes, decides." =:^)

(IIRC I did bring up the same thing, naturally arguing my standalone 
string test preference, in an earlier eclass proposal thread.  However, I 
believe I explicitly mentioned that I considered it a purely personal 
style thing and that others had the opposite preference, with to my 
knowledge no official gentoo coding style position taken on it.  Given 
that I had seen, I think you, bring up the point before, I was hoping 
someone would post the gentoo style-guide link proving me wrong, if there 
was one.  Given that nobody did so, I still believe it to be "he who 
codes, decides" territory.)

-- 
Duncan - List replies preferred.   No HTML msgs.
"Every nonfree program has a lord, a master --
and if you use the program, he is your master."  Richard Stallman

Re: [gentoo-dev] Re: [PATCH eutils] Introduce run_in_build_dir() used in a few ebuilds.

[Dan Douglas]

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

On Tuesday, January 15, 2013 03:20:20 AM Duncan wrote:
> Mike Frysinger posted on Mon, 14 Jan 2013 17:09:51 -0500 as excerpted:
> 
> >>> +	[[ ${BUILD_DIR} ]] || die "${FUNCNAME}: BUILD_DIR not set."
> >> 
> >> really should use -n there
> > 
> >> Doesn't matter.
> > 
> > the point wasn't "will it work".  it's more "how easy is it to glance at
> > code and know what it is doing".
> 
> Indeed.  But arguably standalone [[ ${var} ]] tests ARE easier to "know 
> what it doing."  

While I agree, I wouldn't lose sleep over it. Both are perfectly acceptable 
and nearly equal in clarity. [[ $x ]] is probably most common.

> Consider:
> ...
> 4) You are arguing the "at a glance" position, but didn't even MENTION 
> the needlessly negated logic of [[ -n $string ]] || ... , which could be 
> rewritten to avoid the || negation as [[ -z $string ]] && ...

-z is the negation. -n is the default. [[ $x ]] is rewritten internally as [[ 
-n $x ]]. [[ ! $x ]] == [[ ! -n $x ]] == [[ -z $x ]]. It's perfectly logical 
that this expression evaluates true when given a non-empty value. As ''expr || 
die'' is the most expected form for error-checking, negating this with ''-z'' 
or otherwise makes little sense.

"Empty == False" is not an uncommon concept. e.g. python -c 'print(bool(""))'

> 5) [[ ]] is already a bashism while the standalone string test is common 
> shell.  Surely you're not arguing that people familiar enough with the 
> [[ ]] || construct to parse it at a glance can't equally capably parse 
> the a standalone string test, given its use in non-bash shell context as 
> well.

I wish people would stop referring to features that predate both Bash and 
POSIX as "Bashisms". ''[['' is superior to and easier to understand than 
''[''. Everyone should be familiar with both. As only Bash is relevant here, 
''test'' and ''['' are mostly irrelevant. With few exceptions, ''[['' should 
be preferred when available (and in this case, guaranteed available).

> Obviously the example in #6 is taking it a bit far, but my point is, an 
> explicit (if one-of-many) -z/-n test vs. the standalone (single-case) 
> $string test really is personal preference.

The performance difference is considerable.

-- 
Dan Douglas

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

Re: [gentoo-dev] Re: [PATCH eutils] Introduce run_in_build_dir() used in a few ebuilds.

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 2150 bytes --]

On Monday 14 January 2013 22:20:20 Duncan wrote:
> Mike Frysinger posted on Mon, 14 Jan 2013 17:09:51 -0500 as excerpted:
> >>> +	[[ ${BUILD_DIR} ]] || die "${FUNCNAME}: BUILD_DIR not set."
> >> 
> >> really should use -n there
> >> 
> >> Doesn't matter.
> > 
> > the point wasn't "will it work".  it's more "how easy is it to glance at
> > code and know what it is doing".
> 
> Indeed.  But arguably standalone [[ ${var} ]] tests ARE easier to "know
> what it doing."
> 
> 1) The [[ $var ]] case is exactly one of one.  There's no misinterpreting
> it.  Even if you don't know the rule by rote, given that it's a boolean
> test on a string, there's logically only one way to parse it.  No
> mistakes to make.

not really.  when you see [[ ${var} ]], which did the dev actually mean to do:
	if ( ${var} ) ; then
	if ${var} ; then
	if [[ -n ${var} ]] ; then

if i see the -n, i'm pretty confident they meant to do a string test.  if i 
don't, then i need to read the rest of the code to figure out the meaning & use 
of ${var} to see if they screwed up.

and yes, this still happens.  i've seen fixes in the last month along these 
lines.

> 2) By contrast, -n is only one of a whole list of -X style tests, and one
> must stop and think, "Let's see... Oh, yes, -n was non-null."

i don't buy that.  -z and -n are the most common `test` tests out there.  if 
you can't handle that, then you probably can't handle a lot of the 
ebuilds/eclasses.

> 4) You are arguing the "at a glance" position, but didn't even MENTION
> the needlessly negated logic of [[ -n $string ]] || ... , which could be
> rewritten to avoid the || negation as [[ -z $string ]] && ...

that depends on the code.  there is no correct answer here.

> 5) [[ ]] is already a bashism while the standalone string test is common
> shell.  Surely you're not arguing that people familiar enough with the
> [[ ]] || construct to parse it at a glance can't equally capably parse
> the a standalone string test, given its use in non-bash shell context as
> well.

bashisms are irrelevant.  we already stated the whole tree is bash.
-mike

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