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 refusing to update #175611

Merged

Conversation

waldheinz
Copy link
Contributor

Handling of the string length condition in should_update
was broken, as evident with the log message

leaving systemd-boot 246 in place (250.4 is not newer)

when updating an old NixOS system.

Fixes a30de3b

Sorry, I goofed up following the code in bootctl.c

@waldheinz waldheinz requested a review from dasJ as a code owner May 31, 2022 14:50
@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 31, 2022
@waldheinz
Copy link
Contributor Author

Ping and sorry for the noise, @mweinelt

@mweinelt
Copy link
Member

What is it with those length checks anyway? What are you trying to port? I think we can just rely on that last return statement.

>>> "" < "250.4"
True
>>> "246" < "250.4"
True
>>> "246.1" < "250.4"
True
>>> "246.99" < "250.4"
True
False
>>> "250.3" < "250.4"
True
>>> "250.5" < "250.4"
False

@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 31, 2022
@waldheinz
Copy link
Contributor Author

I tried to implement what that compare_product function does. I really don't know what's the purpose, I just wanted to copy the functionality and failed.

@mweinelt
Copy link
Member

mweinelt commented May 31, 2022

The upstream functions returns a tristate (-1, 0, 1) while we return a bool. Our function is also semantically different, which is fine. If you can't come up with a reason to keep the length checks I'm for dropping them.

When should we update?

  • to is newer (>) than from

When shouldn't we update?

  • to is the same (=) as from
  • to is older than (<) from

That's basically only updating when to > from, which is the return statement.

@waldheinz
Copy link
Contributor Author

I agree, will update the PR tomorrow. It's unlikely that anyone is hurt by the current state because only updates are affected and the old bootloader was obviously good enough for the system.

Handling of the string length condition in should_update
was broken, as evident with the log message

> leaving systemd-boot 246 in place (250.4 is not newer)

Discussion with @mweinelt came to the conclusion
that Python's "<" operator already does what we need,
so the should_update function can be dropped.

Fixes a30de3b
@github-actions
Copy link
Contributor

github-actions bot commented Jun 1, 2022

Successfully created backport PR #175728 for release-22.05.

@waldheinz waldheinz deleted the systemd-boot-builder-does-not-update branch June 1, 2022 10:41
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants