public inbox for gentoo-dev@lists.gentoo.org
 help / color / mirror / Atom feed
* [gentoo-dev] [PATCH eutils] Move remove_libtool_files() from autotools-utils for wider use.
@ 2012-05-28  7:58 Michał Górny
  2012-05-28  9:03 ` Pacho Ramos
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Michał Górny @ 2012-05-28  7:58 UTC (permalink / raw
  To: gentoo-dev; +Cc: Michał Górny

As autotools-utils exports phase functions, it will be better if
remove_libtool_files() functions would be somewhere else.
---
 eutils.eclass |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/eutils.eclass b/eutils.eclass
index c88ef35..fb92256 100644
--- a/eutils.eclass
+++ b/eutils.eclass
@@ -1330,6 +1330,74 @@ makeopts_jobs() {
 	echo ${jobs:-1}
 }
 
+# @FUNCTION: remove_libtool_files
+# @USAGE: [all]
+# @DESCRIPTION:
+# Determines unnecessary libtool files (.la) and libtool static archives (.a),
+# and removes them from installation image.
+#
+# To unconditionally remove all libtool files, pass 'all' as an argument.
+# Otherwise, libtool archives required for static linking will be preserved.
+remove_libtool_files() {
+	debug-print-function ${FUNCNAME} "$@"
+	local removing_all
+	[[ ${#} -le 1 ]] || die "Invalid number of args to ${FUNCNAME}()"
+	if [[ ${#} -eq 1 ]]; then
+		case "${1}" in
+			all)
+				removing_all=1
+				;;
+			*)
+				die "Invalid argument to ${FUNCNAME}(): ${1}"
+		esac
+	fi
+
+	local pc_libs=()
+	if [[ ! ${removing_all} ]]; then
+		local arg
+		for arg in $(find "${D}" -name '*.pc' -exec \
+					sed -n -e 's;^Libs:;;p' {} +); do
+			[[ ${arg} == -l* ]] && pc_libs+=(lib${arg#-l}.la)
+		done
+	fi
+
+	local f
+	find "${D}" -type f -name '*.la' -print0 | while read -r -d '' f; do
+		local shouldnotlink=$(sed -ne '/^shouldnotlink=yes$/p' "${f}")
+		local archivefile=${f/%.la/.a}
+		[[ "${f}" != "${archivefile}" ]] || die 'regex sanity check failed'
+
+		# Remove static libs we're not supposed to link against.
+		if [[ ${shouldnotlink} ]]; then
+			einfo "Removing unnecessary ${archivefile#${D%/}}"
+			rm -f "${archivefile}" || die
+			# The .la file may be used by a module loader, so avoid removing it
+			# unless explicitly requested.
+			[[ ${removing_all} ]] || continue
+		fi
+
+		# Remove .la files when:
+		# - user explicitly wants us to remove all .la files,
+		# - respective static archive doesn't exist,
+		# - they are covered by a .pc file already,
+		# - they don't provide any new information (no libs & no flags).
+		local removing
+		if [[ ${removing_all} ]]; then removing='forced'
+		elif [[ ! -f ${archivefile} ]]; then removing='no static archive'
+		elif has "$(basename "${f}")" "${pc_libs[@]}"; then
+			removing='covered by .pc'
+		elif [[ ! $(sed -n -e \
+			"s/^\(dependency_libs\|inherited_linker_flags\)='\(.*\)'$/\2/p" \
+			"${f}") ]]; then removing='no libs & flags'
+		fi
+
+		if [[ ${removing} ]]; then
+			einfo "Removing unnecessary ${f#${D%/}} (${removing})"
+			rm -f "${f}" || die
+		fi
+	done
+}
+
 check_license() { die "you no longer need this as portage supports ACCEPT_LICENSE itself"; }
 
 fi
-- 
1.7.10.2




^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [gentoo-dev] [PATCH eutils] Move remove_libtool_files() from autotools-utils for wider use.
  2012-05-28  7:58 [gentoo-dev] [PATCH eutils] Move remove_libtool_files() from autotools-utils for wider use Michał Górny
@ 2012-05-28  9:03 ` Pacho Ramos
  2012-05-29 13:50 ` [gentoo-dev] " Steven J Long
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 13+ messages in thread
From: Pacho Ramos @ 2012-05-28  9:03 UTC (permalink / raw
  To: gentoo-dev

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

El lun, 28-05-2012 a las 09:58 +0200, Michał Górny escribió:
> As autotools-utils exports phase functions, it will be better if
> remove_libtool_files() functions would be somewhere else.
> ---
>  eutils.eclass |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/eutils.eclass b/eutils.eclass
> index c88ef35..fb92256 100644
> --- a/eutils.eclass
> +++ b/eutils.eclass
> @@ -1330,6 +1330,74 @@ makeopts_jobs() {
>  	echo ${jobs:-1}
>  }
>  
> +# @FUNCTION: remove_libtool_files
> +# @USAGE: [all]
> +# @DESCRIPTION:
> +# Determines unnecessary libtool files (.la) and libtool static archives (.a),
> +# and removes them from installation image.
> +#
> +# To unconditionally remove all libtool files, pass 'all' as an argument.
> +# Otherwise, libtool archives required for static linking will be preserved.
> +remove_libtool_files() {
> +	debug-print-function ${FUNCNAME} "$@"
> +	local removing_all
> +	[[ ${#} -le 1 ]] || die "Invalid number of args to ${FUNCNAME}()"
> +	if [[ ${#} -eq 1 ]]; then
> +		case "${1}" in
> +			all)
> +				removing_all=1
> +				;;
> +			*)
> +				die "Invalid argument to ${FUNCNAME}(): ${1}"
> +		esac
> +	fi
> +
> +	local pc_libs=()
> +	if [[ ! ${removing_all} ]]; then
> +		local arg
> +		for arg in $(find "${D}" -name '*.pc' -exec \
> +					sed -n -e 's;^Libs:;;p' {} +); do
> +			[[ ${arg} == -l* ]] && pc_libs+=(lib${arg#-l}.la)
> +		done
> +	fi
> +
> +	local f
> +	find "${D}" -type f -name '*.la' -print0 | while read -r -d '' f; do
> +		local shouldnotlink=$(sed -ne '/^shouldnotlink=yes$/p' "${f}")
> +		local archivefile=${f/%.la/.a}
> +		[[ "${f}" != "${archivefile}" ]] || die 'regex sanity check failed'
> +
> +		# Remove static libs we're not supposed to link against.
> +		if [[ ${shouldnotlink} ]]; then
> +			einfo "Removing unnecessary ${archivefile#${D%/}}"
> +			rm -f "${archivefile}" || die
> +			# The .la file may be used by a module loader, so avoid removing it
> +			# unless explicitly requested.
> +			[[ ${removing_all} ]] || continue
> +		fi
> +
> +		# Remove .la files when:
> +		# - user explicitly wants us to remove all .la files,
> +		# - respective static archive doesn't exist,
> +		# - they are covered by a .pc file already,
> +		# - they don't provide any new information (no libs & no flags).
> +		local removing
> +		if [[ ${removing_all} ]]; then removing='forced'
> +		elif [[ ! -f ${archivefile} ]]; then removing='no static archive'
> +		elif has "$(basename "${f}")" "${pc_libs[@]}"; then
> +			removing='covered by .pc'
> +		elif [[ ! $(sed -n -e \
> +			"s/^\(dependency_libs\|inherited_linker_flags\)='\(.*\)'$/\2/p" \
> +			"${f}") ]]; then removing='no libs & flags'
> +		fi
> +
> +		if [[ ${removing} ]]; then
> +			einfo "Removing unnecessary ${f#${D%/}} (${removing})"
> +			rm -f "${f}" || die
> +		fi
> +	done
> +}
> +
>  check_license() { die "you no longer need this as portage supports ACCEPT_LICENSE itself"; }
>  
>  fi

+1

This was the main reason for me still doing manually cleaning over using
this function 

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [gentoo-dev] Re: [PATCH eutils] Move remove_libtool_files() from autotools-utils for wider use.
  2012-05-28  7:58 [gentoo-dev] [PATCH eutils] Move remove_libtool_files() from autotools-utils for wider use Michał Górny
  2012-05-28  9:03 ` Pacho Ramos
@ 2012-05-29 13:50 ` Steven J Long
  2012-05-29 14:26   ` Steven J Long
  2012-05-30 17:16   ` Michał Górny
  2012-05-30 21:19 ` [gentoo-dev] " Mike Frysinger
  2012-06-05  2:46 ` [gentoo-dev] Re: [PATCH eutils] Move remove_libtool_files() from autotools-utils for wider use Ryan Hill
  3 siblings, 2 replies; 13+ messages in thread
From: Steven J Long @ 2012-05-29 13:50 UTC (permalink / raw
  To: gentoo-dev

Michał Górny wrote:

> +  find "${D}" -type f -name '*.la' -print0 | while read -r -d '' f; 
..
> +	rm -f "${f}" || die
..
> +  done

Don't pipe to read like that; it means the final command is in a subshell 
and "die is /not/ guaranteed to work correctly if called from a subshell 
environment."[1]

More seriously, the script doesn't actually get the correct filenames, 
despite being written to handle any filename.
eg:
$ touch $'  foo bar \n\t  '
$ while read -r -d '' f; do echo "'$f'"; done < <(find . -type f -print0)  
'./  foo bar'

You do it like this:

while read -rd ''; do
   f=$REPLY;
   ..
done < <(find "$D" -type f -name '*.la' -print0)

eg:
$ while read -rd ''; do f=$REPLY; echo "'$f'"; done < <(find . -type f -
print0)                                                                                           
'./  foo bar                                                                                     
          '

Or use: while IFS= read -rd '' f; do .. if you prefer.
See: help read # in a terminal.

It's called 'Process Substitution' if anyone wants to read about it in
man bash. The classic example with find is to get the list in an array:
arr=()
while read -rd ''; do
  arr+=("$REPLY")
done < <(find "$dir" -type f .. -print0)

(perhaps conditionally though that's usually better done within find
 which can later be handled on a per-file basis, or passed to:
foo "${arr[@]}"

..or if you just want to know whether there is a matching file:
if read -rd '' < <(find . -type f -print0); then
   something matched
else nothing did
fi

They're both things I came up with a few years ago when I was learning
from #bash, which you are in dire need of, based on reading git-2.eclass.

[1] http://dev.gentoo.org/~ulm/pms/head/pms.html#x1-12600011.3.3
(11.3.3.6)
-- 
#friendly-coders -- We're friendly, but we're not /that/ friendly ;-)





^ permalink raw reply	[flat|nested] 13+ messages in thread

* [gentoo-dev] Re: [PATCH eutils] Move remove_libtool_files() from autotools-utils for wider use.
  2012-05-29 13:50 ` [gentoo-dev] " Steven J Long
