On Sun, Jul 16, 2017 at 01:23:00PM +0200, Klemens Nanni wrote:
> On Sun, Jul 16, 2017 at 10:26:25AM +0000, Robert Peichaer wrote:
> > On Sun, Jul 16, 2017 at 03:37:15AM +0200, Klemens Nanni wrote:
> > > Why looping over all existing archives, picking the latest version of
> > > the current archive, skipping it in case it's already in our list of
> > > selected latest versions or adding it otherwise?
> > >
> > > The current code runs ls|sort|tail about n * (v - 1) times for n
> > > different libraries and v versions respectively since the globbed list
> > > is almost always sorted already, effectively adding the latest versions
> > > after skipping all others.
> >
> > Yeah well, until the globbing isn't sorting as expected.
> >
> > $ ls -1 libc.so.*.a
> > libc.so.89.10.a
> > libc.so.89.6.a
> > libc.so.89.7.a
> > libc.so.89.8.a
> > libc.so.89.9.a
> Exactly, that's why sort(1) is used.
> >
> > But that's not the point anyway in your otherwise perfectly valid change.
> >
> > > This diff makes it much clearer and simpler by sorting and picking
> > > only as many versions as there are libraries to reorder (two). Globbing
> > > is done within the loop so future libraries with different naming
> > > schemes comes at no cost.
> > >
> > > Applies cleanly to both the current revision as well as my previous diff
> > > but the previous one will fail on top of this one.
> > >
> > > Feedback? Comments?
> > >
> > > Index: rc
> > > ===================================================================
> > > RCS file: /cvs/src/etc/rc,v
> > > retrieving revision 1.507
> > > diff -u -p -r1.507 rc
> > > --- rc 4 Jul 2017 19:02:11 -0000 1.507
> > > +++ rc 16 Jul 2017 01:15:43 -0000
> > > @@ -171,13 +171,10 @@ reorder_libs() {
> > > echo -n 'reordering libraries:'
> > >
> > > # Only choose the latest version of the libraries.
> > > - for _liba in /usr/lib/libc.so.*.a /usr/lib/libcrypto.so.*.a; do
> > > - _liba=$(ls ${_liba%%.[0-9]*}*.a | sort -V | tail -1)
> > > - for _l in $_libas; do
> > > - [[ $_l == $_liba ]] && continue 2
> > > - done
> > > - _libas="$_libas $_liba"
> > > + for _liba in 'libc.so.*.a' 'libcrypto.so.*.a'; do
> > > + _libas="$_libas $(ls /usr/lib/$_liba | sort -V | tail -1)"
> > > done
> >
> > That's definitely a much better approach.
> >
> > But I'd like to stay strict matching the filenames.
> >
> > + for _liba in /usr/lib/lib{c,crypto}; do
> > + _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
> > + done
> >
> > > + _libas=${_libas# }
> >
> > This would be another way of suppressing the blank right away.
> > But it's not really necessary anyway, because the leading blank is
> > removed in the list of the other for-loop.
> >
> > _libas="${_libas:+$_libas }$(ls $_liba.so.+([0-9.]).a | sort -V | tail
> > -1)"
> I explicitly avoided this since stripping the leading space in the end
> seemed clearer about what's going on. Also, this check is done for each
> library whereas only the first one is true.
>
> I also thought about leaving it in but stripping it makes clear I know
> what I'm doing and future changes where $_libas might get quoted will
> be working as expected.
To the contrary what my previous answer might indicate, I don't care
that much either. If you want to explicitely remove the blank, go for
it.
--
-=[rpe]=-
