Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nixos/systemd-boot: fix systemd-boot-builder dowgrade to fail #172849

Merged
merged 1 commit into from
May 28, 2022

Conversation

waldheinz
Copy link
Contributor

@waldheinz waldheinz commented May 13, 2022

Since, 4ddc788 systemd-boot-builder
is broken in two ways:

  • if no systemd-boot is currently installed and the NIXOS_INSTALL_BOOTLOADER
    env variable is not set, it will try to run "bootctl update", which will fail
  • if the currently installed systemd-boot version is newer than the version
    we're about to install, it will also try to run "bootctl update", which will fail

This patch changes the behaviour,

  • for the first case to still fail, but not even bother to try running
    "bootctl update" and instead throwing erroring out with an exception
  • for the second case to leave the newer version in place, restoring
    the pre - 4ddc788 behaviour

To do the proper version check a new "should_update" helper function was introduced,
mimicing the compare_product C function from bootctl.

This change allows to again switch to a different NixOS configuration which contains
an older systemd-boot.

@waldheinz waldheinz requested a review from dasJ as a code owner May 13, 2022 10:44
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels May 13, 2022
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels May 13, 2022
@waldheinz waldheinz force-pushed the systemd-boot-builder-downgrade branch 2 times, most recently from bd65afe to e7ba0c6 Compare May 20, 2022 08:48
Copy link
Member

@sorki sorki left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@waldheinz
Copy link
Contributor Author

Could you please have a look, @dasJ ?

@waldheinz
Copy link
Contributor Author

See also systemd/systemd#23450

@waldheinz waldheinz marked this pull request as ready for review May 20, 2022 09:47
@waldheinz
Copy link
Contributor Author

Ping @jrobsonchase

@Artturin Artturin added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label May 22, 2022
@waldheinz waldheinz force-pushed the systemd-boot-builder-downgrade branch from eee2e62 to 50e166e Compare May 22, 2022 08:48
@waldheinz waldheinz force-pushed the systemd-boot-builder-downgrade branch from 50e166e to b541dd9 Compare May 24, 2022 10:38
@waldheinz
Copy link
Contributor Author

waldheinz commented May 24, 2022

I improved the commit with what @mweinelt suggestend and additionally renamed the variables to have available_ (what we'd like to update to) and installed_ (what's currently installed) prefixes, making the code easier to follow.

@waldheinz waldheinz force-pushed the systemd-boot-builder-downgrade branch from b541dd9 to a6f293e Compare May 25, 2022 06:58
@waldheinz waldheinz requested a review from SuperSandro2000 May 25, 2022 07:09
Copy link
Member

@dasJ dasJ left a comment

Choose a reason for hiding this comment

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

I don't feel comfortable reviewing that Python code.
If others think it's good, I have no objections to the change in general.
I added the backport label for 22.05 so we can get it on the release branch as well since we have already branched off.

Copy link
Member

@mweinelt mweinelt left a comment

Choose a reason for hiding this comment

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

I think this will work.

One more nit, regarding the regex parsing. It didn't originate in this pull request, but since you're touching it we might as well.

Since, 4ddc788 systemd-boot-builder
is broken in two ways:

  * if no systemd-boot is currently installed *and* the NIXOS_INSTALL_BOOTLOADER
    env variable is not set, it will try to run "bootctl update", which will fail
  * if the currently installed systemd-boot version is newer than the version
    we're about to install, it will also try to run "bootctl update", which will fail

This patch changes the behaviour,

  * for the first case to still fail, but not even bother to try running
    "bootctl update" and instead erroring out with an exception
  * for the second case to leave the newer version in place, restoring
    the pre - 4ddc788 behaviour

To do the proper version check a new "should_update" helper function was introduced,
mimicing the compare_product C function from bootctl. If the following systemd
issue gets resolved, we would have a nice way to get rid of this function:

> systemd/systemd#23450

This change allows to again switch to a different NixOS configuration which contains
an older systemd-boot.

Co-authored-by: Martin Weinelt <[email protected]>
@waldheinz waldheinz force-pushed the systemd-boot-builder-downgrade branch from 67a6d16 to a30de3b Compare May 28, 2022 11:18
@mweinelt mweinelt merged commit c48756a into NixOS:master May 28, 2022
@waldheinz
Copy link
Contributor Author

@mweinelt I wanted to add that I just tested this again and it still works. Thanks for merging!

@github-actions
Copy link
Contributor

Successfully created backport PR #175115 for release-22.05.

@danielfullmer
Copy link
Contributor

Shouldn't this and #175611 both get backported to NixOS 21.11 as well? I thought we had an additional month of support for 21.11.

I was unable to rollback from 22.05 back to 21.11, since the 21.11 branch doesn't include these changes.

(This is potentially a good example of why tying the bootloader entry generation to the NixOS generation is problematic, and that whatever program generates bootloader entries from NixOS bootspec definitions should be independently upgradable.) CC @grahamc

@RuRo
Copy link
Contributor

RuRo commented Jun 24, 2022

I'm a bit late to the party, but I just wanted to point out that the

leaving systemd-boot X in place (Y is not newer)

message is a bit poorly worded, IMHO.

Maybe, I am just paranoid, but to me this message reads like something went wrong when trying to upgrade systemd-boot: the installed version of systemd-boot is newer than the one in nixpkgs. This message gets printed on every nixos-rebuild switch. Perhaps, systemd-boot-builder shouldn't print anything when installed_version == available_version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants