systemd-boot-builder.py: fix crash, clean up code

[m-bdf]

Motivation for this change

Fix a crash introduced in e2be5dc and clean up the code to improve coding style consistency, match Python 3 best practices and make the script more readable and easy to maintain overall.

Followed PyLint's advice, which now rates the file 10.00/10 with these reports disabled:

invalid-name,missing-docstring,wrong-import-position,unspecified-encoding
line-too-long,too-many-locals,too-many-branches,too-many-statements

(changes are grouped, so per-commit diffs are a lot clearer than the PR's unified one)

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 22.05 Release Notes (or backporting 21.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[edrex]

I spent ~30 minutes reading through these changes, and no issues jumped out. Overall it's a huge improvement in code quality. The test suite over at nixos/tests/systemd-boot.nix still seems to pass (if I ran it correctly).

[jonringer]

cc better python devs than I. @FRidh @Mic92

[edrex]

also CC @danielfullmer who wrote the tests, @Enzime who authored and @pennae who reviewed #150408

[Enzime]

EDIT: My mistake I didn’t see the other commits

Can you write a test case for the specific crash this fixes?

To anyone who wants to review this, I would click on the commits tab and look at them individually

[edrex]

+1 that #155054 (comment) should be reverted before this is merged

[Mic92]

I also did not saw any concerns when reading the code. But maybe address @edrex comment from above.

[lucc]

I also looked through these changes and they are looking good to me.

This should fix #159623.

Can I help to speed this up? (I am currently affected by this and am "motivated" :/ )
@Enzime what kind of test should be written where for this script? The only place where I see it referenced is the nix file next to it and that does only do a static code check with mypy during installation to the nix store:

nixpkgs/nixos/modules/system/boot/loader/systemd-boot/systemd-boot.nix

Lines 50 to 59 in 6471f13

	checkedSystemdBootBuilder = pkgs.runCommand "systemd-boot" {
	nativeBuildInputs = [ pkgs.mypy ];
	} ''
	install -m755 ${systemdBootBuilder} $out
	mypy \
	--no-implicit-optional \
	--disallow-untyped-calls \
	--disallow-untyped-defs \
	$out
	'';

There are some tests for the systemd-boot setting in nixos/tests/systemd-boot.nix but the bug that should be fixed in this PR is in the python script: undefined variable profile in line 230:

nixpkgs/nixos/modules/system/boot/loader/systemd-boot/systemd-boot-builder.py

Lines 220 to 230 in e2be5dc

	gens = get_generations()
	for profile in get_profiles():
	gens += get_generations(profile)
	remove_old_entries(gens)
	for gen in gens:
	try:
	write_entry(*gen, machine_id)
	if os.readlink(system_dir(*gen)) == args.default_config:
	write_loader_conf(*gen)
	except OSError as e:
	print("ignoring profile '{}' in the list of boot entries because of the following error:\n{}".format(profile, e), file=sys.stderr)

[edrex]

@lucc AFAIK this doesn't have a fix for #159623, which is a regression from ce8c102. Reverting that change should fix the error reporting, revealing the underlying error (in my case it was ENOSPACE, indicating my boot partition was full). Since this branch hasn't been rebased since that was merged, there would be a merge conflict i guess.

[edrex]

Since ce8c102 has been backported to 21.11, I think that should be reverted in a separate commit before this is merged, and then that commit can be backported. @m-bdf do you want to rebase prepending a revert commit since this seems ready to merge otherwise?

[sternenseemann]

Resolved the merge conflict and force-pushed. Can confirm this PR solves #15962 for me (i. e. garbled error messages).

I think we should either merge this or revert the regression quickly, as this is a pretty non-obvious pitfall and risks people inadventingly booting into old generations.

[m-bdf]

Thanks everyone for taking the time to review this PR 💕

@edrex for some reasons I won't explain here, I've been working on a NixOS live USB for a few days already, and trying to clone the nixpkgs repo (unsurprisingly) freezes the entire system, so I haven't been able to revert ce8c102 yet.

In the meantime, @sternenseemann rebased this PR on master, making it mergeable as-is by resolving the conflict in the first commit instead of prepending a proper revert commit. Thus, do you think backporting 7c21a5f (or even this entire PR) would be reasonable?

[edrex]

sorry slow response, some time AFK

do you think backporting 7c21a5f (or even this entire PR) would be reasonable?

It seems like just backporting the regression fix would have the smallest impact, but I'm new here so not sure what the guidelines are for backporting. I feel like a fix should definitely be backported since the regression was.

Also, didn't mean to steamroll @Enzime's request:

Can you write a test case for the specific crash this fixes?

At least characterizing the crash seems like good process.

[Mic92]

I also think we should only backport a fix and not the whole refactoring part of this pr.

[FRidh]

Paths shouldn't be represented anymore as strings but as pathlib.Path objects.

[FRidh]

how is libc.so.6 found? Should we hardcode a path?

[FRidh]

please avoid from imports so it is clear directly in code where something comes from.

[FRidh]

Avoid string concatenation when the strings represent paths. Instead, use pathlib and the / operator.

[FRidh]

For clarity I would

Suggested change

	return f"{generation_dir}/specialisation/{gen.specialisation}" if gen.specialisation else generation_dir
	specialisation_dir = f"{generation_dir}/specialisation/{gen.specialisation}"
	return specialisation_dir if gen.specialisation else generation_dir

Also, pathlib.

[m-bdf]

Thanks for your review @FRidh!

I'm working on the changes you've suggested, using pathlib.Path everywhere is quite a big change though, so it's going to take a bit of time to implement properly 😄