Ensure packages are installed before removing them

[evertiro]

Fixes outstanding issue on #2408

Checks

• I've updated the changelog.
• I've tested this PR
• This PR is for the develop branch not the stable branch.
• This PR is complete and ready for review.

[update-docs]

Thanks for opening this pull request! Make sure CHANGELOG.md gets updated with this change, additionally any docs that need updated can be found at https://github.com/Varying-Vagrant-Vagrants/varyingvagrantvagrants.org

GitHub

Varying-Vagrant-Vagrants/varyingvagrantvagrants.org

The VVV docs and website. Contribute to Varying-Vagrant-Vagrants/varyingvagrantvagrants.org development by creating an account on GitHub.

[tomjn]

I wonder if another PR doing the inverse for installing packages might be nice 🤔 , this PR looks good but needs testing

[evertiro]

I wonder if another PR doing the inverse for installing packages might be nice 🤔 , this PR looks good but needs testing

I'd say no. Installing over an existing package doesn't result in an error, and may actually be intentional at times.

[Mte90]

Well it is dpkg if the packages doesn't exists just write a string in the command and will move on with the others so is not a blocker. Also if the package exist and you install again it will do the same (I am a debian user).

[tomjn]

I wonder if another PR doing the inverse for installing packages might be nice 🤔 , this PR looks good but needs testing

I'd say no. Installing over an existing package doesn't result in an error, and may actually be intentional at times.

but it might let us avoid running apt install in the first place

[evertiro]

I wonder if another PR doing the inverse for installing packages might be nice 🤔 , this PR looks good but needs testing

I'd say no. Installing over an existing package doesn't result in an error, and may actually be intentional at times.

but it might let us avoid running apt install in the first place

I can't speak to any other setup, but the install actually does do necessary things on my build.

[tomjn]

yes but on the second provision once all those packages are installed, does it really need to make apt go and fetch them again then check? If all the packages were previously installed then we can avoid an update/clean/etc

[Mte90]

yes but on the second provision once all those packages are installed, does it really need to make apt go and fetch them again then check? If all the packages were previously installed then we can avoid an update/clean/etc

It wasn't part of this pr #2281

[tomjn]

@Mte90 that PR does something very different, that PR changes when apt is re-run, I'm saying that if nginx is already installed, that we don't ask it to install it, and only ask for the packages we're missing

[Mte90]

Well we need to do a lot of check with dpkg and it is something that apt will do automatically for us.
So I don't understand why we need to do a check before to run apt install we are rewriting a feature that is already present in apt and will not reinstall the packages.

[tomjn]

E.g. instead of:

VVV_PACKAGE_LIST+=(nginx)

we have something like this:

if ! command -v nginx &> /dev/null
then
    VVV_PACKAGE_LIST+=(nginx)
fi

[tomjn]

Well we need to do a lot of check with dpkg and it is something that apt will do automatically for us.
So I don't understand why we need to do a check before to run apt install we are rewriting a feature that is already present in apt and will not reinstall the packages.

Because it's faster, testing if Nginx or MariaDB are installed is much faster when we know what to look for. It also doesn't require us to update apt sources, clean out apt files, etc. Apt is the biggest time consumer in a fresh provision right now

[Mte90]

Honestly I don't know if it will be more faster to do that on by own or let do it by apt...

[evertiro]

I actually do think it'd be faster to check on our own.

[tomjn]

It's what Ansible does IIRC, it only tries to install things that need installing. Checking if a file exists or is in path will be much faster than checking apt sources and the local dpkg database.

As for this PR, it just needs testing, it looks ok from reading the code

[tomjn]

@evertiro I'm not sure why but I can no longer check out this branch locally:

~/dev/vvv ᚴ:develop 
❯ git checkout -b evertiro-issue/2408 develop
Switched to a new branch 'evertiro-issue/2408'
~/dev/vvv ᚴ:evertiro-issue/2408 
❯ git pull https://github.com/evertiro/VVV.git issue/2408
remote: Enumerating objects: 7, done.
remote: Counting objects: 100% (7/7), done.
remote: Compressing objects: 100% (1/1), done.
remote: Total 4 (delta 3), reused 4 (delta 3), pack-reused 0
Unpacking objects: 100% (4/4), 1.17 KiB | 171.00 KiB/s, done.
From https://github.com/evertiro/VVV
 * branch            issue/2408 -> FETCH_HEAD
fatal: Not possible to fast-forward, aborting.

[tomjn]

I suspect you need to rebase on to the latest develop branch

[tomjn]

I manually tested this, and it's functional but dpkg prints out a lot of information, and it's one of the rare situations we want to silence.

I modified the function to this:

vvv_package_remove() {
  declare -a packages=($@)

  # Ensure packages are actually installed before removing them
  if [ ${#packages[@]} -ne 0 ]; then
    for package in "${packages[@]}"; do
      if ! dpkg -s -l "${package}"; then
        packages=("${packages[@]/$package}")
      fi
    done
  fi
  echo "${packages[@]}"
  return 0

and got this:

vagrant@vvv:/srv/provision$ vvv_package_remove "banana test nginx"
dpkg-query: package 'banana' is not installed and no information is available
Use dpkg --info (= dpkg-deb --info) to examine archive files,
and dpkg --contents (= dpkg-deb --contents) to list their contents.
dpkg-query: package 'test' is not installed and no information is available
Use dpkg --info (= dpkg-deb --info) to examine archive files,
and dpkg --contents (= dpkg-deb --contents) to list their contents.
Package: nginx
Status: install ok installed
Priority: optional
Section: httpd
Installed-Size: 3011
Maintainer: Sergey Budnevitch <sb@nginx.com>
Architecture: amd64
Version: 1.19.8-1~bionic
Replaces: nginx-common, nginx-core
Provides: httpd, nginx, nginx-r1.19.8
Depends: libc6 (>= 2.27), libpcre3, libssl1.1 (>= 1.1.1), zlib1g (>= 1:1.1.4), lsb-base (>= 3.0-6), adduser
Conflicts: nginx-common, nginx-core
Conffiles:
 /etc/default/nginx e2b1ae0f31c6d03d3305ef526b0ba3b5
 /etc/default/nginx-debug 719f6f9981039a05a64c201a4b1db19f
 /etc/init.d/nginx 7224be660d7c280a775bd6eca2547df4
 /etc/init.d/nginx-debug 57e8f64b4f464fc13701c6b22240ac10
 /etc/logrotate.d/nginx a4da44b03e39926b999329061770362b
 /etc/nginx/conf.d/default.conf 411a0b28ff9b21a1cd94a6cb2ae2217d
 /etc/nginx/fastcgi_params 4729c30112ca3071f4650479707993ad
 /etc/nginx/koi-utf 3e338aca6a53a5420fc791b5ef86f64c
 /etc/nginx/koi-win bfa0b80381fed2b1dfcf617b0ba204ec
 /etc/nginx/mime.types cbd0fe7ed116af80a5a81b570559146e
 /etc/nginx/nginx.conf f7984934bd6cab883e1f33d5129834bb
 /etc/nginx/scgi_params df8c71e25e0356ffc539742f08fddfff
 /etc/nginx/uwsgi_params 88ac833ee8ea60904a8b3063fde791de
 /etc/nginx/win-utf 3749ffe19bedd842eb87e83d544e5ce6
Description: high performance web server
 nginx [engine x] is an HTTP and reverse proxy server, as well as
 a mail proxy server.
Homepage: https://nginx.org
  nginx
vagrant@vvv:/srv/provision$ 

https://askubuntu.com/questions/703160/running-dpkg-s-or-l-silent should help with remedying that

Ask Ubuntu

Running "dpkg -s" or "-l" silent

I am writing a script that checks for a very big list of dependencies. So I wrote a function that gets the package name as input and using dpkg checks if it is installed and prints an appropriate o...

[tomjn]

On further inspection this won't work because it's modifying the list as it's looping through it, which mangles the results.

I've made tweaks locally to fix this as well as put the inverse logic on the install and the main provisioner went from ~40s to 27s

[evertiro]

Your way is much cleaner than mine, closing this issue