GNOME Bugzilla – Bug 739416
List deployment versions in bootloader UI
Last modified: 2014-11-11 02:33:24 UTC
Created attachment 289689 [details] [review] First cut Listing the deployment's version label in a bootloader UI is more user-friendly than an index value. I imagine this will require a few iterations, so the attached patch is only a first cut. Presently if a version can't be found for a deployment then the boot entry title just falls back to the old style. I think Colin had some other criteria for this but I need more clarification. Haven't actually tested this yet because I'm trying to figure out how to do so without running a whole compose.
Review of attachment 289689 [details] [review]: I think this is pretty good. A further elaboration of this would be to include the osname iff there are multiple. (It's possible to use ostree to parallel install in separate roots the same OS version, with separate /etc and /var streams)
I thought of that afterward but then wasn't sure if PRETTY_NAME from /etc/os-release would sufficiently identify the OS. Didn't consider the use case you mention, so I'll go ahead and do that. Do you want me to make the fallback case look similar? Don't know how often it would get used in practice, but maybe something like: $PRETTY_NAME ostree[:$OSNAME]:($VERSION|$INDEX) IOW, show the osname iff there are multiple like you said, and show the index iff no version is available?
Created attachment 289898 [details] [review] Revised patch Revised patch uses the title format in comment #2.
Review of attachment 289898 [details] [review]: Could you add a test for this in tests/admin-test.sh? Something like: diff --git a/tests/admin-test.sh b/tests/admin-test.sh index 94aef64..cc92892 100755 --- a/tests/admin-test.sh +++ b/tests/admin-test.sh @@ -29,6 +29,7 @@ ostree admin --sysroot=sysroot deploy --karg=root=LABEL=MOO --karg=quiet --os=te ostree admin --sysroot=sysroot status | tee status.txt assert_file_has_content status.txt 'Version: 1.0.10' +assert_file_has_content sysroot/boot/loader/syslinux.conf 'TestOS 1.0.10' echo "ok deploy command" ::: src/libostree/ostree-sysroot-deploy.c @@ +1367,3 @@ + + title_key = g_string_new (val); + g_string_append (title_key, " ostree:"); I go back and forth on whether this should be in the UI. In a multi-boot scenario it makes sense, but if you're *only* using ostree it's kind of noise =/ Not sure. Maybe demote it to a suffix? e.g. Fedora Atomic Host 21.0 (ostree) @@ +1653,3 @@ + * here is not fatal to the whole operation. We just gracefully + * fall back to the deployment index. */ + ostree_sysroot_get_repo (self, &repo, cancellable, NULL); I add (void) for things like this to please static analyzers like Coverity that want one to consistently check function return values.
Created attachment 289937 [details] [review] Revised patch v2 Updated per your comments. See the commit message for the overall title format.
Review of attachment 289937 [details] [review]: Ok, this looks pretty good to me!
https://git.gnome.org/browse/ostree/commit/?id=e5b147c64386ed7835a66d0770c69e7ce9dc5225
This actually broke the syslinux backend. It does: else if (!parsing_label && (g_str_has_prefix (line, "DEFAULT "))) { saw_default = TRUE; if (g_str_has_prefix (line, "DEFAULT ostree:")) regenerate_default = TRUE; Which is kind of conceptually broken, but I was trying to avoid changing the default for existing syslinux installations. We'll need to update this code to also do strstr("(ostree") as well or something?
Created attachment 290296 [details] [review] Followup patch I'm not yet familiar with the bootloader code at all and barely know the first thing about SYSLINUX so I'm pretty much trusting your suggestion, but also peppering the code with additional comments.
Review of attachment 290296 [details] [review]: ::: src/libostree/ostree-bootloader-syslinux.c @@ +210,3 @@ + * but this hack is at least noted in the code that builds + * the title to hopefully avoid regressions. */ + if (strstr (line, "(ostree") != NULL) We need to search for *both* patterns right? For the case where we're upgrading from an ostree that used "ostree: " to one that uses "(ostree" right? I think the flow would be: - Running old version - Deploy new tree (and still writing *old* titles) - Reboot into new tree - Upgrade *again* using new ostree (and thus read the *old* title config, but deploying *new*)
Created attachment 290371 [details] [review] Followup patch Yeah you're right. More generally, I guess it's gonna have to know about *all* the variations if there's any further changes in the future.
Review of attachment 290371 [details] [review]: This looks good to me. An elaboration of this might be a magic comment in the syslinux config, like: # OSTree managed default DEFAULT blah But this is fine.
https://git.gnome.org/browse/ostree/commit/?id=5c9e83c02caa429045d26e78b932bc54e853ec16