installer: clean up clean_old()

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

installer: clean up clean_old()

Christian Weisgerber
* Remove syspatch files from the installed system and not the ramdisk.
* Use extended globs and generally adopt to the style of this script.

ok?

I'm not very happy with the way the clang version is determined.
If we ever were to move to 10.0.0, this would remove the wrong
directory.  I considered examining the output of "chroot /mnt
/usr/bin/clang -v", but I don't like that much either.

Index: distrib/miniroot/install.sub
===================================================================
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1138
diff -u -p -r1.1138 install.sub
--- distrib/miniroot/install.sub 13 Aug 2019 07:47:40 -0000 1.1138
+++ distrib/miniroot/install.sub 13 Aug 2019 19:45:23 -0000
@@ -2860,14 +2860,16 @@ __EOT
 }
 
 clean_old() {
+ local _cver
+
  if [[ -f /mnt/usr/bin/gcc ]]; then
- rm -rf -- `ls -d /mnt/usr/lib/gcc-lib/* | grep -v ${VNAME}$`
+ rm -rf /mnt/usr/lib/gcc-lib/!(*$VNAME)
  fi
  if [[ -f /mnt/usr/bin/clang ]]; then
- CVER=$(cd /mnt/usr/lib/clang && ls -r | sed -e 1q)
- rm -rf -- `ls -d /mnt/usr/lib/clang/* | grep -v "${CVER}$"`
+ _cver=$(cd /mnt/usr/lib/clang && ls -r | sed -e 1q) &&
+ rm -rf /mnt/usr/lib/clang/!($_cver)
  fi
- rm -rf /var/syspatch/*
+ rm -rf /mnt/var/syspatch/*
 }
 
 do_autoinstall() {
--
Christian "naddy" Weisgerber                          [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: installer: clean up clean_old()

Theo Buehler-3
On Tue, Aug 13, 2019 at 09:59:28PM +0200, Christian Weisgerber wrote:
> * Remove syspatch files from the installed system and not the ramdisk.
> * Use extended globs and generally adopt to the style of this script.
>
> ok?

I'm ok with your patch. One suggestion:

>   if [[ -f /mnt/usr/bin/clang ]]; then

Could you add '&& -d /mnt/usr/lib/clang' to check whether the directory
exists before trying to cd into it (similarly for /mnt/usr/bin/gcc and
gcc-libs)?

On amd64, /usr/bin/clang is in the base set due to KARL while the
/usr/lib/clang directory is shipped as part of the comp set.  I know we
don't want to support installs that skip some sets, but still.

> - CVER=$(cd /mnt/usr/lib/clang && ls -r | sed -e 1q)
> - rm -rf -- `ls -d /mnt/usr/lib/clang/* | grep -v "${CVER}$"`
> + _cver=$(cd /mnt/usr/lib/clang && ls -r | sed -e 1q) &&
> + rm -rf /mnt/usr/lib/clang/!($_cver)
>   fi

Reply | Threaded
Open this post in threaded view
|

Re: installer: clean up clean_old()

Theo de Raadt-2
Theo Buehler <[hidden email]> wrote:

> On Tue, Aug 13, 2019 at 09:59:28PM +0200, Christian Weisgerber wrote:
> > * Remove syspatch files from the installed system and not the ramdisk.
> > * Use extended globs and generally adopt to the style of this script.
> >
> > ok?
>
> I'm ok with your patch. One suggestion:
>
> >   if [[ -f /mnt/usr/bin/clang ]]; then
>
> Could you add '&& -d /mnt/usr/lib/clang' to check whether the directory
> exists before trying to cd into it (similarly for /mnt/usr/bin/gcc and
> gcc-libs)?

Then might as well just -d only, and skip the -f

But then people who mangle their system with symbolic links will lose

Perhaps that is actually for the best....

Reply | Threaded
Open this post in threaded view
|

Re: installer: clean up clean_old()

Christian Weisgerber
On 2019-08-13, "Theo de Raadt" <[hidden email]> wrote:

>> Could you add '&& -d /mnt/usr/lib/clang' to check whether the directory
>> exists before trying to cd into it (similarly for /mnt/usr/bin/gcc and
>> gcc-libs)?
>
> Then might as well just -d only, and skip the -f

Seriously, why bother with a check in the first place?
If the glob fails to match, it will be passed unexpanded to rm, which
will fail silently.  That's not different from syspatch/*.

Does anybody see a problem with this:

clean_old() {
        local _cver

        rm -rf /mnt/usr/lib/gcc-lib/!(*$VNAME)
        _cver=$(cd /mnt/usr/lib/clang 2>/dev/null && ls -r | sed -e 1q) &&
                rm -rf /mnt/usr/lib/clang/!($_cver)
        rm -rf /mnt/var/syspatch/*
}

> But then people who mangle their system with symbolic links will lose

At most a symlink will be removed and the directory still exists
somewhere else. *shrug*
I don't think we should worry about such corner cases on nonstandard
systems.

--
Christian "naddy" Weisgerber                          [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: installer: clean up clean_old()

Theo de Raadt-2
Christian Weisgerber <[hidden email]> wrote:

> > But then people who mangle their system with symbolic links will lose
>
> At most a symlink will be removed and the directory still exists
> somewhere else. *shrug*
> I don't think we should worry about such corner cases on nonstandard
> systems.

If it is relative symlink, correct.  If it is absolute symlink the
whole contraption fails silently -- doing nothing -- and that's OK.

Looks good to me.

Reply | Threaded
Open this post in threaded view
|

Re: installer: clean up clean_old()

Christian Weisgerber
In reply to this post by Christian Weisgerber
On 2019-08-14, Christian Weisgerber <[hidden email]> wrote:

> clean_old() {
>         local _cver
>
>         rm -rf /mnt/usr/lib/gcc-lib/!(*$VNAME)
>         _cver=$(cd /mnt/usr/lib/clang 2>/dev/null && ls -r | sed -e 1q) &&
>                 rm -rf /mnt/usr/lib/clang/!($_cver)
>         rm -rf /mnt/var/syspatch/*
> }

Use shell tools instead of fiddling with ls|sed:

clean_old() {
        local _cver

        rm -rf /mnt/usr/lib/gcc-lib/!(*$VNAME)
        set -A _cver -- /mnt/usr/lib/clang/* &&
                unset _cver[${#_cver[@]}-1] &&
                rm -rf "${_cver[@]}"
        rm -rf /mnt/var/syspatch/*
}

This sets an array to the expansion of the glob, which is an
alphabetically sorted list, drops the last element, and rm(1)s
the rest.

--
Christian "naddy" Weisgerber                          [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: installer: clean up clean_old()

Theo Buehler-3
> This sets an array to the expansion of the glob, which is an
> alphabetically sorted list, drops the last element, and rm(1)s
> the rest.

Nice. reads & works ok