[gentoo-dev] RFC: Update to elisp-common.eclass

[gentoo-dev] RFC: Update to elisp-common.eclass

[Ulrich Mueller]

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

The patch below will introduce two changes to function
elisp-site-regen (which is called in postinst and postrm phases):

1. Site-init files of Elisp packages will only be looked for in
   the site-gentoo.d subdirectory, but no longer in the main
   /usr/share/emacs/site-lisp dir. The elisp-site-file-install
   function installs them in the former location since 2007.

   I am not aware of any ebuild that would still install its site-init
   file in the top-level site-lisp dir (bypassing the proper eclass
   function). Also emacs-updater warns about this since a long time.
   However, if your ebuild does this, now is the time for changing it.

2. Some additional error checks added. Since callers usually don't
   test for the function's return status, die on errors, instead of
   returning bad exit status.

Please review. I intend to commit the changes in a few days from now.

Ulrich

--- a/eclass/elisp-common.eclass
+++ b/eclass/elisp-common.eclass
@@ -1,4 +1,4 @@
-# Copyright 1999-2013 Gentoo Foundation
+# Copyright 1999-2014 Gentoo Foundation
 # Distributed under the terms of the GNU General Public License v2
 # $Header: $
 #
@@ -349,39 +349,27 @@ elisp-site-file-install() {
 
 elisp-site-regen() {
 	local sitelisp=${ROOT}${EPREFIX}${SITELISP}
-	local sf i null="" page=$'\f'
+	local sf i ret=0 null="" page=$'\f'
 	local -a sflist
 
-	if [[ ! -d ${sitelisp} ]]; then
-		eerror "elisp-site-regen: Directory ${sitelisp} does not exist"
-		return 1
-	fi
-
-	if [[ ! -d ${T} ]]; then
-		eerror "elisp-site-regen: Temporary directory ${T} does not exist"
-		return 1
-	fi
-
 	if [[ ${EBUILD_PHASE} = *rm && ! -e ${sitelisp}/site-gentoo.el ]]; then
 		ewarn "Refusing to create site-gentoo.el in ${EBUILD_PHASE} phase."
 		return 0
 	fi
 
+	[[ -d ${sitelisp} ]] \
+		|| die "elisp-site-regen: Directory ${sitelisp} does not exist"
+
+	[[ -d ${T} ]] \
+		|| die "elisp-site-regen: Temporary directory ${T} does not exist"
+
 	ebegin "Regenerating site-gentoo.el for GNU Emacs (${EBUILD_PHASE})"
 
-	for sf in "${sitelisp}"/[0-9][0-9]*-gentoo.el \
-		"${sitelisp}"/site-gentoo.d/[0-9][0-9]*.el
-	do
-		[[ -r ${sf} ]] || continue
-		# sort files by their basename. straight insertion sort.
-		for ((i=${#sflist[@]}; i>0; i--)); do
-			[[ ${sf##*/} < ${sflist[i-1]##*/} ]] || break
-			sflist[i]=${sflist[i-1]}
-		done
-		sflist[i]=${sf}
+	for sf in "${sitelisp}"/site-gentoo.d/[0-9][0-9]*.el; do
+		[[ -r ${sf} ]] && sflist+=("${sf}")
 	done
 
-	cat <<-EOF >"${T}"/site-gentoo.el
+	cat <<-EOF >"${T}"/site-gentoo.el || ret=$?
 	;;; site-gentoo.el --- site initialisation for Gentoo-installed packages
 
 	;;; Commentary:
@@ -391,8 +379,8 @@ elisp-site-regen() {
 	;;; Code:
 	EOF
 	# Use sed instead of cat here, since files may miss a trailing newline.
-	sed '$q' "${sflist[@]}" </dev/null >>"${T}"/site-gentoo.el
-	cat <<-EOF >>"${T}"/site-gentoo.el
+	sed '$q' "${sflist[@]}" </dev/null >>"${T}"/site-gentoo.el || ret=$?
+	cat <<-EOF >>"${T}"/site-gentoo.el || ret=$?
 
 	${page}
 	(provide 'site-gentoo)
@@ -405,7 +393,10 @@ elisp-site-regen() {
 	;;; site-gentoo.el ends here
 	EOF
 
-	if cmp -s "${sitelisp}"/site-gentoo.el "${T}"/site-gentoo.el; then
+	if [[ ${ret} -ne 0 ]]; then
+		eend ${ret} "elisp-site-regen: Writing site-gentoo.el failed."
+		die
+	elif cmp -s "${sitelisp}"/site-gentoo.el "${T}"/site-gentoo.el; then
 		# This prevents outputting unnecessary text when there
 		# was actually no change.
 		# A case is a remerge where we have doubled output.
@@ -414,7 +405,7 @@ elisp-site-regen() {
 		einfo "... no changes."
 	else
 		mv "${T}"/site-gentoo.el "${sitelisp}"/site-gentoo.el
-		eend
+		eend $? "elisp-site-regen: Replacing site-gentoo.el failed" || die
 		case ${#sflist[@]} in
 			0) [[ ${PN} = emacs-common-gentoo ]] \
 				|| ewarn "... Huh? No site initialisation files found." ;;

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

Re: [gentoo-dev] RFC: Update to elisp-common.eclass

[Jeroen Roovers]

On Sun, 18 May 2014 20:13:07 +0200
Ulrich Mueller <ulm@gentoo.org> wrote:

Are you sure this is useful?

> --- a/eclass/elisp-common.eclass
> +++ b/eclass/elisp-common.eclass
> @@ -1,4 +1,4 @@
> -# Copyright 1999-2013 Gentoo Foundation
> +# Copyright 1999-2014 Gentoo Foundation
>  # Distributed under the terms of the GNU General Public License v2
>  # $Header: $
>  #
> @@ -349,39 +349,27 @@ elisp-site-file-install() {
>  
>  elisp-site-regen() {
[...]
> -	if [[ ! -d ${T} ]]; then
> -		eerror "elisp-site-regen: Temporary directory ${T}
> does not exist"
> -		return 1
> -	fi

"The package manager must define the following environment variables.

[...]

"T [...]The full path to a temporary directory for use by the
ebuild."[1]


Regards,
     jer


[1] http://dev.gentoo.org/~ulm/pms/5/pms.html#x1-11800011.1

Re: [gentoo-dev] RFC: Update to elisp-common.eclass

[Ulrich Mueller]

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

>>>>> On Mon, 19 May 2014, Jeroen Roovers wrote:

> Are you sure this is useful?

>> elisp-site-regen() {
> [...]
>> -	if [[ ! -d ${T} ]]; then
>> -		eerror "elisp-site-regen: Temporary directory ${T} does not exist"
>> -		return 1
>> -	fi

> "The package manager must define the following environment variables.

> [...]

> "T [...]The full path to a temporary directory for use by the
> ebuild."[1]

So is the theory. The test exists because this wasn't always the case
in practice [2].

I'd rather leave it there, because if T is empty then files will be
written to / instead. There is no sandbox in post{inst,rm}, so being
extra careful won't harm.

Ulrich

> [1] http://dev.gentoo.org/~ulm/pms/5/pms.html#x1-11800011.1
[2] http://paludis.exherbo.org/trac/ticket/517

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

Re: [gentoo-dev] RFC: Update to elisp-common.eclass

[Alex Xu]

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

On 18/05/14 02:13 PM, Ulrich Mueller wrote:
>  	if [[ ${EBUILD_PHASE} = *rm && ! -e ${sitelisp}/site-gentoo.el ]]; then
>  		ewarn "Refusing to create site-gentoo.el in ${EBUILD_PHASE} phase."
>  		return 0
>  	fi
>  
> +	[[ -d ${sitelisp} ]] \
> +		|| die "elisp-site-regen: Directory ${sitelisp} does not exist"
> +
> +	[[ -d ${T} ]] \
> +		|| die "elisp-site-regen: Temporary directory ${T} does not exist"
> +
!qefs

if ROOT, EPREFIX, or SITELISP has whitespace, this will throw a syntax
error.


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

Re: [gentoo-dev] RFC: Update to elisp-common.eclass

[Ulrich Mueller]

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

>>>>> On Mon, 19 May 2014, Alex Xu wrote:

> On 18/05/14 02:13 PM, Ulrich Mueller wrote:
>> if [[ ${EBUILD_PHASE} = *rm && ! -e ${sitelisp}/site-gentoo.el ]]; then
>> ewarn "Refusing to create site-gentoo.el in ${EBUILD_PHASE} phase."
>> return 0
>> fi
>> 
>> +	[[ -d ${sitelisp} ]] \
>> +		|| die "elisp-site-regen: Directory ${sitelisp} does not exist"
>> +
>> +	[[ -d ${T} ]] \
>> +		|| die "elisp-site-regen: Temporary directory ${T} does not exist"
>> +
> !qefs

> if ROOT, EPREFIX, or SITELISP has whitespace, this will throw a syntax
> error.

Where? In the snippet you quoted, I don't see where whitespace in
${sitelisp} could cause any problems.

"Word splitting and filename expansion are not performed on the words
between the `[[' and `]]'" -- GNU Bash Reference Manual

Ulrich

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

Re: [gentoo-dev] RFC: Update to elisp-common.eclass

[Alex Xu]

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

On 19/05/14 07:17 AM, Ulrich Mueller wrote:
>>>>>> On Mon, 19 May 2014, Alex Xu wrote:
> 
>> On 18/05/14 02:13 PM, Ulrich Mueller wrote:
>>> if [[ ${EBUILD_PHASE} = *rm && ! -e ${sitelisp}/site-gentoo.el ]]; then
>>> ewarn "Refusing to create site-gentoo.el in ${EBUILD_PHASE} phase."
>>> return 0
>>> fi
>>>
>>> +	[[ -d ${sitelisp} ]] \
>>> +		|| die "elisp-site-regen: Directory ${sitelisp} does not exist"
>>> +
>>> +	[[ -d ${T} ]] \
>>> +		|| die "elisp-site-regen: Temporary directory ${T} does not exist"
>>> +
>> !qefs
> 
>> if ROOT, EPREFIX, or SITELISP has whitespace, this will throw a syntax
>> error.
> 
> Where? In the snippet you quoted, I don't see where whitespace in
> ${sitelisp} could cause any problems.
> 
> "Word splitting and filename expansion are not performed on the words
> between the `[[' and `]]'" -- GNU Bash Reference Manual
> 
> Ulrich
> 

/me ponders

hm...

never mind, I was mistaken.


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