[gentoo-dev] RFD: split out some functions from eutils.eclass?

[gentoo-dev] RFD: split out some functions from eutils.eclass?

[Ulrich Mueller]

Some functions in eutils.eclass address very special tasks, so I
wonder if they shouldn't be split out to dedicated eclasses:

- CDROM functions (cdrom_get_cds, cdrom_load_next_cd).
  These are used by some 40 ebuilds only, most of them in games-*.
  Maybe they could be moved to a dedicated cdrom.eclass?

- unpack_pdv() is used by one ebuild in the tree only.

Ulrich

Re: [gentoo-dev] RFD: split out some functions from eutils.eclass?

["Paweł Hajdan, Jr."]

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

On 1/7/12 12:42 PM, Ulrich Mueller wrote:
> Some functions in eutils.eclass address very special tasks, so I
> wonder if they shouldn't be split out to dedicated eclasses:
> 
> - CDROM functions (cdrom_get_cds, cdrom_load_next_cd).
>   These are used by some 40 ebuilds only, most of them in games-*.
>   Maybe they could be moved to a dedicated cdrom.eclass?

Seconded, that should also help with PROPERTIES="interactive".

> - unpack_pdv() is used by one ebuild in the tree only.

Is it applicable to more ebuilds? If not maybe it can just be moved to
that one ebuild.


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

[gentoo-dev] Re: RFD: split out some functions from eutils.eclass?

[Ulrich Mueller]

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 1062 bytes --]

>>>>> On Sat, 7 Jan 2012, Ulrich Mueller wrote:

> Some functions in eutils.eclass address very special tasks, so I
> wonder if they shouldn't be split out to dedicated eclasses:

> - CDROM functions (cdrom_get_cds, cdrom_load_next_cd).
>   These are used by some 40 ebuilds only, most of them in games-*.
>   Maybe they could be moved to a dedicated cdrom.eclass?

A draft version for a new cdrom.eclass is attached. It contains the
cdrom_* functions split out from eutils.eclass. Mike says that the new
eclass could be maintained by the games team.

Please review.

As far as I can see, eutils.eclass doesn't need to inherit
portability.eclass any more, because it was only used in the cdrom
functions for get_mounts(). It would be nice if somebody could
double-check this.

(I don't include a diff for eutils.eclass, since apart from the change
in inherit it would be some 200 removal lines only.)

> - unpack_pdv() is used by one ebuild in the tree only.

Mike told me that it should stay together with unpack_makeself(),
so I won't touch this one.

Ulrich


[-- Attachment #2: cdrom.eclass --]
[-- Type: text/plain, Size: 7249 bytes --]

# Copyright 1999-2012 Gentoo Foundation
# Distributed under the terms of the GNU General Public License v2
# $Header: $

# @ECLASS: cdrom.eclass
# @MAINTAINER:
# games@gentoo.org
# @BLURB: Functions for cdrom handling
# @DESCRIPTION:
# Acquire cd(s) for those lovely cd-based emerges.  Yes, this violates
# the whole 'non-interactive' policy, but damnit I want CD support!
#
# With these cdrom functions we handle all the user interaction and
# standardize everything.  All you have to do is call cdrom_get_cds()
# and when the function returns, you can assume that the cd has been
# found at CDROM_ROOT.

inherit portability

# @FUNCTION: cdrom_get_cds
# @USAGE: <file on cd1> [file on cd2] [file on cd3] [...]
# @DESCRIPTION:
# The function will attempt to locate a cd based upon a file that is on
# the cd.  The more files you give this function, the more cds the cdrom
# functions will handle.
#
# Normally the cdrom functions will refer to the cds as 'cd #1', 'cd #2',
# etc...  If you want to give the cds better names, then just export
# the appropriate CDROM_NAME variable before calling cdrom_get_cds().
# Use CDROM_NAME for one cd, or CDROM_NAME_# for multiple cds.  You can
# also use the CDROM_NAME_SET bash array.
#
# For those multi cd ebuilds, see the cdrom_load_next_cd() function.
cdrom_get_cds() {
	# first we figure out how many cds we're dealing with by
	# the # of files they gave us
	local cdcnt=0
	local f=
	for f in "$@" ; do
		((++cdcnt))
		export CDROM_CHECK_${cdcnt}="$f"
	done
	export CDROM_TOTAL_CDS=${cdcnt}
	export CDROM_CURRENT_CD=1

	# now we see if the user gave use CD_ROOT ...
	# if they did, let's just believe them that it's correct
	if [[ -n ${CD_ROOT}${CD_ROOT_1} ]] ; then
		local var=
		cdcnt=0
		while [[ ${cdcnt} -lt ${CDROM_TOTAL_CDS} ]] ; do
			((++cdcnt))
			var="CD_ROOT_${cdcnt}"
			[[ -z ${!var} ]] && var="CD_ROOT"
			if [[ -z ${!var} ]] ; then
				eerror "You must either use just the CD_ROOT"
				eerror "or specify ALL the CD_ROOT_X variables."
				eerror "In this case, you will need" \
					"${CDROM_TOTAL_CDS} CD_ROOT_X variables."
				die "could not locate CD_ROOT_${cdcnt}"
			fi
		done
		export CDROM_ROOT=${CD_ROOT_1:-${CD_ROOT}}
		einfo "Found CD #${CDROM_CURRENT_CD} root at ${CDROM_ROOT}"
		export CDROM_SET=-1
		for f in ${CDROM_CHECK_1//:/ } ; do
			((++CDROM_SET))
			[[ -e ${CDROM_ROOT}/${f} ]] && break
		done
		export CDROM_MATCH=${f}
		return
	fi

	# User didn't help us out so lets make sure they know they can
	# simplify the whole process ...
	if [[ ${CDROM_TOTAL_CDS} -eq 1 ]] ; then
		einfo "This ebuild will need the ${CDROM_NAME:-cdrom for ${PN}}"
		echo
		einfo "If you do not have the CD, but have the data files"
		einfo "mounted somewhere on your filesystem, just export"
		einfo "the variable CD_ROOT so that it points to the"
		einfo "directory containing the files."
		echo
		einfo "For example:"
		einfo "export CD_ROOT=/mnt/cdrom"
		echo
	else
		if [[ -n ${CDROM_NAME_SET} ]] ; then
			# Translate the CDROM_NAME_SET array into CDROM_NAME_#
			cdcnt=0
			while [[ ${cdcnt} -lt ${CDROM_TOTAL_CDS} ]] ; do
				((++cdcnt))
				export CDROM_NAME_${cdcnt}="${CDROM_NAME_SET[$((${cdcnt}-1))]}"
			done
		fi

		einfo "This package will need access to ${CDROM_TOTAL_CDS} cds."
		cdcnt=0
		while [[ ${cdcnt} -lt ${CDROM_TOTAL_CDS} ]] ; do
			((++cdcnt))
			var="CDROM_NAME_${cdcnt}"
			[[ ! -z ${!var} ]] && einfo " CD ${cdcnt}: ${!var}"
		done
		echo
		einfo "If you do not have the CDs, but have the data files"
		einfo "mounted somewhere on your filesystem, just export"
		einfo "the following variables so they point to the right place:"
		einfon ""
		cdcnt=0
		while [[ ${cdcnt} -lt ${CDROM_TOTAL_CDS} ]] ; do
			((++cdcnt))
			echo -n " CD_ROOT_${cdcnt}"
		done
		echo
		einfo "Or, if you have all the files in the same place, or"
		einfo "you only have one cdrom, you can export CD_ROOT"
		einfo "and that place will be used as the same data source"
		einfo "for all the CDs."
		echo
		einfo "For example:"
		einfo "export CD_ROOT_1=/mnt/cdrom"
		echo
	fi

	export CDROM_SET=""
	export CDROM_CURRENT_CD=0
	cdrom_load_next_cd
}

# @FUNCTION: cdrom_load_next_cd
# @DESCRIPTION:
# Some packages are so big they come on multiple CDs.  When you're done
# reading files off a CD and want access to the next one, just call this
# function.  Again, all the messy details of user interaction are taken
# care of for you.  Once this returns, just read the variable CDROM_ROOT
# for the location of the mounted CD.  Note that you can only go forward
# in the CD list, so make sure you only call this function when you're
# done using the current CD.
cdrom_load_next_cd() {
	local var
	((++CDROM_CURRENT_CD))

	unset CDROM_ROOT
	var=CD_ROOT_${CDROM_CURRENT_CD}
	[[ -z ${!var} ]] && var="CD_ROOT"
	if [[ -z ${!var} ]] ; then
		var="CDROM_CHECK_${CDROM_CURRENT_CD}"
		_cdrom_locate_file_on_cd ${!var}
	else
		export CDROM_ROOT=${!var}
	fi

	einfo "Found CD #${CDROM_CURRENT_CD} root at ${CDROM_ROOT}"
}

# this is used internally by the cdrom_get_cds() and cdrom_load_next_cd()
# functions.  this should *never* be called from an ebuild.
# all it does is try to locate a give file on a cd ... if the cd isn't
# found, then a message asking for the user to insert the cdrom will be
# displayed and we'll hang out here until:
# (1) the file is found on a mounted cdrom
# (2) the user hits CTRL+C
_cdrom_locate_file_on_cd() {
	local mline=""
	local showedmsg=0 showjolietmsg=0

	while [[ -z ${CDROM_ROOT} ]] ; do
		local i=0
		local -a cdset=(${*//:/ })
		if [[ -n ${CDROM_SET} ]] ; then
			cdset=(${cdset[${CDROM_SET}]})
		fi

		while [[ -n ${cdset[${i}]} ]] ; do
			local dir=$(dirname ${cdset[${i}]})
			local file=$(basename ${cdset[${i}]})

			local point= node= fs= foo=
			while read point node fs foo ; do
				[[ " cd9660 iso9660 udf " != *" ${fs} "* ]] && \
					! [[ ${fs} == "subfs" && ",${opts}," == *",fs=cdfss,"* ]] \
					&& continue
				point=${point//\040/ }
				[[ ! -d ${point}/${dir} ]] && continue
				[[ -z $(find "${point}/${dir}" -maxdepth 1 -iname "${file}") ]] \
					&& continue
				export CDROM_ROOT=${point}
				export CDROM_SET=${i}
				export CDROM_MATCH=${cdset[${i}]}
				return
			done <<< "$(get_mounts)"

			((++i))
		done

		echo
		if [[ ${showedmsg} -eq 0 ]] ; then
			if [[ ${CDROM_TOTAL_CDS} -eq 1 ]] ; then
				if [[ -z ${CDROM_NAME} ]] ; then
					einfo "Please insert+mount the cdrom for ${PN} now !"
				else
					einfo "Please insert+mount the ${CDROM_NAME} cdrom now !"
				fi
			else
				if [[ -z ${CDROM_NAME_1} ]] ; then
					einfo "Please insert+mount cd #${CDROM_CURRENT_CD}" \
						"for ${PN} now !"
				else
					local var="CDROM_NAME_${CDROM_CURRENT_CD}"
					einfo "Please insert+mount the ${!var} cdrom now !"
				fi
			fi
			showedmsg=1
		fi
		einfo "Press return to scan for the cd again"
		einfo "or hit CTRL+C to abort the emerge."
		echo
		if [[ ${showjolietmsg} -eq 0 ]] ; then
			showjolietmsg=1
		else
			ewarn "If you are having trouble with the detection"
			ewarn "of your CD, it is possible that you do not have"
			ewarn "Joliet support enabled in your kernel.  Please"
			ewarn "check that CONFIG_JOLIET is enabled in your kernel."
			ebeep 5
		fi
		read || die "something is screwed with your system"
	done
}

Re: [gentoo-dev] Re: RFD: split out some functions from eutils.eclass?

["Paweł Hajdan, Jr."]

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

On 1/11/12 11:09 PM, Ulrich Mueller wrote:
> A draft version for a new cdrom.eclass is attached. It contains the
> cdrom_* functions split out from eutils.eclass. Mike says that the new
> eclass could be maintained by the games team.
> 
> Please review.

I think it could be worth it to add PROPERTIES="interactive" to the
eclass. Anything that uses it is interactive, right?


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

Re: [gentoo-dev] Re: RFD: split out some functions from eutils.eclass?

[Michał Górny]

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

On Wed, 11 Jan 2012 23:09:46 +0100
Ulrich Mueller <ulm@gentoo.org> wrote:

> # @ECLASS: cdrom.eclass
> # @MAINTAINER:
> # games@gentoo.org
> # @BLURB: Functions for cdrom handling
> # @DESCRIPTION:
> # Acquire cd(s) for those lovely cd-based emerges.  Yes, this violates
> # the whole 'non-interactive' policy, but damnit I want CD support!

Maybe the eclass should now set PROPERTIES=interactive?

> cdrom_get_cds() {
> 	# first we figure out how many cds we're dealing with by
> 	# the # of files they gave us
> 	local cdcnt=0
> 	local f=
> 	for f in "$@" ; do
> 		((++cdcnt))
> 		export CDROM_CHECK_${cdcnt}="$f"
> 	done
> 	export CDROM_TOTAL_CDS=${cdcnt}
> 	export CDROM_CURRENT_CD=1

Why are you exporting all that? I don't think that should be necessary
unless you subshell somewhere.

CDROM_CHECK=( "${@}" )
CDROM_TOTAL_CDS=${#} # (or ${#CDROM_CHECK[@]})

And then you can just refer to ${CDROM_CHECK[$i]} rather than using all
those ugly ${!...}.

> 	# now we see if the user gave use CD_ROOT ...
> 	# if they did, let's just believe them that it's correct
> 	if [[ -n ${CD_ROOT}${CD_ROOT_1} ]] ; then
> 		local var=
> 		cdcnt=0
> 		while [[ ${cdcnt} -lt ${CDROM_TOTAL_CDS} ]] ; do
> 			((++cdcnt))
> 			var="CD_ROOT_${cdcnt}"
> 			[[ -z ${!var} ]] && var="CD_ROOT"
> 			if [[ -z ${!var} ]] ; then
> 				eerror "You must either use just the
> CD_ROOT" eerror "or specify ALL the CD_ROOT_X variables."
> 				eerror "In this case, you will need" \
> 					"${CDROM_TOTAL_CDS} CD_ROOT_X
> variables." die "could not locate CD_ROOT_${cdcnt}"
> 			fi
> 		done
> 		export CDROM_ROOT=${CD_ROOT_1:-${CD_ROOT}}
> 		einfo "Found CD #${CDROM_CURRENT_CD} root at
> ${CDROM_ROOT}"

#1. You are using 1 in variable calls, I don't see a reason to use
longer ${CDROM_CURRENT_CD} in this one output case.

>		export CDROM_SET=-1
> 		for f in ${CDROM_CHECK_1//:/ } ; do
> 			((++CDROM_SET))

More when I get replies on these ones.

-- 
Best regards,
Michał Górny

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

Re: [gentoo-dev] Re: RFD: split out some functions from eutils.eclass?

[Ulrich Mueller]

>>>>> On Thu, 12 Jan 2012, Michał Górny wrote:

> Maybe the eclass should now set PROPERTIES=interactive?

Good idea.

>> for f in "$@" ; do
>> ((++cdcnt))
>> export CDROM_CHECK_${cdcnt}="$f"
>> done
>> export CDROM_TOTAL_CDS=${cdcnt}
>> export CDROM_CURRENT_CD=1

> Why are you exporting all that? I don't think that should be
> necessary unless you subshell somewhere.

Indeed, looks like these exports could be removed.

> CDROM_CHECK=( "${@}" )
> CDROM_TOTAL_CDS=${#} # (or ${#CDROM_CHECK[@]})

> And then you can just refer to ${CDROM_CHECK[$i]} rather than using
> all those ugly ${!...}.

I'm reluctant to make such intrusive changes. Maybe the code isn't
elegant, but it's been in production use for years. So I'd rather not
risk any breakage.

>> einfo "Found CD #${CDROM_CURRENT_CD} root at ${CDROM_ROOT}"

> #1. You are using 1 in variable calls, I don't see a reason to use
> longer ${CDROM_CURRENT_CD} in this one output case.

That's just a stylistic change with no functional difference.

> More when I get replies on these ones.

Maybe really vapier should answer your questions. These functions are
his code, I'm just splitting them out from eutils.

Ulrich

Re: [gentoo-dev] Re: RFD: split out some functions from eutils.eclass?

[Mike Frysinger]

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

On Thursday 12 January 2012 01:49:59 Michał Górny wrote:
> On Wed, 11 Jan 2012 23:09:46 +0100 Ulrich Mueller wrote:
> > cdrom_get_cds() {
> > 
> > 	# first we figure out how many cds we're dealing with by
> > 	# the # of files they gave us
> > 	local cdcnt=0
> > 	local f=
> > 	for f in "$@" ; do
> > 	
> > 		((++cdcnt))
> > 		export CDROM_CHECK_${cdcnt}="$f"
> > 	
> > 	done
> > 	export CDROM_TOTAL_CDS=${cdcnt}
> > 	export CDROM_CURRENT_CD=1
> 
> Why are you exporting all that? I don't think that should be necessary
> unless you subshell somewhere.

technically, the "export" can probably be removed.  it does help to clearly 
mark the variables that are "stateful" across multiple/different calls, but 
that's probably also done via the "CDROM_XXX" naming.  i'm indifferent.

> And then you can just refer to ${CDROM_CHECK[$i]} rather than using all
> those ugly ${!...}.

yes, i wrote this before i had a grasp of bash arrays.  however, for Ulrich's 
work, he should just be relocating code without modifying.  if we want to do 
cleanups, it'd be a follow up.  i'm sure i could rewrite quite a bit now just 
by reading it.

> > 	# now we see if the user gave use CD_ROOT ...
> > 	# if they did, let's just believe them that it's correct
> > 	if [[ -n ${CD_ROOT}${CD_ROOT_1} ]] ; then
> > 		local var=
> > 		cdcnt=0
> > 		while [[ ${cdcnt} -lt ${CDROM_TOTAL_CDS} ]] ; do
> > 			((++cdcnt))
> > 			var="CD_ROOT_${cdcnt}"
> > 			[[ -z ${!var} ]] && var="CD_ROOT"
> > 			if [[ -z ${!var} ]] ; then
> > 				eerror "You must either use just the
> > CD_ROOT" eerror "or specify ALL the CD_ROOT_X variables."
> > 				eerror "In this case, you will need" \
> > 					"${CDROM_TOTAL_CDS} CD_ROOT_X
> > variables." die "could not locate CD_ROOT_${cdcnt}"
> > 			fi
> > 		done
> > 		export CDROM_ROOT=${CD_ROOT_1:-${CD_ROOT}}
> > 		einfo "Found CD #${CDROM_CURRENT_CD} root at
> > ${CDROM_ROOT}"
> 
> #1. You are using 1 in variable calls, I don't see a reason to use
> longer ${CDROM_CURRENT_CD} in this one output case.

i don't know what you mean
-mike

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

Re: [gentoo-dev] Re: RFD: split out some functions from eutils.eclass?

[Mike Frysinger]

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

On Wednesday 11 January 2012 17:09:46 Ulrich Mueller wrote:
> >>>>> On Sat, 7 Jan 2012, Ulrich Mueller wrote:
> > - unpack_pdv() is used by one ebuild in the tree only.
> 
> Mike told me that it should stay together with unpack_makeself(),
> so I won't touch this one.

this is being addressed in a new unpacker.eclass.  i'll start a new thread 
about it.
-mike

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

Re: [gentoo-dev] Re: RFD: split out some functions from eutils.eclass?

[Michał Górny]

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

On Wed, 18 Jan 2012 06:22:24 -0500
Mike Frysinger <vapier@gentoo.org> wrote:

> > And then you can just refer to ${CDROM_CHECK[$i]} rather than using
> > all those ugly ${!...}.
> 
> yes, i wrote this before i had a grasp of bash arrays.  however, for
> Ulrich's work, he should just be relocating code without modifying.
> if we want to do cleanups, it'd be a follow up.  i'm sure i could
> rewrite quite a bit now just by reading it.

Agreed.

> > > 	# now we see if the user gave use CD_ROOT ...
> > > 	# if they did, let's just believe them that it's correct
> > > 	if [[ -n ${CD_ROOT}${CD_ROOT_1} ]] ; then
> > > 		local var=
> > > 		cdcnt=0
> > > 		while [[ ${cdcnt} -lt ${CDROM_TOTAL_CDS} ]] ; do
> > > 			((++cdcnt))
> > > 			var="CD_ROOT_${cdcnt}"
> > > 			[[ -z ${!var} ]] && var="CD_ROOT"
> > > 			if [[ -z ${!var} ]] ; then
> > > 				eerror "You must either use just
> > > the CD_ROOT" eerror "or specify ALL the CD_ROOT_X variables."
> > > 				eerror "In this case, you will
> > > need" \ "${CDROM_TOTAL_CDS} CD_ROOT_X
> > > variables." die "could not locate CD_ROOT_${cdcnt}"
> > > 			fi
> > > 		done
> > > 		export CDROM_ROOT=${CD_ROOT_1:-${CD_ROOT}}
> > > 		einfo "Found CD #${CDROM_CURRENT_CD} root at
> > > ${CDROM_ROOT}"
> > 
> > #1. You are using 1 in variable calls, I don't see a reason to use
> > longer ${CDROM_CURRENT_CD} in this one output case.
> 
> i don't know what you mean

einfo "Found CD #1 root at ..."

-- 
Best regards,
Michał Górny

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

Re: [gentoo-dev] Re: RFD: split out some functions from eutils.eclass?

[Mike Frysinger]

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

On Wednesday 18 January 2012 08:39:04 Michał Górny wrote:
> On Wed, 18 Jan 2012 06:22:24 -0500 Mike Frysinger wrote:
> > > > 	# now we see if the user gave use CD_ROOT ...
> > > > 	# if they did, let's just believe them that it's correct
> > > > 	if [[ -n ${CD_ROOT}${CD_ROOT_1} ]] ; then
> > > > 	
> > > > 		local var=
> > > > 		cdcnt=0
> > > > 		while [[ ${cdcnt} -lt ${CDROM_TOTAL_CDS} ]] ; do
> > > > 			((++cdcnt))
> > > > 			var="CD_ROOT_${cdcnt}"
> > > > 			[[ -z ${!var} ]] && var="CD_ROOT"
> > > > 			if [[ -z ${!var} ]] ; then
> > > > 				eerror "You must either use just
> > > > the CD_ROOT" eerror "or specify ALL the CD_ROOT_X variables."
> > > > 				eerror "In this case, you will
> > > > need" \ "${CDROM_TOTAL_CDS} CD_ROOT_X
> > > > variables." die "could not locate CD_ROOT_${cdcnt}"
> > > > 			fi
> > > > 		done
> > > > 		export CDROM_ROOT=${CD_ROOT_1:-${CD_ROOT}}
> > > > 		einfo "Found CD #${CDROM_CURRENT_CD} root at
> > > > ${CDROM_ROOT}"
> > > 
> > > #1. You are using 1 in variable calls, I don't see a reason to use
> > > longer ${CDROM_CURRENT_CD} in this one output case.
> > 
> > i don't know what you mean
> 
> einfo "Found CD #1 root at ..."

*shrug* i preferred the # in the output
-mike

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

Re: [gentoo-dev] Re: RFD: split out some functions from eutils.eclass?

[Michał Górny]

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

On Wed, 18 Jan 2012 09:48:59 -0500
Mike Frysinger <vapier@gentoo.org> wrote:

> On Wednesday 18 January 2012 08:39:04 Michał Górny wrote:
> > On Wed, 18 Jan 2012 06:22:24 -0500 Mike Frysinger wrote:
> > > > > 	# now we see if the user gave use CD_ROOT ...
> > > > > 	# if they did, let's just believe them that it's
> > > > > correct if [[ -n ${CD_ROOT}${CD_ROOT_1} ]] ; then
> > > > > 	
> > > > > 		local var=
> > > > > 		cdcnt=0
> > > > > 		while [[ ${cdcnt} -lt ${CDROM_TOTAL_CDS} ]] ;
> > > > > do ((++cdcnt))
> > > > > 			var="CD_ROOT_${cdcnt}"
> > > > > 			[[ -z ${!var} ]] && var="CD_ROOT"
> > > > > 			if [[ -z ${!var} ]] ; then
> > > > > 				eerror "You must either use
> > > > > just the CD_ROOT" eerror "or specify ALL the CD_ROOT_X
> > > > > variables." eerror "In this case, you will
> > > > > need" \ "${CDROM_TOTAL_CDS} CD_ROOT_X
> > > > > variables." die "could not locate CD_ROOT_${cdcnt}"
> > > > > 			fi
> > > > > 		done
> > > > > 		export CDROM_ROOT=${CD_ROOT_1:-${CD_ROOT}}
> > > > > 		einfo "Found CD #${CDROM_CURRENT_CD} root at
> > > > > ${CDROM_ROOT}"
> > > > 
> > > > #1. You are using 1 in variable calls, I don't see a reason to
> > > > use longer ${CDROM_CURRENT_CD} in this one output case.
> > > 
> > > i don't know what you mean
> > 
> > einfo "Found CD #1 root at ..."
> 
> *shrug* i preferred the # in the output

I don't get what you mean. I just say it's pointless to use
${CDROM_CURRENT_CD} here.

-- 
Best regards,
Michał Górny

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

Re: [gentoo-dev] Re: RFD: split out some functions from eutils.eclass?

[Mike Frysinger]

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

On Wednesday 18 January 2012 14:11:14 Michał Górny wrote:
> On Wed, 18 Jan 2012 09:48:59 -0500 Mike Frysinger wrote:
> > On Wednesday 18 January 2012 08:39:04 Michał Górny wrote:
> > > On Wed, 18 Jan 2012 06:22:24 -0500 Mike Frysinger wrote:
> > > > > > 	# now we see if the user gave use CD_ROOT ...
> > > > > > 	# if they did, let's just believe them that it's
> > > > > > 
> > > > > > correct if [[ -n ${CD_ROOT}${CD_ROOT_1} ]] ; then
> > > > > > 		local var=
> > > > > > 		cdcnt=0
> > > > > > 		while [[ ${cdcnt} -lt ${CDROM_TOTAL_CDS} ]] ;
> > > > > > do ((++cdcnt))
> > > > > > 			var="CD_ROOT_${cdcnt}"
> > > > > > 			[[ -z ${!var} ]] && var="CD_ROOT"
> > > > > > 			if [[ -z ${!var} ]] ; then
> > > > > > 			
> > > > > > 				eerror "You must either use
> > > > > > just the CD_ROOT" eerror "or specify ALL the CD_ROOT_X
> > > > > > variables." eerror "In this case, you will
> > > > > > need" \ "${CDROM_TOTAL_CDS} CD_ROOT_X
> > > > > > variables." die "could not locate CD_ROOT_${cdcnt}"
> > > > > > 			fi
> > > > > > 		done
> > > > > > 		export CDROM_ROOT=${CD_ROOT_1:-${CD_ROOT}}
> > > > > > 		einfo "Found CD #${CDROM_CURRENT_CD} root at
> > > > > > ${CDROM_ROOT}"
> > > > > 
> > > > > #1. You are using 1 in variable calls, I don't see a reason to
> > > > > use longer ${CDROM_CURRENT_CD} in this one output case.
> > > > 
> > > > i don't know what you mean
> > > 
> > > einfo "Found CD #1 root at ..."
> > 
> > *shrug* i preferred the # in the output
> 
> I don't get what you mean. I just say it's pointless to use
> ${CDROM_CURRENT_CD} here.

in multi cd sets, it keeps thing clearer imo at little cost
-mike

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

Re: [gentoo-dev] Re: RFD: split out some functions from eutils.eclass?

[Michał Górny]

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

On Wed, 18 Jan 2012 14:29:27 -0500
Mike Frysinger <vapier@gentoo.org> wrote:

> On Wednesday 18 January 2012 14:11:14 Michał Górny wrote:
> > On Wed, 18 Jan 2012 09:48:59 -0500 Mike Frysinger wrote:
> > > On Wednesday 18 January 2012 08:39:04 Michał Górny wrote:
> > > > On Wed, 18 Jan 2012 06:22:24 -0500 Mike Frysinger wrote:
> > > > > > > 	# now we see if the user gave use CD_ROOT ...
> > > > > > > 	# if they did, let's just believe them that it's
> > > > > > > 
> > > > > > > correct if [[ -n ${CD_ROOT}${CD_ROOT_1} ]] ; then
> > > > > > > 		local var=
> > > > > > > 		cdcnt=0
> > > > > > > 		while [[ ${cdcnt} -lt ${CDROM_TOTAL_CDS}
> > > > > > > ]] ; do ((++cdcnt))
> > > > > > > 			var="CD_ROOT_${cdcnt}"
> > > > > > > 			[[ -z ${!var} ]] && var="CD_ROOT"
> > > > > > > 			if [[ -z ${!var} ]] ; then
> > > > > > > 			
> > > > > > > 				eerror "You must either
> > > > > > > use just the CD_ROOT" eerror "or specify ALL the CD_ROOT_X
> > > > > > > variables." eerror "In this case, you will
> > > > > > > need" \ "${CDROM_TOTAL_CDS} CD_ROOT_X
> > > > > > > variables." die "could not locate CD_ROOT_${cdcnt}"
> > > > > > > 			fi
> > > > > > > 		done
> > > > > > > 		export CDROM_ROOT=${CD_ROOT_1:-${CD_ROOT}}
> > > > > > > 		einfo "Found CD #${CDROM_CURRENT_CD} root
> > > > > > > at ${CDROM_ROOT}"
> > > > > > 
> > > > > > #1. You are using 1 in variable calls, I don't see a reason
> > > > > > to use longer ${CDROM_CURRENT_CD} in this one output case.
> > > > > 
> > > > > i don't know what you mean
> > > > 
> > > > einfo "Found CD #1 root at ..."
> > > 
> > > *shrug* i preferred the # in the output
> > 
> > I don't get what you mean. I just say it's pointless to use
> > ${CDROM_CURRENT_CD} here.
> 
> in multi cd sets, it keeps thing clearer imo at little cost

Consdiering that you always use ${CD_ROOT_1}?

-- 
Best regards,
Michał Górny

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

Re: [gentoo-dev] Re: RFD: split out some functions from eutils.eclass?

[Mike Frysinger]

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

On Wednesday 18 January 2012 15:14:05 Michał Górny wrote:
> On Wed, 18 Jan 2012 14:29:27 -0500 Mike Frysinger wrote:
> > On Wednesday 18 January 2012 14:11:14 Michał Górny wrote:
> > > On Wed, 18 Jan 2012 09:48:59 -0500 Mike Frysinger wrote:
> > > > On Wednesday 18 January 2012 08:39:04 Michał Górny wrote:
> > > > > On Wed, 18 Jan 2012 06:22:24 -0500 Mike Frysinger wrote:
> > > > > > > > 	# now we see if the user gave use CD_ROOT ...
> > > > > > > > 	# if they did, let's just believe them that it's
> > > > > > > > 
> > > > > > > > correct if [[ -n ${CD_ROOT}${CD_ROOT_1} ]] ; then
> > > > > > > > 
> > > > > > > > 		local var=
> > > > > > > > 		cdcnt=0
> > > > > > > > 		while [[ ${cdcnt} -lt ${CDROM_TOTAL_CDS}
> > > > > > > > 
> > > > > > > > ]] ; do ((++cdcnt))
> > > > > > > > 
> > > > > > > > 			var="CD_ROOT_${cdcnt}"
> > > > > > > > 			[[ -z ${!var} ]] && var="CD_ROOT"
> > > > > > > > 			if [[ -z ${!var} ]] ; then
> > > > > > > > 			
> > > > > > > > 				eerror "You must either
> > > > > > > > 
> > > > > > > > use just the CD_ROOT" eerror "or specify ALL the CD_ROOT_X
> > > > > > > > variables." eerror "In this case, you will
> > > > > > > > need" \ "${CDROM_TOTAL_CDS} CD_ROOT_X
> > > > > > > > variables." die "could not locate CD_ROOT_${cdcnt}"
> > > > > > > > 
> > > > > > > > 			fi
> > > > > > > > 		
> > > > > > > > 		done
> > > > > > > > 		export CDROM_ROOT=${CD_ROOT_1:-${CD_ROOT}}
> > > > > > > > 		einfo "Found CD #${CDROM_CURRENT_CD} root
> > > > > > > > 
> > > > > > > > at ${CDROM_ROOT}"
> > > > > > > 
> > > > > > > #1. You are using 1 in variable calls, I don't see a reason
> > > > > > > to use longer ${CDROM_CURRENT_CD} in this one output case.
> > > > > > 
> > > > > > i don't know what you mean
> > > > > 
> > > > > einfo "Found CD #1 root at ..."
> > > > 
> > > > *shrug* i preferred the # in the output
> > > 
> > > I don't get what you mean. I just say it's pointless to use
> > > ${CDROM_CURRENT_CD} here.
> > 
> > in multi cd sets, it keeps thing clearer imo at little cost
> 
> Consdiering that you always use ${CD_ROOT_1}?

yes.  i want to see the disc # and path.
-mike

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

Re: [gentoo-dev] Re: RFD: split out some functions from eutils.eclass?

[Michał Górny]

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

On Wed, 18 Jan 2012 15:16:02 -0500
Mike Frysinger <vapier@gentoo.org> wrote:

> On Wednesday 18 January 2012 15:14:05 Michał Górny wrote:
> > On Wed, 18 Jan 2012 14:29:27 -0500 Mike Frysinger wrote:
> > > On Wednesday 18 January 2012 14:11:14 Michał Górny wrote:
> > > > On Wed, 18 Jan 2012 09:48:59 -0500 Mike Frysinger wrote:
> > > > > On Wednesday 18 January 2012 08:39:04 Michał Górny wrote:
> > > > > > On Wed, 18 Jan 2012 06:22:24 -0500 Mike Frysinger wrote:
> > > > > > > > > 	# now we see if the user gave use CD_ROOT ...
> > > > > > > > > 	# if they did, let's just believe them that
> > > > > > > > > it's
> > > > > > > > > 
> > > > > > > > > correct if [[ -n ${CD_ROOT}${CD_ROOT_1} ]] ; then
> > > > > > > > > 
> > > > > > > > > 		local var=
> > > > > > > > > 		cdcnt=0
> > > > > > > > > 		while [[ ${cdcnt} -lt
> > > > > > > > > ${CDROM_TOTAL_CDS}
> > > > > > > > > 
> > > > > > > > > ]] ; do ((++cdcnt))
> > > > > > > > > 
> > > > > > > > > 			var="CD_ROOT_${cdcnt}"
> > > > > > > > > 			[[ -z ${!var} ]] &&
> > > > > > > > > var="CD_ROOT" if [[ -z ${!var} ]] ; then
> > > > > > > > > 			
> > > > > > > > > 				eerror "You must
> > > > > > > > > either
> > > > > > > > > 
> > > > > > > > > use just the CD_ROOT" eerror "or specify ALL the
> > > > > > > > > CD_ROOT_X variables." eerror "In this case, you will
> > > > > > > > > need" \ "${CDROM_TOTAL_CDS} CD_ROOT_X
> > > > > > > > > variables." die "could not locate CD_ROOT_${cdcnt}"
> > > > > > > > > 
> > > > > > > > > 			fi
> > > > > > > > > 		
> > > > > > > > > 		done
> > > > > > > > > 		export
> > > > > > > > > CDROM_ROOT=${CD_ROOT_1:-${CD_ROOT}} einfo "Found CD
> > > > > > > > > #${CDROM_CURRENT_CD} root
> > > > > > > > > 
> > > > > > > > > at ${CDROM_ROOT}"
> > > > > > > > 
> > > > > > > > #1. You are using 1 in variable calls, I don't see a
> > > > > > > > reason to use longer ${CDROM_CURRENT_CD} in this one
> > > > > > > > output case.
> > > > > > > 
> > > > > > > i don't know what you mean
> > > > > > 
> > > > > > einfo "Found CD #1 root at ..."
> > > > > 
> > > > > *shrug* i preferred the # in the output
> > > > 
> > > > I don't get what you mean. I just say it's pointless to use
> > > > ${CDROM_CURRENT_CD} here.
> > > 
> > > in multi cd sets, it keeps thing clearer imo at little cost
> > 
> > Consdiering that you always use ${CD_ROOT_1}?
> 
> yes.  i want to see the disc # and path.

But isn't CDROM_CURRENT_ID always 1 there?

-- 
Best regards,
Michał Górny

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

Re: [gentoo-dev] Re: RFD: split out some functions from eutils.eclass?

[Mike Frysinger]

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

On Wednesday 18 January 2012 15:28:45 Michał Górny wrote:
> On Wed, 18 Jan 2012 15:16:02 -0500 Mike Frysinger wrote:
> > On Wednesday 18 January 2012 15:14:05 Michał Górny wrote:
> > > On Wed, 18 Jan 2012 14:29:27 -0500 Mike Frysinger wrote:
> > > > On Wednesday 18 January 2012 14:11:14 Michał Górny wrote:
> > > > > On Wed, 18 Jan 2012 09:48:59 -0500 Mike Frysinger wrote:
> > > > > > On Wednesday 18 January 2012 08:39:04 Michał Górny wrote:
> > > > > > > On Wed, 18 Jan 2012 06:22:24 -0500 Mike Frysinger wrote:
> > > > > > > > > > 	# now we see if the user gave use CD_ROOT ...
> > > > > > > > > > 	# if they did, let's just believe them that
> > > > > > > > > > 
> > > > > > > > > > it's
> > > > > > > > > > 
> > > > > > > > > > correct if [[ -n ${CD_ROOT}${CD_ROOT_1} ]] ; then
> > > > > > > > > > 
> > > > > > > > > > 		local var=
> > > > > > > > > > 		cdcnt=0
> > > > > > > > > > 		while [[ ${cdcnt} -lt
> > > > > > > > > > 
> > > > > > > > > > ${CDROM_TOTAL_CDS}
> > > > > > > > > > 
> > > > > > > > > > ]] ; do ((++cdcnt))
> > > > > > > > > > 
> > > > > > > > > > 			var="CD_ROOT_${cdcnt}"
> > > > > > > > > > 			[[ -z ${!var} ]] &&
> > > > > > > > > > 
> > > > > > > > > > var="CD_ROOT" if [[ -z ${!var} ]] ; then
> > > > > > > > > > 
> > > > > > > > > > 				eerror "You must
> > > > > > > > > > 
> > > > > > > > > > either
> > > > > > > > > > 
> > > > > > > > > > use just the CD_ROOT" eerror "or specify ALL the
> > > > > > > > > > CD_ROOT_X variables." eerror "In this case, you will
> > > > > > > > > > need" \ "${CDROM_TOTAL_CDS} CD_ROOT_X
> > > > > > > > > > variables." die "could not locate CD_ROOT_${cdcnt}"
> > > > > > > > > > 
> > > > > > > > > > 			fi
> > > > > > > > > > 		
> > > > > > > > > > 		done
> > > > > > > > > > 		export
> > > > > > > > > > 
> > > > > > > > > > CDROM_ROOT=${CD_ROOT_1:-${CD_ROOT}} einfo "Found CD
> > > > > > > > > > #${CDROM_CURRENT_CD} root
> > > > > > > > > > 
> > > > > > > > > > at ${CDROM_ROOT}"
> > > > > > > > > 
> > > > > > > > > #1. You are using 1 in variable calls, I don't see a
> > > > > > > > > reason to use longer ${CDROM_CURRENT_CD} in this one
> > > > > > > > > output case.
> > > > > > > > 
> > > > > > > > i don't know what you mean
> > > > > > > 
> > > > > > > einfo "Found CD #1 root at ..."
> > > > > > 
> > > > > > *shrug* i preferred the # in the output
> > > > > 
> > > > > I don't get what you mean. I just say it's pointless to use
> > > > > ${CDROM_CURRENT_CD} here.
> > > > 
> > > > in multi cd sets, it keeps thing clearer imo at little cost
> > > 
> > > Consdiering that you always use ${CD_ROOT_1}?
> > 
> > yes.  i want to see the disc # and path.
> 
> But isn't CDROM_CURRENT_ID always 1 there?

yes.  this func is the primer so it starts at 1, and after this, people call 
cdrom_load_next_cd which then prints out:
	einfo "Found CD #2 root at ..."
	einfo "Found CD #3 root at ..."
	einfo "Found CD #4 root at ..."
since they have the same `einfo` message structure

i could replace the variable in the first einfo with a "1", but it gains 
nothing, and imo kills the "these messages are clearly in sync between these 
two funcs".
-mike

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

Re: [gentoo-dev] Re: RFD: split out some functions from eutils.eclass?

[Michał Górny]

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

On Wed, 18 Jan 2012 15:39:24 -0500
Mike Frysinger <vapier@gentoo.org> wrote:

> On Wednesday 18 January 2012 15:28:45 Michał Górny wrote:
> > On Wed, 18 Jan 2012 15:16:02 -0500 Mike Frysinger wrote:
> > > On Wednesday 18 January 2012 15:14:05 Michał Górny wrote:
> > > > On Wed, 18 Jan 2012 14:29:27 -0500 Mike Frysinger wrote:
> > > > > On Wednesday 18 January 2012 14:11:14 Michał Górny wrote:
> > > > > > On Wed, 18 Jan 2012 09:48:59 -0500 Mike Frysinger wrote:
> > > > > > > On Wednesday 18 January 2012 08:39:04 Michał Górny wrote:
> > > > > > > > On Wed, 18 Jan 2012 06:22:24 -0500 Mike Frysinger wrote:
> > > > > > > > > > > 	# now we see if the user gave use
> > > > > > > > > > > CD_ROOT ... # if they did, let's just believe
> > > > > > > > > > > them that
> > > > > > > > > > > 
> > > > > > > > > > > it's
> > > > > > > > > > > 
> > > > > > > > > > > correct if [[ -n ${CD_ROOT}${CD_ROOT_1} ]] ; then
> > > > > > > > > > > 
> > > > > > > > > > > 		local var=
> > > > > > > > > > > 		cdcnt=0
> > > > > > > > > > > 		while [[ ${cdcnt} -lt
> > > > > > > > > > > 
> > > > > > > > > > > ${CDROM_TOTAL_CDS}
> > > > > > > > > > > 
> > > > > > > > > > > ]] ; do ((++cdcnt))
> > > > > > > > > > > 
> > > > > > > > > > > 			var="CD_ROOT_${cdcnt}"
> > > > > > > > > > > 			[[ -z ${!var} ]] &&
> > > > > > > > > > > 
> > > > > > > > > > > var="CD_ROOT" if [[ -z ${!var} ]] ; then
> > > > > > > > > > > 
> > > > > > > > > > > 				eerror "You must
> > > > > > > > > > > 
> > > > > > > > > > > either
> > > > > > > > > > > 
> > > > > > > > > > > use just the CD_ROOT" eerror "or specify ALL the
> > > > > > > > > > > CD_ROOT_X variables." eerror "In this case, you
> > > > > > > > > > > will need" \ "${CDROM_TOTAL_CDS} CD_ROOT_X
> > > > > > > > > > > variables." die "could not locate
> > > > > > > > > > > CD_ROOT_${cdcnt}"
> > > > > > > > > > > 
> > > > > > > > > > > 			fi
> > > > > > > > > > > 		
> > > > > > > > > > > 		done
> > > > > > > > > > > 		export
> > > > > > > > > > > 
> > > > > > > > > > > CDROM_ROOT=${CD_ROOT_1:-${CD_ROOT}} einfo "Found
> > > > > > > > > > > CD #${CDROM_CURRENT_CD} root
> > > > > > > > > > > 
> > > > > > > > > > > at ${CDROM_ROOT}"
> > > > > > > > > > 
> > > > > > > > > > #1. You are using 1 in variable calls, I don't see a
> > > > > > > > > > reason to use longer ${CDROM_CURRENT_CD} in this one
> > > > > > > > > > output case.
> > > > > > > > > 
> > > > > > > > > i don't know what you mean
> > > > > > > > 
> > > > > > > > einfo "Found CD #1 root at ..."
> > > > > > > 
> > > > > > > *shrug* i preferred the # in the output
> > > > > > 
> > > > > > I don't get what you mean. I just say it's pointless to use
> > > > > > ${CDROM_CURRENT_CD} here.
> > > > > 
> > > > > in multi cd sets, it keeps thing clearer imo at little cost
> > > > 
> > > > Consdiering that you always use ${CD_ROOT_1}?
> > > 
> > > yes.  i want to see the disc # and path.
> > 
> > But isn't CDROM_CURRENT_ID always 1 there?
> 
> yes.  this func is the primer so it starts at 1, and after this,
> people call cdrom_load_next_cd which then prints out:
> 	einfo "Found CD #2 root at ..."
> 	einfo "Found CD #3 root at ..."
> 	einfo "Found CD #4 root at ..."
> since they have the same `einfo` message structure
> 
> i could replace the variable in the first einfo with a "1", but it
> gains nothing, and imo kills the "these messages are clearly in sync
> between these two funcs".

Then please clearly use the ${!...} magic to get CD_ROOT_1 rather than
mixing hardcoded '1' and $CDROM_CURRENT_ID.

-- 
Best regards,
Michał Górny

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

Re: [gentoo-dev] Re: RFD: split out some functions from eutils.eclass?

[Mike Frysinger]

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

On Wednesday 18 January 2012 16:17:50 Michał Górny wrote:
> On Wed, 18 Jan 2012 15:39:24 -0500 Mike Frysinger wrote:
> > yes.  this func is the primer so it starts at 1, and after this,
> > 
> > people call cdrom_load_next_cd which then prints out:
> > 	einfo "Found CD #2 root at ..."
> > 	einfo "Found CD #3 root at ..."
> > 	einfo "Found CD #4 root at ..."
> > 
> > since they have the same `einfo` message structure
> > 
> > i could replace the variable in the first einfo with a "1", but it
> > gains nothing, and imo kills the "these messages are clearly in sync
> > between these two funcs".
> 
> Then please clearly use the ${!...} magic to get CD_ROOT_1 rather than
> mixing hardcoded '1' and $CDROM_CURRENT_ID.

pretty sure that's independent of the `einfo` output

i'm probably not going to bother looking for feedback on the code style until 
it gets rewritten in arrays
-mike

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