Re: [gentoo-dev] Re: [gentoo-commits] gentoo-x86 commit in eclass: rox-0install.eclass

[Jim Ramsay]

Donnie Berkholz wrote:
> I don't remember this going by gentoo-dev. Always send eclasses to
> gentoo-dev before committing them.

My apologies, I was just so excited that I'd finally finished it!

Apparently I missed a few things, thanks very much for catching
it.

> > 0install_native_feed() {
> > 	local src="${1}"; shift
> > 	local path="${1}"; shift
>
> This is a rather strange paradigm to me, instead of:
>
> local src=$1 path=$2
> shift 2

This is cleaner, thanks.  In fact, I don't even really need the
shift at all.

> > 	local feedfile=$(basename "${src}")
>
> You could do this in pure bash, although it doesn't really matter:
>
> local feedfile=${src##*/}

Sure, may as well, save a subshell.

> > 	local dest="${path}/$feedfile"
>
> How do you decide when not to use braces { } around variables?

In general I've used them everywhere... except there :)  Fixed.

> > 	0distutils "${src}" > tmp.native_feed || die "0distutils feed edit failed"
> >
> > 	if [[ ${ZEROINSTALL_STRIP_REQUIRES} ]]; then
> > 		# Strip out all 'requires' sections
> > 		sed -i -e '/<requires.*\/>/d' \
> > 			-e '/<requires.*\>/,/<\/requires>/d' tmp.native_feed
>
> What happens if the contents of a <requires> section are on a separate
> line? Is this a concern?

It hasn't been so far - The convention in all known cases is
either it's all on one line, or a multi-line as is caught  by the
second sed expression.  This is just a stopgap measure until I
rework 0distutils to do this optional stripping on its own.

> > 	local feedname
>
> You could just declare feedname local and set it in the same line.

Not if I want to potentially die on the assignment.  As I found
out in #gentoo-dev-help today, try this:

$ t() { local a=$(false) || echo "Die"; }; t

Versus this:

$ t() { local a; a=$(false) || echo "Die"; }; t
Die

> > 	feedname=$(0distutils -e "${src}") || "0distutils URI escape failed"
>
> What's the || doing? You've got a string sitting there. Is 'die'
> missing?

Verily - nice catch!

> > 	if ! $installed; then
>
> This is kind of a weird way to do it. I'd check instead for
> [[ -n ${installed} ]] and initialize it to empty.

Sure, that looks nicer.

I'll be committing these changes right away, since the "die" ones
at least are very important.  But I'll be submitting further
changes to the list first.  Sorry about that :)

Luckily only 1 ebuild so far which uses this eclass has actually
hit the tree!

-- 
Jim Ramsay
Gentoo/Linux Developer (rox/fluxbox/gkrellm)