@ 2012-05-29 14:26   ` Steven J Long
  2012-05-30 17:16   ` Michał Górny
  1 sibling, 0 replies; 13+ messages in thread
From: Steven J Long @ 2012-05-29 14:26 UTC (permalink / raw
  To: gentoo-dev

Steven J Long wrote:
> More seriously, the script doesn't actually get the correct filenames,
> despite being written to handle any filename.

This doesn't actually apply in this case with -name '*.la' but it still
looks wrong, and implies one doesn't really grok the usage. *shrug*

-- 
#friendly-coders -- We're friendly, but we're not /that/ friendly ;-)





^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [gentoo-dev] Re: [PATCH eutils] Move remove_libtool_files() from autotools-utils for wider use.
  2012-05-29 13:50 ` [gentoo-dev] " Steven J Long
  2012-05-29 14:26   ` Steven J Long
@ 2012-05-30 17:16   ` Michał Górny
  1 sibling, 0 replies; 13+ messages in thread
From: Michał Górny @ 2012-05-30 17:16 UTC (permalink / raw
  To: gentoo-dev; +Cc: slong

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

On Tue, 29 May 2012 14:50:19 +0100
Steven J Long <slong@rathaus.eclipse.co.uk> wrote:

> Michał Górny wrote:
> 
> > +  find "${D}" -type f -name '*.la' -print0 | while read -r -d ''
> > f; 
> ..
> > +	rm -f "${f}" || die
> ..
> > +  done
> 
> Don't pipe to read like that; it means the final command is in a
> subshell and "die is /not/ guaranteed to work correctly if called
> from a subshell environment."[1]

Didn't we actually change that in the past? I think I'm recalling
something like that...

> More seriously, the script doesn't actually get the correct
> filenames, despite being written to handle any filename.
> eg:
> $ touch $'  foo bar \n\t  '
> $ while read -r -d '' f; do echo "'$f'"; done < <(find . -type f
> -print0) './  foo bar'
> 
> You do it like this:
> 
> while read -rd ''; do
>    f=$REPLY;
>    ..
> done < <(find "$D" -type f -name '*.la' -print0)

Thanks.

-- 
Best regards,
Michał Górny

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [gentoo-dev] [PATCH eutils] Move remove_libtool_files() from autotools-utils for wider use.
  2012-05-28  7:58 [gentoo-dev] [PATCH eutils] Move remove_libtool_files() from autotools-utils for wider use Michał Górny
  2012-05-28  9:03 ` Pacho Ramos
  2012-05-29 13:50 ` [gentoo-dev] " Steven J Long
@ 2012-05-30 21:19 ` Mike Frysinger
  2012-05-31  5:46   ` Michał Górny
  2012-05-31 12:55   ` [gentoo-dev] [PATCH eutils] Introduce prune_libtool_files() Michał Górny
  2012-06-05  2:46 ` [gentoo-dev] Re: [PATCH eutils] Move remove_libtool_files() from autotools-utils for wider use Ryan Hill
  3 siblings, 2 replies; 13+ messages in thread
From: Mike Frysinger @ 2012-05-30 21:19 UTC (permalink / raw
  To: gentoo-dev

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

On Monday 28 May 2012 03:58:56 Michał Górny wrote:
> +# @FUNCTION: remove_libtool_files

"preen_libtool_files" might be better.  after all, you aren't just removing 
them all.

> +# @USAGE: [all]

this is incorrect.  the usage is:
	<all | files to remove>

> +	[[ ${#} -le 1 ]] || die "Invalid number of args to ${FUNCNAME}()"
> +	if [[ ${#} -eq 1 ]]; then
> +		case "${1}" in
> +			all)
> +				removing_all=1
> +				;;
> +			*)
> +				die "Invalid argument to ${FUNCNAME}(): ${1}"
> +		esac
> +	fi

personally i don't think the internal @ $ 0-9 # variables should get braces 
unless absolutely necessary

the *) case is missing a ";;" ... yes, it's not required in the last case 
statement, but that's easy to bite people, so i prefer to make it explicit

> +	if [[ ! ${removing_all} ]]; then

i know you don't like specifying -z/-n but instead relying on the implicit -n, 
but i find it less clear, especially for people who aren't aware of the exact 
semantics

> +		local arg
> +		for arg in $(find "${D}" -name '*.pc' -exec \
> +					sed -n -e 's;^Libs:;;p' {} +); do
> +			[[ ${arg} == -l* ]] && pc_libs+=(lib${arg#-l}.la)
> +		done

let sed do the processing:
pc_libs=(
	$(find "${D}" -name '*.pc' \
		-exec sed -n -e '/^Libs:/{s|^[^:]*:||;s: :\n:g;p}' {} + | \
		sed -n '/^-l/{s:^-l:lib:;s:$:.la:;p}')
)

however, i'd point out that parsing these files directly doesn't actually work.  
some .pc files use variables in the filename which isn't expanded by using sed.  
thus your only recourse is to let pkg-config expand things for you.

$ grep '^Libs:.*-l[^ ]*[$]' /usr/lib64/pkgconfig/*.pc
/usr/lib64/pkgconfig/apr-1.pc:Libs: -L${libdir} -lapr-${APR_MAJOR_VERSION} ...
/usr/lib64/pkgconfig/gtk+-2.0.pc:Libs: -L${libdir} -lgtk-${target}-2.0

so maybe something like:
	local pc
	while read -rd '' pc ; do
		sed -e '/^Requires:/d' "${pc}" > "${T}/${pc##*/}"
		pc_libs+=(
			$($(tc-getPKG_CONFIG) --libs "${T}"/${pc##*/}" | \
				tr ' ' '\n' | \
				sed -n '/^-l/{s:^-l:lib:;s:$:.la:;p}')
		)
	done < <(find "${D}" -name '*.pc' -type f -print0)
	pc_libs=( $(printf '%s\n' "${pc_libs[@]}") | sort -u )

although, since we don't call die or anything, we can pipeline it to speed 
things up a bit:
	pc_libs=( $(
		tpc="${T}/.pc"
		find "${D}" -name '*.pc' -type f | \
		while read pc ; do
			sed -e '/^Requires:/d' "${pc}" > "${tpc}"
			$(tc-getPKG_CONFIG) --libs "${tpc}"
		done | tr ' ' '\n' | sort -u | \
		sed -n '/^-l/{s:^-l:lib:;s:$:.la:;p}'
		rm -f "${tpc}"
	) )

> +		local archivefile=${f/%.la/.a}

remove the suffix and it'll be faster i think:
	local archivefile="${f%.la}.a"

> +		[[ "${f}" != "${archivefile}" ]] || die 'regex sanity check failed'

no need to quote the ${f}, but eh

> +			rm -f "${archivefile}" || die

`rm -f` almost never fails.  in the edge cases where it does, you've got 
bigger problems.

> +		if [[ ${removing_all} ]]; then removing='forced'
> +		elif [[ ! -f ${archivefile} ]]; then removing='no static archive'

unwrap these

> +		elif has "$(basename "${f}")" "${pc_libs[@]}"; then

use ${f##*/} rather than `basename`

> +			removing='covered by .pc'
> +		elif [[ ! $(sed -n -e \
> +			"s/^\(dependency_libs\|inherited_linker_flags\)='\(.*\)'$/\2/p" \
> +			"${f}") ]]; then removing='no libs & flags'

unwrap that body, and use -r rather than escaping the (|) chars
-mike

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [gentoo-dev] [PATCH eutils] Move remove_libtool_files() from autotools-utils for wider use.
  2012-05-30 21:19 ` [gentoo-dev] " Mike Frysinger
@ 2012-05-31  5:46   ` Michał Górny
  2012-05-31  6:09     ` Mike Frysinger
  2012-05-31 12:55   ` [gentoo-dev] [PATCH eutils] Introduce prune_libtool_files() Michał Górny
  1 sibling, 1 reply; 13+ messages in thread
From: Michał Górny @ 2012-05-31  5:46 UTC (permalink / raw
  To: gentoo-dev; +Cc: vapier

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

On Wed, 30 May 2012 17:19:49 -0400
Mike Frysinger <vapier@gentoo.org> wrote:

> On Monday 28 May 2012 03:58:56 Michał Górny wrote:
> > +# @USAGE: [all]
> 
> this is incorrect.  the usage is:
> 	<all | files to remove>

No, it's perfectly valid. Moreover, it even explains what the function
actually does rather than your imagination.

But if we're not interested in keeping it compatible, I'd probably
start with making it '--all'.

> > +		local arg
> > +		for arg in $(find "${D}" -name '*.pc' -exec \
> > +					sed -n -e 's;^Libs:;;p' {}
> > +); do
> > +			[[ ${arg} == -l* ]] &&
> > pc_libs+=(lib${arg#-l}.la)
> > +		done
> 
> let sed do the processing:
> pc_libs=(
> 	$(find "${D}" -name '*.pc' \
> 		-exec sed -n -e '/^Libs:/{s|^[^:]*:||;s: :\n:g;p}' {}
> + | \ sed -n '/^-l/{s:^-l:lib:;s:$:.la:;p}')
> )

Nope. My poor eyes are too weak to disband all those magical characters.

> however, i'd point out that parsing these files directly doesn't
> actually work. some .pc files use variables in the filename which
> isn't expanded by using sed. thus your only recourse is to let
> pkg-config expand things for you.

Right. I'll have to think about this a little.

> although, since we don't call die or anything, we can pipeline it to
> speed things up a bit:
> 	pc_libs=( $(
> 		tpc="${T}/.pc"
> 		find "${D}" -name '*.pc' -type f | \
> 		while read pc ; do
> 			sed -e '/^Requires:/d' "${pc}" > "${tpc}"
> 			$(tc-getPKG_CONFIG) --libs "${tpc}"
> 		done | tr ' ' '\n' | sort -u | \
> 		sed -n '/^-l/{s:^-l:lib:;s:$:.la:;p}'
> 		rm -f "${tpc}"
> 	) )

Could you remind me, please, what performance-critical use of this
function does justify making it so harsh?

> > +		local archivefile=${f/%.la/.a}
> 
> remove the suffix and it'll be faster i think:
> 	local archivefile="${f%.la}.a"

Hmm, it's indeed inconsistent.

> > +		[[ "${f}" != "${archivefile}" ]] || die 'regex
> > sanity check failed'
> 
> no need to quote the ${f}, but eh

Yep.

> > +			rm -f "${archivefile}" || die
> 
> `rm -f` almost never fails.  in the edge cases where it does, you've
> got bigger problems.

And that problem is good enough to die here.

> > +		elif has "$(basename "${f}")" "${pc_libs[@]}"; then
> 
> use ${f##*/} rather than `basename`

Hmm, yes. I'd split it out to a sep var as I see you used it already
more than once.

> > +			removing='covered by .pc'
> > +		elif [[ ! $(sed -n -e \
> > +			"s/^\(dependency_libs\|inherited_linker_flags\)='\(.*\)'$/\2/p"
> > \
> > +			"${f}") ]]; then removing='no libs & flags'
> 
> unwrap that body, and use -r rather than escaping the (|) chars

Will do.

Expect a new version in next 12hrs.

-- 
Best regards,
Michał Górny

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [gentoo-dev] [PATCH eutils] Move remove_libtool_files() from autotools-utils for wider use.
  2012-05-31  5:46   ` Michał Górny
@ 2012-05-31  6:09     ` Mike Frysinger
  2012-05-31 11:40       ` Michał Górny
  0 siblings, 1 reply; 13+ messages in thread
From: Mike Frysinger @ 2012-05-31  6:09 UTC (permalink / raw
  To: gentoo-dev

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

On Thursday 31 May 2012 01:46:41 Michał Górny wrote:
> On Wed, 30 May 2012 17:19:49 -0400 Mike Frysinger wrote:
> > On Monday 28 May 2012 03:58:56 Michał Górny wrote:
> > > +# @USAGE: [all]
> > 
> > this is incorrect.  the usage is:
> > 	<all | files to remove>
> 
> No, it's perfectly valid. Moreover, it even explains what the function
> actually does rather than your imagination.

why are you so angry all the time ?  try being less confrontational for once.

going from the usage:
	remove_libtool_files [all]

that means this may be called in only two ways:
1)	remove_libtool_files
2)	remove_libtool_files all

