Gentoo Archives: gentoo-dev

From: "Michał Górny" <mgorny@g.o>
To: gentoo-dev@l.g.o
Cc: vapier@g.o
Subject: Re: [gentoo-dev] [PATCH eutils] Move remove_libtool_files() from autotools-utils for wider use.
Date: Thu, 31 May 2012 05:46:35
Message-Id: 20120531074641.73b93222@pomiocik.lan
In Reply to: Re: [gentoo-dev] [PATCH eutils] Move remove_libtool_files() from autotools-utils for wider use. by Mike Frysinger
On Wed, 30 May 2012 17:19:49 -0400
Mike Frysinger <vapier@g.o> 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

Attachments

File name MIME type
signature.asc application/pgp-signature

Replies