improve error handling 3 #255

adrelanos · 2023-11-22T05:39:42Z

Fixes #224

adrelanos · 2023-11-22T05:49:47Z

Debian stretch build still failing as mentioned earlier in #248 (comment).

But not an issue introduced by this PR. This PR here makes the issue only visible.

I compared with an arbitrary different recent PR. A PR not authored by me. Namely this PR:

improve error handling 2 #248 (comment)

Here is the relevant excerpt from the other PR build log:

https://github.com/grml/grml-debootstrap/actions/runs/6913689021/job/18810784414?pr=249

2023-11-18T11:48:47.2487870Z Err:7 http://security.debian.org stretch/updates/main amd64 Packages
2023-11-18T11:48:47.2488469Z   404  Not Found
2023-11-18T11:48:47.2529303Z Get:9 http://archive.debian.org/debian stretch/main Translation-en [5377 kB]
2023-11-18T11:48:47.2586326Z Ign:8 http://security.debian.org stretch/updates/main Translation-en
2023-11-18T11:48:48.6862318Z Fetched 5380 kB in 1s (2900 kB/s)
2023-11-18T11:48:49.1516508Z Reading package lists...
2023-11-18T11:48:49.1748396Z W: The repository 'http://security.debian.org stretch/updates Release' does not have a Release file.
2023-11-18T11:48:49.1750626Z E: Failed to fetch http://security.debian.org/dists/stretch/updates/main/binary-amd64/Packages  404  Not Found
2023-11-18T11:48:49.1752139Z E: Some index files failed to download. They have been ignored, or old ones used instead.

This issue seems parmement. And also not worth reporting to Debian as stretch doesn't have security support by Debian anymore anyhow.

Potential solutions:

A) Dropping support for Debian stretch. This requires the least code. I am not a big fan of supporting legacy versions but you might have other requirements here.

Quote https://wiki.debian.org/DebianStretch

Security updates have been discontinued as of 2020-07-06.

B) Not enabling the security repository for Debian stretch builds.
C) Adding || true for invocations of $APTUPDATE (and similar) but only for Debian stretch builds. Not a fan of this.

I'll attempt to implement option B) for now until I receive other feedback.

adrelanos · 2023-11-22T06:19:08Z

All tests are passing now except shellcheck but these seem to be unrelated to this PR.

I would appreciate some feedback on how to proceed with this PR.

mika · 2023-12-07T15:59:47Z

grml-debootstrap

@@ -257,7 +257,9 @@ cleanup() {

  # Remove temporary mountpoint again
  if echo "$MNTPOINT" | grep -q '/mnt/debootstrap\.' ; then
-    rmdir "$MNTPOINT" 2>/dev/null
+    if test -d "$MNTPOINT" ; then


please use if [ -d .... style here, as we do elsewhere in the code

May I.

A) simply add a commit on top (preferred, easier for me) or,

B) should I (learn to) modify the existing commit and fix the style?

Fixed after force push just now.

mika · 2023-12-07T16:04:23Z

chroot-script

-     if stage $i ; then
-       $i
-       stage $i 'done'
+     if stage "$i" ; then


leaving the comment here, since on GitHub one sadly can't seem to comment on the commit message itself, AFAICS: the change itself LGTM, though please use something like "chroot-script: properly quote stage execution code" in the commit message

This means I should leave most commit messages as is but modify this specific commit message?

I'd be more than happy to get nice commit messages! This one only clearly jumped into my eye :) So please feel free and invited to the reword your commit messages (and also squash logically related commits!), so they tell a nice story when going through them, but also have a (somewhat) nice state/story for debian/changelog (the first line of each commit message message shows up there by default when I release a new version, see gbp dch!)

I was about to suggest, I was hoping, all of these commits could be squashed into 1. These changes aren't really interesting for users. Just "improved error handling".

I rewrote this commit message, grouped together and also improved other commit messages.

mika · 2023-12-07T16:08:36Z

chroot-script

-    unstable|sid) ;;  # no security pool available
-    jessie|stretch|buster)
+    unstable|sid|stretch) ;;  # no security pool available
+    jessie|buster)


are you sure this change makes sense? stretch is in between jessie and buster (see https://wiki.debian.org/DebianReleases)?

Unfortunately, I am sure. The repository is defunct but not worth reporting upstream to Debian because that suite is end of life.

Ok, but then at least jessie should be affected as well, nor? :)
Also see http://archive.debian.org/debian-security/dists/

In theory, I agree. It should also be affected. But in practice, it's not as I could see from the CI tests. I don't have an explanation why it is as is. I just noticed that oddly 1 Debian security repository for an EOL suite is broken.

Porting to archive.debian.org might fix this issue but lets consider that a separate issue that is not worth doing for an EOL suite? :)

Not that not enabling security repository for stretch didn't reduce the security or change installed packages.

Previously: the stretch build would just show an error message that the stretch repository is unavailable, ignore it, and proceed.

After this PR that enabled strict error checking: That resulted in a build failure so we at least know something is wrong. It only made this issue apparent. But the issue was there all along even if nobody noticed.This PR however didn't worsen anything. Well, could argue in theory if that stretch security repository ever comes back online then it's missing from these builds. If that is considered an issue then indeed code to make stretch use archive.debian.org might be required.

mika · 2023-12-07T16:10:13Z

chroot-script

@@ -480,6 +480,10 @@ EOF
    local rootfs_mount_options=""

    if [ -z "${FILESYSTEM}" ] ; then
+      ## Debugging.
+      ls -la /dev/disk/by-uuid/"${TARGET_UUID}"
+      blkid /dev/disk/by-uuid/"${TARGET_UUID}"


no reason to leave this commit in this PR, nor?

Ok. Removed after force push. Done.

mika · 2023-12-07T16:10:59Z

All tests are passing now except shellcheck but these seem to be unrelated to this PR.

Ok great

I would appreciate some feedback on how to proceed with this PR.

Yeah I think we can go forward with this, I left comments on some of the commits, and think we should get rid of the debugging related changes that shouldn't be part of the main grml-debootstrap? (No objections to enable debugging code when running with --debug option or alike though!)

because these are now covered by the error_handler

No need to use both, `eend 1` and `bailout 1`. In these cases, only `bailout 1` is sufficient.

only delete `$MNTPOINT` if such a folder actually exists

because these are now covered by the new error handling method

grml#224 (comment)

because the cleanup function gets run by the error handler and should not be nested

because it will not always work will not work in cases where /dev etc is still mounted inside the chroot

to work around an issue by github actions drop `-q` from modprobe for better debug output

because no longer provided by Debian

adrelanos · 2023-12-07T19:06:30Z

I've hopefully addressed all your feedback. Please kindly let me know if there's other blockers to get this merged.

There's a new related issue which is triggered (but not caused) by this PR for which I created a separate issue:

Debian stretch build failing due to missing busybox #256

instead of sometimes mixed with "${MNTPOINT}/boot/efi"

mika · 2023-12-22T16:12:25Z

Sorry for the delay, finally found some spare minutes to take care of this. Thanks for all your work around this, @adrelanos ! 👍

adrelanos force-pushed the error-handling2 branch from 3eaca7a to da01692 Compare November 22, 2023 05:54

mika reviewed Dec 7, 2023

View reviewed changes

adrelanos added 4 commits December 7, 2023 12:50

enable error handler by default

1a229d0

use || eend $? consistently in cleanup function

0ad48de

remove obsolete uses of eend $?

2cf99e2

because these are now covered by the error_handler

code simplification

e0dc8d8

No need to use both, `eend 1` and `bailout 1`. In these cases, only `bailout 1` is sufficient.

adrelanos force-pushed the error-handling2 branch from da01692 to e2222be Compare December 7, 2023 18:38

adrelanos added 17 commits December 7, 2023 13:50

fix cleanup function

601c99c

only delete `$MNTPOINT` if such a folder actually exists

fix cleanup function

d2be7fb

only delete `$MNTPOINT` if such a folder actually exists

don't suppress stderr of rmdir "$MNTPOINT"

792af79

fix use of eend in cleanup function

bee27f0

adjust use of dialog for new error handling method

105ae6f

remove obsolete || return 1

53ed3b4

because these are now covered by the new error handling method

adjust stages executing for new error handling method

fcaea6e

code simplification of stages executing

ed17c95

trigger error handler as soon as possible

c59878b

grml#224 (comment)

chroot-script: properly quote stage execution code

6db61f4

adjust options parsing for new error handling method

406f1e4

adjust use of dialog for new error handling method

521689a

syntax check before parsing config files

4812ce4

ignore exit codes of umount during cleanup function

d39c8df

because the cleanup function gets run by the error handler and should not be nested

the first rmdir "$MNTPOINT" needs || true

cc96e88

because it will not always work will not work in cases where /dev etc is still mounted inside the chroot

ignore non-zero exist codes of modprobe loop and modprobe dm-mod

08f75f2

to work around an issue by github actions drop `-q` from modprobe for better debug output

ci: enable --debug

016c466

adrelanos added 3 commits December 7, 2023 13:52

ignore exit code of blkid as it might fail on some file systems

65e45e5

improve error handling for EFI uuid

e11f706

not enabling stretch security repository

7a3f1d9

because no longer provided by Debian

adrelanos force-pushed the error-handling2 branch from e2222be to 7a3f1d9 Compare December 7, 2023 18:53

adrelanos mentioned this pull request Dec 7, 2023

Debian stretch build failing due to missing busybox #256

Closed

adrelanos mentioned this pull request Dec 9, 2023

refactoring --vmefi --arch arm64 #258

Open

adrelanos added 2 commits December 11, 2023 11:23

style fix: always use "${MNTPOINT}"/boot/efi

14e8a82

instead of sometimes mixed with "${MNTPOINT}/boot/efi"

fix error handling for unmounting on the arm64 architecture

cc891df

mika merged commit bfe0fed into grml:master Dec 22, 2023
34 of 40 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve error handling 3 #255

improve error handling 3 #255

adrelanos commented Nov 22, 2023

adrelanos commented Nov 22, 2023

adrelanos commented Nov 22, 2023

mika Dec 7, 2023

adrelanos Dec 7, 2023

adrelanos Dec 7, 2023

mika Dec 7, 2023

adrelanos Dec 7, 2023

mika Dec 7, 2023

adrelanos Dec 7, 2023

adrelanos Dec 7, 2023

mika Dec 7, 2023

adrelanos Dec 7, 2023

mika Dec 7, 2023

adrelanos Dec 7, 2023

adrelanos Dec 7, 2023

adrelanos Dec 7, 2023

mika Dec 7, 2023

adrelanos Dec 7, 2023

mika commented Dec 7, 2023

adrelanos commented Dec 7, 2023

mika commented Dec 22, 2023

improve error handling 3 #255

improve error handling 3 #255

Conversation

adrelanos commented Nov 22, 2023

adrelanos commented Nov 22, 2023

adrelanos commented Nov 22, 2023

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

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

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

Choose a reason for hiding this comment

mika commented Dec 7, 2023

adrelanos commented Dec 7, 2023

mika commented Dec 22, 2023