yet, if you read the actual code, you'll see:
+	[[ ${#} -le 1 ]] || die "Invalid number of args to ${FUNCNAME}()"
+	if [[ ${#} -eq 1 ]]; then
+	...
+	fi

that means if more than 1 argument is passed, no error is thrown.  i thought 
you were intending to parse $@ further on because of it (hence the suggestion 
of updating the @USAGE), but it looks merely like your arg parsing is 
incorrect and needs fixing.  probably easiest by doing:
	case $#:$1 in
	0:'') ;;
	1:all) removing_all=1 ;;
	*) die "invalid usage" ;;
	esac

> > although, since we don't call die or anything, we can pipeline it to
> > speed things up a bit:
> > 	pc_libs=( $(
> > 		tpc="${T}/.pc"
> > 		find "${D}" -name '*.pc' -type f | \
> > 		while read pc ; do
> > 			sed -e '/^Requires:/d' "${pc}" > "${tpc}"
> > 			$(tc-getPKG_CONFIG) --libs "${tpc}"
> > 		done | tr ' ' '\n' | sort -u | \
> > 		sed -n '/^-l/{s:^-l:lib:;s:$:.la:;p}'
> > 		rm -f "${tpc}"
> > 	) )
> 
> Could you remind me, please, what performance-critical use of this
> function does justify making it so harsh?

looks perfectly fine to me, and it has the bonus of working

> > > +			rm -f "${archivefile}" || die
> > 
> > `rm -f` almost never fails.  in the edge cases where it does, you've
> > got bigger problems.
> 
> And that problem is good enough to die here.

more like the system at large is going to be falling over independently
-mike

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [gentoo-dev] [PATCH eutils] Move remove_libtool_files() from autotools-utils for wider use.
  2012-05-31  6:09     ` Mike Frysinger
@ 2012-05-31 11:40       ` Michał Górny
  0 siblings, 0 replies; 13+ messages in thread
From: Michał Górny @ 2012-05-31 11:40 UTC (permalink / raw
  To: gentoo-dev; +Cc: vapier

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

On Thu, 31 May 2012 02:09:11 -0400
Mike Frysinger <vapier@gentoo.org> wrote:

> yet, if you read the actual code, you'll see:
> +	[[ ${#} -le 1 ]] || die "Invalid number of args to
> ${FUNCNAME}()"
> +	if [[ ${#} -eq 1 ]]; then
> +	...
> +	fi
> 
> that means if more than 1 argument is passed, no error is thrown.

The exact opposite. If more than a single argument is passed, error is
thrown.

> thought you were intending to parse $@ further on because of it
> (hence the suggestion of updating the @USAGE), but it looks merely
> like your arg parsing is incorrect and needs fixing.  probably
> easiest by doing: case $#:$1 in
> 	0:'') ;;
> 	1:all) removing_all=1 ;;
> 	*) die "invalid usage" ;;
> 	esac

Just a little reverse logic in spirit of makefiles. But the case
variant would be probably more readable indeed.

-- 
Best regards,
Michał Górny

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [gentoo-dev] [PATCH eutils] Introduce prune_libtool_files().
  2012-05-30 21:19 ` [gentoo-dev] " Mike Frysinger
  2012-05-31  5:46   ` Michał Górny
@ 2012-05-31 12:55   ` Michał Górny
  2012-06-05  6:00     ` Mike Frysinger
  1 sibling, 1 reply; 13+ messages in thread
From: Michał Górny @ 2012-05-31 12:55 UTC (permalink / raw
  To: gentoo-dev; +Cc: Michał Górny

A function which determines correct .la files for removal and removes
them.
---
 gx86/eclass/eutils.eclass |   92 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 91 insertions(+), 1 deletion(-)

diff --git a/gx86/eclass/eutils.eclass b/gx86/eclass/eutils.eclass
index c88ef35..b0399ac 100644
--- a/gx86/eclass/eutils.eclass
+++ b/gx86/eclass/eutils.eclass
@@ -18,7 +18,7 @@
 if [[ ${___ECLASS_ONCE_EUTILS} != "recur -_+^+_- spank" ]] ; then
 ___ECLASS_ONCE_EUTILS="recur -_+^+_- spank"
 
-inherit multilib user
+inherit multilib toolchain-funcs user
 
 DESCRIPTION="Based on the ${ECLASS} eclass"
 
@@ -1330,6 +1330,96 @@ makeopts_jobs() {
 	echo ${jobs:-1}
 }
 
+# @FUNCTION: prune_libtool_files
+# @USAGE: [--all]
+# @DESCRIPTION:
+# Locate unnecessary libtool files (.la) and libtool static archives
+# (.a) and remove them from installation image.
+#
+# By default, .la files are removed whenever the static linkage can
+# either be performed using pkg-config or doesn't introduce additional
+# flags.
+#
+# If '--all' argument is passed, all .la files are removed. This is
+# usually useful when the package installs plugins and does not use .la
+# files for loading them.
+#
+# The .a files are only removed whenever corresponding .la files state
+# that they should not be linked to, i.e. whenever these files
+# correspond to plugins.
+#
+# Note: this function implicitly calls pkg-config. You should add it to
+# your DEPEND when using it.
+prune_libtool_files() {
+	debug-print-function ${FUNCNAME} "$@"
+
+	local removing_all opt
+	for opt; do
+		case "${opt}" in
+			--all)
+				removing_all=1
+				;;
+			*)
+				die "Invalid argument to ${FUNCNAME}(): ${opt}"
+		esac
+	done
+
+	# Create a list of all .pc-covered libs.
+	local pc_libs=()
+	if [[ ! ${removing_all} ]]; then
+		local f
+		local tf=${T}/prune-lt-files.pc
+		local pkgconf=$(tc-getPKG_CONFIG)
+
+		while IFS= read -r -d '' f; do # for all .pc files
+			sed -e '/^Requires:/d' "${f}" > "${tf}"
+			for arg in $("${pkgconf}" --libs "${tf}"); do
+				[[ ${arg} == -l* ]] && pc_libs+=( lib${arg#-l}.la )
+			done
+		done < <(find "${D}" -type f -name '*.pc' -print0)
+	fi
+
+	local f
+	while IFS= read -r -d '' f; do # for all .la files
+		local archivefile=${f/%.la/.a}
+
+		[[ ${f} != ${archivefile} ]] || die 'regex sanity check failed'
+
+		# Remove static libs we're not supposed to link against.
+		if grep -q '^shouldnotlink=yes$' "${f}"; then
+			einfo "Removing unnecessary ${archivefile#${D%/}}"
+			rm -f "${archivefile}"
+
+			# The .la file may be used by a module loader, so avoid removing it
+			# unless explicitly requested.
+			[[ ${removing_all} ]] || continue
+		fi
+
+		# Remove .la files when:
+		# - user explicitly wants us to remove all .la files,
+		# - respective static archive doesn't exist,
+		# - they are covered by a .pc file already,
+		# - they don't provide any new information (no libs & no flags).
+		local reason
+		if [[ ${removing_all} ]]; then
+			reason='requested'
+		elif [[ ! -f ${archivefile} ]]; then
+			reason='no static archive'
+		elif has "${f##*/}" "${pc_libs[@]}"; then
+			reason='covered by .pc'
+		elif [[ ! $(sed -nre \
+				"s/^(dependency_libs|inherited_linker_flags)='(.*)'$/\2/p" \
+				"${f}") ]]; then
+			reason='no libs & flags'
+		fi
+
+		if [[ ${reason} ]]; then
+			einfo "Removing unnecessary ${f#${D%/}} (${reason})"
+			rm -f "${f}"
+		fi
+	done < <(find "${D}" -type f -name '*.la' -print0)
+}
+
 check_license() { die "you no longer need this as portage supports ACCEPT_LICENSE itself"; }
 
 fi
-- 
1.7.10.2




^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [gentoo-dev] Re: [PATCH eutils] Move remove_libtool_files() from autotools-utils for wider use.
  2012-05-28  7:58 [gentoo-dev] [PATCH eutils] Move remove_libtool_files() from autotools-utils for wider use Michał Górny
                   ` (2 preceding siblings ...)
  2012-05-30 21:19 ` [gentoo-dev] " Mike Frysinger
@ 2012-06-05  2:46 ` Ryan Hill
  3 siblings, 0 replies; 13+ messages in thread
From: Ryan Hill @ 2012-06-05  2:46 UTC (permalink / raw
  To: gentoo-dev

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

On Mon, 28 May 2012 09:58:56 +0200
Michał Górny <mgorny@gentoo.org> wrote:

> As autotools-utils exports phase functions, it will be better if
> remove_libtool_files() functions would be somewhere else.

Thank you.


-- 
fonts, gcc-porting
toolchain, wxwidgets
@ gentoo.org

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [gentoo-dev] [PATCH eutils] Introduce prune_libtool_files().
  2012-05-31 12:55   ` [gentoo-dev] [PATCH eutils] Introduce prune_libtool_files() Michał Górny
@ 2012-06-05  6:00     ` Mike Frysinger
  2012-06-06 15:40       ` Michał Górny
  0 siblings, 1 reply; 13+ messages in thread
From: Mike Frysinger @ 2012-06-05  6:00 UTC (permalink / raw
  To: gentoo-dev

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

On Thursday 31 May 2012 08:55:25 Michał Górny wrote:
> +# Note: this function implicitly calls pkg-config. You should add it to
> +# your DEPEND when using it.

should clarify: implicitly calls pkg-config when your package provides a .pc.

> +	if [[ ! ${removing_all} ]]; then
> +		local f
> +		local tf=${T}/prune-lt-files.pc
> +		local pkgconf=$(tc-getPKG_CONFIG)
> +
> +		while IFS= read -r -d '' f; do # for all .pc files
> +			sed -e '/^Requires:/d' "${f}" > "${tf}"
> +			for arg in $("${pkgconf}" --libs "${tf}"); do

missing `local arg`

> +		done < <(find "${D}" -type f -name '*.pc' -print0)

not a big deal since it'll get cleaned anyways, but this doesn't clean up $tf 
when done

if no one else has feedback, i guess merge it
-mike

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [gentoo-dev] [PATCH eutils] Introduce prune_libtool_files().
  2012-06-05  6:00     ` Mike Frysinger
@ 2012-06-06 15:40       ` Michał Górny
  0 siblings, 0 replies; 13+ messages in thread
From: Michał Górny @ 2012-06-06 15:40 UTC (permalink / raw
  To: gentoo-dev; +Cc: vapier

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

On Tue, 5 Jun 2012 02:00:34 -0400
Mike Frysinger <vapier@gentoo.org> wrote:

> if no one else has feedback, i guess merge it

/var/cvsroot/gentoo-x86/eclass/ChangeLog,v  <--  ChangeLog
new revision: 1.291; previous revision: 1.290
/var/cvsroot/gentoo-x86/eclass/eutils.eclass,v  <--  eutils.eclass
new revision: 1.395; previous revision: 1.394

-- 
Best regards,
Michał Górny

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2012-06-06 15:41 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-05-28  7:58 [gentoo-dev] [PATCH eutils] Move remove_libtool_files() from autotools-utils for wider use Michał Górny
2012-05-28  9:03 ` Pacho Ramos
2012-05-29 13:50 ` [gentoo-dev] " Steven J Long
2012-05-29 14:26   ` Steven J Long
2012-05-30 17:16   ` Michał Górny
2012-05-30 21:19 ` [gentoo-dev] " Mike Frysinger
2012-05-31  5:46   ` Michał Górny
2012-05-31  6:09     ` Mike Frysinger
2012-05-31 11:40       ` Michał Górny
2012-05-31 12:55   ` [gentoo-dev] [PATCH eutils] Introduce prune_libtool_files() Michał Górny
2012-06-05  6:00     ` Mike Frysinger
2012-06-06 15:40       ` Michał Górny
2012-06-05  2:46 ` [gentoo-dev] Re: [PATCH eutils] Move remove_libtool_files() from autotools-utils for wider use Ryan Hill

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox