Skip to content

Comments

feat(26.04): add e2fsprogs#883

Open
Meulengracht wants to merge 3 commits intocanonical:ubuntu-26.04from
Meulengracht:feature/e2fsprogs
Open

feat(26.04): add e2fsprogs#883
Meulengracht wants to merge 3 commits intocanonical:ubuntu-26.04from
Meulengracht:feature/e2fsprogs

Conversation

@Meulengracht
Copy link
Member

@Meulengracht Meulengracht commented Feb 2, 2026

Proposed changes

The e2fsprogs suite is needed by Ubuntu Core 26. This needs logsave (#879) and systemd changes (#878)

Checklist

@lczyk
Copy link
Collaborator

lczyk commented Feb 2, 2026

  • blocked on ci error preparing lxd machine in test #828. working on a fix. for now cancelled the integration tests as they would never succeed and eventually time out, and they were taking up worker allocation. will re-trigger when ci is back to a working state. apologies for the inconvenience!

@lczyk lczyk added the Blocked Waiting for something external label Feb 2, 2026
@ROCKsBot ROCKsBot requested a review from a team February 3, 2026 01:41
@zhijie-yang zhijie-yang changed the title slices,tests: add e2fsprogs feat(26.04): add e2fsprogs Feb 3, 2026
@lczyk
Copy link
Collaborator

lczyk commented Feb 5, 2026

@lczyk lczyk removed the Blocked Waiting for something external label Feb 5, 2026
@alesancor1 alesancor1 added the Blocked Waiting for something external label Feb 7, 2026
@alesancor1
Copy link
Member

Blocked by #879

summary: Integration tests for e2fsprogs

execute: |
rootfs="$(install-slices e2fsprogs_standard)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that standard contains everything, but we should test each of the slices separately to guarantee that they can work standalone.

For a better coverage I'd suggest to just run some smoke tests for the standard slice (just run -v and -h for the binaries) and move the functionality testing to a test for each individual slice.

Use variant and separated sh scripts for each variant (anticipating @lczyk's comment :p ) - For reference: example

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep. seconded ☝️ :) makes for a muuuch cleaner code imo (bash linting + syntax highlight of the test files) + spread runs all the bits as separate tests, so if one breaks it does not take the whole test down

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a PR specific comment.

@lczyk @cjdcordeiro we have a bunch of tests for libraries like this one, but some other libraries are not tested - should we put a convention in place to always do a simple existence check for the files?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think lots of lower hanging fruit and i'm not convinced that simple existence test is really testing anything that installability tests are not testing already. honestly, i'd even say to remove this and libss2 tests

@ROCKsBot ROCKsBot requested a review from a team February 8, 2026 01:41
@lczyk
Copy link
Collaborator

lczyk commented Feb 17, 2026

@lczyk lczyk removed the Blocked Waiting for something external label Feb 17, 2026
summary: Integration tests for e2fsprogs

execute: |
rootfs="$(install-slices e2fsprogs_standard)"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep. seconded ☝️ :) makes for a muuuch cleaner code imo (bash linting + syntax highlight of the test files) + spread runs all the bits as separate tests, so if one breaks it does not take the whole test down

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think lots of lower hanging fruit and i'm not convinced that simple existence test is really testing anything that installability tests are not testing already. honestly, i'd even say to remove this and libss2 tests

slices:
libs:
essential:
- libc6_libs
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- libc6_libs
- libc6_libs
- libcom-err2_libs

libcom-err2_libs requires libcom_err.so.2 from libcom-err2. here's ldd output:

	linux-vdso.so.1 (0x0000ffff8084c000)
	libcom_err.so.2 => /usr/lib/aarch64-linux-gnu/libcom_err.so.2 (0x0000ffff80740000)
	libc.so.6 => /usr/lib/aarch64-linux-gnu/libc.so.6 (0x0000ffff80540000)
	/lib/ld-linux-aarch64.so.1 (0x0000ffff80810000)

Comment on lines +27 to +29
/usr/sbin/fsck.ext2:
/usr/sbin/fsck.ext3:
/usr/sbin/fsck.ext4:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/usr/sbin/fsck.ext2:
/usr/sbin/fsck.ext3:
/usr/sbin/fsck.ext4:
/usr/sbin/fsck.ext2: # Symlink to e2fsck
/usr/sbin/fsck.ext3: # Symlink to e2fsck
/usr/sbin/fsck.ext4: # Symlink to e2fsck

Comment on lines +42 to +44
/usr/sbin/mkfs.ext2:
/usr/sbin/mkfs.ext3:
/usr/sbin/mkfs.ext4:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/usr/sbin/mkfs.ext2:
/usr/sbin/mkfs.ext3:
/usr/sbin/mkfs.ext4:
/usr/sbin/mkfs.ext2: # Symlink to mke2fs
/usr/sbin/mkfs.ext3: # Symlink to mke2fs
/usr/sbin/mkfs.ext4: # Symlink to mke2fs

/usr/libexec/e2fsprogs/e2scrub_all_cron:
/usr/libexec/e2fsprogs/e2scrub_fail:
/usr/sbin/e2scrub:
/usr/sbin/e2scrub_all:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e2scrub and e2scrub_all are bash scripts. they should be in a slice called scripts, and have a dependency on bash_bins, base-files_bin (due to the shebang) and a pile of utils. then, if you want a slice named after a function, you could do something like this:

scrub:
  essential:
    - e2fsprogs_scripts

scripts:
  ...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also these should be tested. it looks like they both have help if you supply some non-existent flag. you can then assert the return code is 2. this is a bit annoying to do with the -e flag (which i nonetheless recommend strongly. here is a small working example

#!/usr/bin/env -S bash -ex

function my_program {
  echo "doing something"
  return 33
}

status_file="$(mktemp)"
trap "rm -f $status_file" EXIT

out="$(
  set +e
  my_program
  echo "$?" >"$status_file"
)"
code="$(cat "$status_file")"

echo "code: $code"
echo "output: $out"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, i've done the analysis of e2scrub and e2scrub_all and they depend on the following binaries:

e2scrub:

echo
awk
date
dumpe2fs
fstrim
grep
lsblk
lvs
realpath
sleep

e2scrub_all:

echo
dumpe2fs
grep
lsblk
lvs
readlink
sed
systemd-escape

i think you can skip the systemd part since the script accounts for it possibly not being there, but do add a comment saying that systemd-escape is needed for full systemd functionality

- libext2fs2t64_libs
- libuuid1_libs
contents:
/usr/sbin/e2label:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/usr/sbin/e2label:
/usr/sbin/e2label: # Symlink to tune2fs

/usr/sbin/dumpe2fs:
/usr/sbin/e2freefrag:
/usr/sbin/e2image:
/usr/sbin/e2mmpstatus:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/usr/sbin/e2mmpstatus:
/usr/sbin/e2mmpstatus: # Symlink to dumpe2fs

@lczyk
Copy link
Collaborator

lczyk commented Feb 17, 2026

@lczyk lczyk added the Blocked Waiting for something external label Feb 17, 2026
@lczyk lczyk mentioned this pull request Feb 17, 2026
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Blocked Waiting for something external

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants