nixos/systemd-boot-builder: Allow characters like before #334473
Conversation
github-actions
bot
added
6.topic: nixos
|
Should we still assert that specialisation names don't contain |
|
Hm.... Well, in theory we could modify the regex to something more complicated like |
|
Yeah, so I meant, should we not have an assertion in nixos then that fails if a specialisation name has a |
|
Well, |
|
Actually, no, regardless of any of this, you just can't have a specialisation name that ends in a valid boot count. That will screw up systemd-boot. So that must actually be a bug we've had all along. So yea, we can use a better regex, and we can assert that with systemd-boot enabled, you're not allowed to have a specialisation whose name ends in a valid boot count. |
ofborg
bot
added
10.rebuild-darwin: 0
| @@ -65,7 +65,7 @@ def from_path(cls: Type["Entry"], path: Path) -> "Entry": | |||
| # Matching nixos*-generation-$number*.conf | |||
| rex_generation = re.compile(r"^nixos.*-generation-([0-9]+).*\.conf$") | |||
| # Matching nixos*-generation-$number-specialisation-$specialisation_name*.conf | |||
| rex_specialisation = re.compile(r"^nixos.*-generation-([0-9]+)-specialisation-([a-zA-Z0-9_]+).*\.conf$") | |||
| rex_specialisation = re.compile(r"^nixos.*-generation-([0-9]+)-specialisation-([^+]+).*\.conf$") | |||
There was a problem hiding this comment.
Let’s be as restrictive as possible here, rather than loosening the grammar. We’ve already gotten bitten by having allowed too much in the past; allowing literally any character other than + is probably setting ourselves up for regret. How about just [a-zA-Z0-9_-]+? (And I guess we should probably match against the counter format as well?)
You said something about specializations starting with generation breaking things, too? Can we check for that?
There was a problem hiding this comment.
I think I was wrong about the generation thing.
We can't be particularly restrictive here, because the whole reason we have the bug in the first place is that specialisation names used to be unrestricted. To maintain compatibility, we have to be as relaxed as possible.
Now, as I said above, I've realized that we've always had problems if your specialisation name ended in a valid boot count, so I'm currently making a change to assert against that. And the other problem is that, because of some faulty systemd code, you can't have a + in your specialisation name if you're using boot counting. So since boot counting is a new feature, I don't mind adding an assertion in that case against having + in the name at all.
There was a problem hiding this comment.
Right, but we’ve learned that we cannot be as permissive as we were and need to break compatibility. Keeping compatibility with arbitrary strings is no longer an option. To avoid further misery, we should be as strict as possible under the constraints of existing reality. We’ve found that disallowing dashes is too far, but I don’t think that means we need to support completely arbitrary strings. After all, anyone who decided + would be a nice character to include in their specialization name already has a problem.
So I think that, rather than clinging to backwards compatibility with something that was way too permissive, we pick the most restrictive thing we can that doesn’t break everyone’s setup. I’d be surprised if there are many people at all with specializations that don’t match [a-zA-Z0-9_-]+, given Nix’s identifier syntax.
There was a problem hiding this comment.
Like, there’s no blocker to loosening the regex some more if we find out that it’s really too strict. But as we’re learning now, making things stricter is hard. We have to do it now, so we should make the most of it to minimize future regret.
There was a problem hiding this comment.
@emilazy I very strongly disagree. The point of this series of PRs is to fix a bug. It is not to improve the acceptable inputs. What you're describing does not fix the bug. It perpetuates the bug.
There was a problem hiding this comment.
I think we can consider adding restrictions at a later time. But I think it needs consideration and thoughtfulness, especially with regards to compatibility and migration. Given that this is a channel blocker, I think it's important to do the most obvious thing quickly, which is the maximally compatible thing.
There was a problem hiding this comment.
Well, I mean, arguably not being able to put boot counts is also a “bug” that started as soon as systemd-boot started looking at that, right? We’re already choosing to perpetuate that bug by fixing that and breaking compatibility. To avoid perpetuating it we’d have to patch systemd-boot or something. So I feel like we’ve already crossed that rubicon and ought to take steps to avoid having to cross it again.
But, how about just using the much more strict regex only if boot counting is on, since it’s going to break naming compatibility and need separate handling anyway?
There was a problem hiding this comment.
Sure but that bug has been around a long time and no one noticed; it's not a brand new regression that we need to fix.
I don't particularly see the point in using a more strict regex if boot counting is on. But admittedly, IMO the real solution to all these problems is to stop using file names as data, and to migrate to a more reasonable separate state file for tracking this stuff.
There was a problem hiding this comment.
Well, with this change we already literally are using a more strict regex if boot counting is on, it’s just only strict about one specific thing we regretted :)
But anyway I don’t want to block the channel further, just voicing my concerns about this coming back to bite us again later.
ElvishJerricco
force-pushed
the
systemd-boot-specialisation-regex
branch
from
571fe2e
to
05e1dd3
Compare
|
Ok, new changes. No longer reverting the assertion. Just making it much more specific, to account for A) A bug we've apparently always had and never known about, and B) Incompatible behavior with the new boot counting feature. The regex now matches anything for the specialisation, excluding for any boot count suffix. The aforementioned assertion, along with the fact that a name with a boot count suffix would have errored in the past, should make this both safe and maximally compatible. |
|
Closed in favor of #334526 |
Description of changes
Reverts #333952
Modify the regex in
systemd-boot-builder.pyto match specialisation names correctly.Similar to #333857, but this regex works with boot counting, I've reverted the faulty mitigation that was merged in the meantime, and I've added a test.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.