Upgrade/install fixes #322

Commit-La-Grenouille · 2022-03-20T13:47:53Z

Quelques ajustements bienvenue pendant que je lançais et re-lançais le script de maj une fois passé à debian 11 (depuis l'image officielle en debian 10)

f-laurens

See my comment below about 14637dc.

f-laurens · 2022-03-23T19:40:57Z

install.sh

@@ -77,7 +77,9 @@ if [ $upgrade -eq 1 -a $makerfaire2018 -eq 0 -a -d ${home_dir}/wm8960 ]; then
  cd ${home_dir}/wm8960
  sudo chown -R ${owner} .git
  pull=`git pull`
-  if [ "$pull" != "Already up to date." ]; then
+  if [ "$pull" == "Already up to date." -o "$pull" == "Déjà à jour." ]; then


This is fine for French, but what about other non-English languages?...
I think a simpler/more general solution would be to just add the line:
export LANGUAGE=
at the beginning of the script, so that git always outputs messages in the default (English) language.

Commit-La-Grenouille · 2022-03-25T11:19:06Z

PR purged from duplicated commits to focus on the smarter builds & models generation ;)

Commit-La-Grenouille · 2022-03-25T13:57:20Z

All good and happy TagTagTag running on debian 11 upgraded from the latest debian 10 image released :)

Commit-La-Grenouille · 2022-04-05T10:15:30Z

So apparently the newer version of Git in Raspbian 11 also slightly changes the message when the clone is already up-to-date:
`+ cd /home/pi/wm8960

sudo chown -R pi .git
++ git pull
pull='Current branch tagtagtag-sound is up to date.'
'[' 'Current branch tagtagtag-sound is up to date.' '!=' 'Already up to date.' ']'
`
So I have updated the PR to support both variations of the message (validated on Raspbian 11).

f-laurens · 2022-04-08T17:37:26Z

So apparently the newer version of Git in Raspbian 11 also slightly changes the message when the clone is already up-to-date

Just curious: What exact version of Git is this? Where/how did you get it?
(My Bullseye rabbits, which are both (Raspberry Pi OS and DetPi) apt upgrade'd to the latest version of Bullseye packages, have Git version 2.30.2, which has the "Already up to date." form of the pull status message)

Commit-La-Grenouille · 2022-04-09T06:41:40Z

I was very surprised to see this slightly different message too on my dev Pi.
Here is the full details of the env:

pi@nabaztag:~/pynab $ uname -a
Linux nabaztag 5.15.32+ #1538 Thu Mar 31 19:37:58 BST 2022 armv6l GNU/Linux
pi@nabaztag:~/pynab $ locale
LANG=en_GB.UTF-8
LANGUAGE=
LC_CTYPE="en_GB.UTF-8"
LC_NUMERIC="en_GB.UTF-8"
LC_TIME="en_GB.UTF-8"
LC_COLLATE="en_GB.UTF-8"
LC_MONETARY="en_GB.UTF-8"
LC_MESSAGES="en_GB.UTF-8"
LC_PAPER="en_GB.UTF-8"
LC_NAME="en_GB.UTF-8"
LC_ADDRESS="en_GB.UTF-8"
LC_TELEPHONE="en_GB.UTF-8"
LC_MEASUREMENT="en_GB.UTF-8"
LC_IDENTIFICATION="en_GB.UTF-8"
LC_ALL=
pi@nabaztag:~/pynab $ git pull -r
Current branch master is up to date.
pi@nabaztag:~/pynab $ git --version
git version 2.30.2
pi@nabaztag:~/pynab $ git config --global -l
pull.rebase=true
user.name=Commit-La-Grenouille
user.email=<my-email-address>

I was not aware that Git could provide alternate variants of the update status but apparently it does.
The only explanation I could think of would be the configuration being the default one from the DEB package instead of a personalized one but seeing the setup script, I highly doubt that the pynab image uses any fancy Git config...

f-laurens · 2022-04-09T07:52:28Z

I highly doubt that the pynab image uses any fancy Git config...

To the best of my knowledge, it does not (and this has not changed recently): Git is just apt-get installed by the Pynab setup script, and then used by setup, upgrade.sh and install.sh, without any further configuration.

Can you reproduce the issue (different "up to date." wording) on a freshly flashed image?
Could it come from something specific in your own ~/.gitconfig or Pynab .git/config files?

Commit-La-Grenouille · 2022-04-11T15:55:00Z

* Could it come from something specific in your own `~/.gitconfig` or Pynab `.git/config` files?

As you can see, my Git config is minimal too (just the name+email to be able to create a commit if necessary):

pi@nabaztag:~ $ cat ~/.gitconfig
[pull]
	rebase = true
[user]
	name = Commit-La-Grenouille
	email = ******@*******.***
pi@nabaztag:~ $ cat pynab/.git/config
[core]
	repositoryformatversion = 0
	filemode = true
	bare = false
	logallrefupdates = true
[remote "origin"]
	url = https://github.com/nabaztag2018/pynab.git
	fetch = +refs/heads/*:refs/remotes/origin/*
	#fetch = +refs/heads/release:refs/remotes/origin/release
[branch "release"]
	remote = origin
	merge = refs/heads/release
[branch "master"]
	remote = origin
	merge = refs/heads/master

* Can you reproduce the issue (different "_up to date._" wording) on a freshly flashed image?

For the reproducer, the official pynab image won't do as it is a debian 10 (except if you want me to do another debian 10 to 11 upgrade in place).
However, if one of your fork's image (raspios or raspbian) is a pynab with debian 11, I will gladly reflash the SD card and try again (for the sake of testing).

f-laurens · 2022-04-11T16:21:31Z

However, if one of your fork's image (raspios or raspbian) is a pynab with debian 11, I will gladly reflash the SD card and try again (for the sake of testing).

You can use one of the images from my v0.9.1+fl0.9.3 release:

the zero_raspbian image is a Raspberry Pi OS Legacy (Debian 10 Buster) Pynab image,
but the zero_raspios image is a Raspberry Pi OS (Debian 11 Bullseye) Pynab image.

Commit-La-Grenouille · 2022-04-13T11:48:39Z

So with the debian 11 image (zero_raspios), it works out-of-the-box and with all updates applied (no change in Git version after update):

pi@Nabaztag:~/pynab $ git pull -r
Already up to date.
pi@Nabaztag:~/pynab $ uname -a
Linux Nabaztag 5.15.32+ #1538 Thu Mar 31 19:37:58 BST 2022 armv6l GNU/Linux
pi@Nabaztag:~/pynab $ git --version
git version 2.30.2

Next text will be to test with the debian 10 image (zero_raspbian) and after upgrading it to debian 11 in place... ;)

UPDATE: with the debian 10 image from your fork, I got BOTH output:

pi@Nabaztag:~ $ uname -a
Linux Nabaztag 5.10.63+ #1496 Wed Dec 1 15:57:05 GMT 2021 armv6l GNU/Linux
pi@Nabaztag:~ $ git --version
git version 2.20.1
pi@Nabaztag:~/pynab $ git pull -r
Already up to date.
Current branch release is up to date.
pi@Nabaztag:~/pynab $ git config --global -l
fatal: unable to read config file '/home/pi/.gitconfig': No such file or directory
pi@Nabaztag:~/pynab $ cat /etc/debian_version 
10.11

The plot thickens... 😁

f-laurens · 2022-04-13T12:43:59Z

Next text will be to test with the debian 10 image (zero_raspbian) and after upgrading it to debian 11 in place... ;)

Do you think this is worth the effort?
My guess is that in any case, on a fresh image (with an unmodified Git configuration for the pi user), be it with git 2.20.1 (which I think is the latest version on Buster), or after upgrade to git 2.30.2 (the current version on Bullseye), you will get the same 'Already up to date.' wording for the git pullmessage.

Commit-La-Grenouille · 2022-04-13T12:47:38Z

I was thinking that if you are concerned that the proposed wildcard may be too generous, I can also replace it by a strict match to "Already up to date." and add another possible strict match to "Current branch release is up to date." (any other branch being development so not being too concerned about optimizing here).
With this alternative, we would not risk a false-positive and yet would skip rebuilding when there is nothing new. What do you think ?

f-laurens · 2022-04-13T13:01:06Z

I was thinking that if you are concerned that the proposed wildcard may be too generous,

Not at all. Your proposed change seems technically fine.
The only issue I have (so far....) with it is that it adresses a problem (the wording change in the 'up to date.' message) that (sorry if that sounds provocative) does not seem to exist... ;-)
[The basic rule of technical support: "if a problem cannot be reproduced, it does not exist"]

f-laurens · 2022-04-13T13:07:37Z

UPDATE: with the debian 10 image from your fork, I got BOTH output:

This is when you do a pull -r.
If you do a pull (as is done in install.sh), you will just get the 'Already up to date.' message.

The plot thickens...

Well... It was so obvious that we missed it, but here is what triggered the message wording change in your original Git config:

[pull]
	rebase = true

since this makes pull behave as pull -r.

f-laurens · 2022-04-13T14:04:14Z

install.sh

  pull=$(LANGUAGE=en git pull)
-  if [ "$pull" == "Already up to date." ]; then
+  if [[ "$pull" == *" up to date." ]]; then


Change might rather be:
pull=$(LANGUAGE=en git pull --no-rebase)
(with no change in the "Already up to date." test)

Commit-La-Grenouille · 2022-04-14T07:03:17Z

As the whole point is to make sure the script behaves properly and as efficiently as possible, I reverted the extra commit as the default configuration of git in the image as well as the script are using a regular pull without rebase.

The good news is: if there is a decision to change the git pull command, default git config or just see that the rebase is used more frequently than we thought, we already have the diff ready to be applied ;)

At least now we went to the bottom of the problem and back and have the satisfaction that we did not hold anybody up (as I don´t think the repo admins are waiting for this to merge the PR 😛 ).

Great catch !

Commit-La-Grenouille · 2022-04-14T07:10:30Z

So as QEmu failed the rebuild after the commit removal, I ended up using the second push to implement the --no-rebase (instead of just a dummy commit).
And to make things simple, I left it separated in case it ends up being considered overkill.

f-laurens · 2022-04-14T07:29:55Z

implement the --no-rebase

Forcing --no-rebase seems indeed useful, in order to get predictable pull behaviour even if user has for some reason changed the local Git config.
Note: to be consistent, the -no-rebase option should then also be added to the pull for the pynab repository itself (in the upgrade.sh script).

f-laurens · 2022-04-14T15:49:12Z

install.sh

+    if [ -d "${ENGINE}" ]; then
+      #
+      file_age=$(stat -c %Y "${ENGINE}")
+      epoch_now=$(date +"%s")
+      #
+      #Trying to save some time & CPU in case the dataset was generated already today
+      if (( (epoch_now - file_age) > (60 * 60 * 24) )); then
+        rm -rf "${ENGINE}"
+        venv/bin/snips-nlu train nabd/nlu/nlu_dataset_${LANG}.json ${ENGINE}
+      fi


This optimisation may not be desirable.
It may indeed seem overkill to re-train the NLU engine when the install.sh script is invoked repeatedly, for testing purposes or whatever.
But in the general use case (regular update) this optimisation will be a problem in the (unlikely but possible) case of 2 successive (less than 24h apart) updates where the second update includes a change in NLU source data: this change will then be missed.

The original scenario was to run the upgrade script after upgrading the OS and having it fail at different points (early adopter). You are right that 24h may be a bit generous. But the idea is to save CPU and time when running it again and again within a window of a few hours.
We could drop the threshold to 2h...

We could drop the threshold to 2h...

That would still leave a window for failure.

But the idea is to save CPU and time when running it again and again within a window of a few hours.

Sure.
But if you want to have this "optimisation" in a public commit, you should at least condition it by the install.sh test parameter/flag (or rather a new adhoc parameter/flag).
The early adopter/tester would then set this flag when running install.sh.

Fair enough. As implementing a new flag for the install.sh would be a decent PR on its own, I removed this change from the diff to keep only the efforts around the git commands.

I still have to apply the additional --no-rebase in the upgrade.sh but I keep it as a reason to push the CI to rebuild (as it is not very cooperative lately...).

I removed this change from the diff to keep only the efforts around the git commands.

Fine. You can still keep the optimisation in a private commit, and just git cherry-pick it for your tests...

.... to push the CI to rebuild (as it is not very cooperative lately...).

Well, for this there is no hope till issue #329 is fixed...

Commit-La-Grenouille · 2022-04-15T09:32:39Z

implement the --no-rebase

Forcing --no-rebase seems indeed useful, in order to get predictable pull behaviour even if user has for some reason changed the local Git config. Note: to be consistent, the -no-rebase option should then also be added to the pull for the pynab repository itself (in the upgrade.sh script).

I checked the upgrade.sh script and in this case there is no check of the output. So we could apply the --no-rebase but like for the LANG= change, it may not be necessary per-se (but could be still put there for the sake of consistency).

f-laurens · 2022-04-15T10:01:52Z

I checked the upgrade.sh script and in this case there is no check of the output.

Indeed. But this is not the issue.
My comment is that, if we force --no-rebase behaviour on pull, this should then be consistent across all occurences of pull in upgrade.sh/install.sh.

f-laurens · 2022-04-15T13:58:21Z

@Commit-La-Grenouille : Thanks for all your work on this PR...
At this point, you might consider:

refactoring the commits into one: why keep 2, since they impact the same script lines, and have same intent to get more predictable, context-free, behaviour for git pull ?
maybe renaming the PR to something like Upgrade/Install fixes (since it finally is not Bullseye related ;-)

Commit-La-Grenouille · 2022-04-16T13:32:14Z

@Commit-La-Grenouille : Thanks for all your work on this PR... At this point, you might consider:

* refactoring the commits into one: why keep 2, since they impact the same script lines, and have same intent to get more predictable, context-free, behaviour for `git pull` ?

* maybe renaming the PR to something like _Upgrade/Install fixes_  (since it finally is not Bullseye related ;-)

PR renamed and changes unified under a unique commit. That is exactly the point of the PR flow: brainstorming to make sure the change is the prettiest it can before going into the main branch ;)

pguyot · 2022-04-23T13:33:45Z

upgrade.sh

    sudo -u ${owner} touch /tmp/pynab.upgrade
    sudo chown ${owner} /tmp/pynab.upgrade
    echo "Updating code - 1/?" > /tmp/pynab.upgrade
    cd ${root_dir}
    if [[ $EUID -ne ${ownerid} ]]; then
-      sudo -u ${owner} git pull
+      sudo -u ${owner} git pull --no-rebase


I don't understand why we would want --no-rebase here.

The context is that when there is a default rebase strategy active in the global git config, the up-to-date message returned by Git is slightly different from the one tested (see PR discussion earlier). I initially proposed to adjust the message matching to support "Already up-to-date" and "Branch XXX is up-to-date". @f-laurens suggested that the message could be made consistent by using --no-rebase.

pguyot · 2022-04-23T13:37:58Z

install.sh

@@ -76,7 +76,8 @@ if [ $upgrade -eq 1 -a $makerfaire2018 -eq 0 -a -d ${home_dir}/wm8960 ]; then
  echo "Updating sound driver - 2/14" > /tmp/pynab.upgrade
  cd ${home_dir}/wm8960
  sudo chown -R ${owner} .git
-  pull=`git pull`
+  # Making sure we keep the up-to-date status message consistent no matter the rebase default behavior
+  pull=$(LANGUAGE=en git pull --no-rebase)


Likewise, I don't understand why we would like LANGUAGE=en here.
This code is not meant to be executed from an interactive session.

See better implementaitons such as:
https://unix.stackexchange.com/questions/155046/determine-if-git-working-directory-is-clean-from-a-script/394674#394674

Good point that there may be some porcelain/plumbing trick we can use to have a consistent status no matter what the locale & git config is active in the OS.
However, the git diff-index discussed in your link seems to work only where the local clone is ahead (or with untracked files) compared to remote. When I tried to test it with a local clone behind the remote (by doing a reset --hard to a previous commit & confirming that status reports a branch behind by X commits) the diff-index seems to return 0 the same way that when the branch is up-to-date.

I will keep on searching in the reference documentation until I hit something that could provide us a reliable way to tell if we have fresh code we want to rebuild ;)

So I ended up decomposing the action in 3: doing a git fetch before, checking the local/remote discrepancy with status --porcelain (safest) and apply the pull if there is something to get and rebuild.

When there is no remote/local difference, the output looks like:
ᐅ git status --branch --porcelain
## bulleye_upgrade_fixes...origin/bulleye_upgrade_fixes

And where there is a difference:
ᐅ git status --branch --porcelain
## bulleye_upgrade_fixes...origin/bulleye_upgrade_fixes [behind 1]

As it is a change of strategy in the code, I kept the change in a separate commit for now until it is considered satisfactory (and then I can rebase everything into a single commit)

Likewise, I don't understand why we would like LANGUAGE=en here. This code is not meant to be executed from an interactive session.

Just my 2 cts. If you want to have consistent messages in your scripts tests, use LC_ALL=C, not LANGUAGE=en. LANGUAGE=en will still depend on the English flavors (en-GB, en-US, en-CA...)

See https://unix.stackexchange.com/questions/87745/what-does-lc-all-c-do for a good explanation.

…atter the locale defined + keeping all git calls immune to explicit rebase config

…k of locale or rebase strategy impacting the result)

github-actions bot added the Setup label Mar 20, 2022

f-laurens reviewed Mar 23, 2022

View reviewed changes

Commit-La-Grenouille force-pushed the bulleye_upgrade_fixes branch 2 times, most recently from be823f2 to 45dcbb6 Compare March 25, 2022 11:17

Commit-La-Grenouille force-pushed the bulleye_upgrade_fixes branch from 45dcbb6 to 817685d Compare March 25, 2022 13:36

f-laurens reviewed Apr 13, 2022

View reviewed changes

Commit-La-Grenouille force-pushed the bulleye_upgrade_fixes branch from b089215 to 817685d Compare April 14, 2022 06:59

f-laurens reviewed Apr 14, 2022

View reviewed changes

Commit-La-Grenouille force-pushed the bulleye_upgrade_fixes branch from 9409c3f to 895438c Compare April 15, 2022 09:35

Commit-La-Grenouille force-pushed the bulleye_upgrade_fixes branch 2 times, most recently from db412f0 to 3878c0f Compare April 15, 2022 10:16

Commit-La-Grenouille force-pushed the bulleye_upgrade_fixes branch 2 times, most recently from b36c32c to c1fc141 Compare April 16, 2022 13:29

Commit-La-Grenouille changed the title ~~Bulleye upgrade fixes~~ Upgrade/install fixes Apr 16, 2022

pguyot reviewed Apr 23, 2022

View reviewed changes

INSTALL: making sure the git clone status checks are predictable no m…

5a5a950

…atter the locale defined + keeping all git calls immune to explicit rebase config

Commit-La-Grenouille force-pushed the bulleye_upgrade_fixes branch from c1fc141 to 5c8af62 Compare April 28, 2022 07:46

INSTALL: using porcelain to detect if the clone is up-to-date (no ris…

67d98b1

…k of locale or rebase strategy impacting the result)

Commit-La-Grenouille force-pushed the bulleye_upgrade_fixes branch from 5c8af62 to 67d98b1 Compare April 28, 2022 07:54

Upgrade/install fixes #322

Are you sure you want to change the base?

Upgrade/install fixes #322

Conversation

Commit-La-Grenouille commented Mar 20, 2022

f-laurens left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Commit-La-Grenouille commented Mar 25, 2022

Commit-La-Grenouille commented Mar 25, 2022

Commit-La-Grenouille commented Apr 5, 2022

f-laurens commented Apr 8, 2022 • edited Loading

Commit-La-Grenouille commented Apr 9, 2022 • edited Loading

f-laurens commented Apr 9, 2022 • edited Loading

Commit-La-Grenouille commented Apr 11, 2022

f-laurens commented Apr 11, 2022 • edited Loading

Commit-La-Grenouille commented Apr 13, 2022 • edited Loading

f-laurens commented Apr 13, 2022 • edited Loading

Commit-La-Grenouille commented Apr 13, 2022

f-laurens commented Apr 13, 2022 • edited Loading

f-laurens commented Apr 13, 2022 • edited Loading

f-laurens Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

Commit-La-Grenouille commented Apr 14, 2022

Commit-La-Grenouille commented Apr 14, 2022

f-laurens commented Apr 14, 2022 • edited Loading

f-laurens Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f-laurens Apr 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f-laurens Apr 15, 2022 • edited Loading

Choose a reason for hiding this comment

Commit-La-Grenouille commented Apr 15, 2022

f-laurens commented Apr 15, 2022 • edited Loading

f-laurens commented Apr 15, 2022 • edited Loading

Commit-La-Grenouille commented Apr 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Commit-La-Grenouille Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f-laurens left a comment •

edited

Loading

f-laurens commented Apr 8, 2022 •

edited

Loading

Commit-La-Grenouille commented Apr 9, 2022 •

edited

Loading

f-laurens commented Apr 9, 2022 •

edited

Loading

f-laurens commented Apr 11, 2022 •

edited

Loading

Commit-La-Grenouille commented Apr 13, 2022 •

edited

Loading

f-laurens commented Apr 13, 2022 •

edited

Loading

f-laurens commented Apr 13, 2022 •

edited

Loading

f-laurens commented Apr 13, 2022 •

edited

Loading

f-laurens Apr 13, 2022 •

edited

Loading

f-laurens commented Apr 14, 2022 •

edited

Loading

f-laurens Apr 14, 2022 •

edited

Loading

f-laurens Apr 15, 2022 •

edited

Loading

f-laurens Apr 15, 2022 •

edited

Loading

f-laurens commented Apr 15, 2022 •

edited

Loading

f-laurens commented Apr 15, 2022 •

edited

Loading

Commit-La-Grenouille Apr 28, 2022 •

edited

Loading