Various tweaks to make mkosi-initrd easier to use - #4227
Conversation
Claude review of PR #4227 (066034d)Must fix
Suggestions
Nits
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if reset and len(values) == 1 and values[0] == "": | ||
| if delimiter is None: | ||
| # by default, split on commas and any whitespace | ||
| values = value.replace(",", " ").split() |
There was a problem hiding this comment.
Claude: suggestion: The new default value.replace(",", " ").split() treats all whitespace as delimiters. The old behavior with delimiter="," was value.replace(",", "\n").split("\n"), which preserved spaces within values.
This matters for settings whose values can contain spaces, particularly path-valued settings like ExtraTree, SkeletonTree, BaseTree, RuntimeTree, SandboxTree, PackageDirectory, etc. A config value like /path/to my files would now be split into three tokens.
While rare in practice, this is a silent behavior change that could break existing configurations.
There was a problem hiding this comment.
As discussed extensively in the commit message, this is intentional.
There was a problem hiding this comment.
I'm really not sure we should break this compat. While we both probably abhor whitespace in path names, I fear they might be not unheard of nowadays.
c0b2879 to
69b96c0
Compare
c6de23a to
8b08663
Compare
| if reset and len(values) == 1 and values[0] == "": | ||
| if delimiter is None: | ||
| # by default, split on commas and any whitespace | ||
| values = value.replace(",", " ").split() |
There was a problem hiding this comment.
I'm really not sure we should break this compat. While we both probably abhor whitespace in path names, I fear they might be not unheard of nowadays.
The tools that mkosi invokes are often quite verbose. So let's also print what those executed commands are by default. This makes it easier to figure out what is going on and doesn't change the total number of printed lines too much.
For some reason github doesn't allow me to reply under that comment. But maybe it's better to break it out into a top-level thread anyway. So… I thought about excluding those "user-controlled paths" from this change too. If you think that's better, I can do that. Nevertheless, I seriously doubt that |
This makes the callers a bit simpler because then they don't need to do explicit conversions of the returned object. There are a few callers which previously turned the value into a set, and one other caller which passed the value to make_cpio, which turns it into a list with sorted(files). So the change should be a noop.
Configuration like 'FirmwareFiles=intel/ibt-0093-* intel/ipu/*' would be interpreted as a single word. This is surprising and not useful. In practice, none of the names that we use should ever have a space in them. Change the parsing for host names, profile names, artifact output lists, includes, dependency names, scripts, repository names, manifest formats, repart directories, packages, package directories, trees, files to remove, uki profiles, initrds, device trees, module and firmware include/exclude lists to accept any-whitespace|comma instead of just newline|comma as separators. (The parsers that used space|newline as separators are not touched.) Arguably, this is a compat break. But I think it's unlikely to matter in any practical case and the removal of the surprising behaviour is more important. The case where a space would be most likely to be used is RemoveFiles… Fortunately that setting takes a glob so the user can just "escape" the path with space as '?' or something to make it work.
Follow-up for 2671849.
This is quite useful to figure out what configuration is used. (I considered adding an option to allow arbitrary mkosi verbs. But most verbs either don't make sense or don't work. E.g. 'shell' could be potentially useful but it doesn't support cpio atm. So limiting this to 'summary' for now seems OK. Having an easy way to show the config will be useful even if we add a more generic hookup into mkosi later.)
8b08663 to
066034d
Compare
That's true, everything but null bytes and path separators is fine, but I think of the "weird" characters, spaces are by far the most common. I've seen commas used in file names, but we never supported this and nobody ever complained about this, so I guess nobody ever ran into this. We have had users running into the problem that they assumed whitespace would split, so I agree this is a real issue for users, but I'm not sure exchanging one for the other is the solution. I think a way to have our cake and eat it, too, would be to run everything through shlex (the |
Yes, that could be done too. But that's a compat break too. I'd be happy to implement that, if there's agreement that this is the way to go. |
|
I agree with @keszybz, I think it will be extremely unlikely to see paths with spaces in mkosi config files, let's take a a risk here, though I won't stop anyone from implementing the shlex solution in a follow up PR either. |
4 participants
No description provided.