From: Colin Watson <cjwatson@ubuntu.com>
To: grub-devel@gnu.org
Subject: Breakage from grub-mkconfig changes for grub-file
Date: Mon, 23 Dec 2013 22:01:41 +0000 [thread overview]
Message-ID: <20131223220141.GA25169@riva.ucam.org> (raw)
ec824e0f2a399ce2ab3a2e3353d372a236595059 introduced extensive changes to
grub-mkconfig, which among other things arranged to run all but a
hardcoded list of grub.d scripts once for each of several platforms with
their output enclosed in an "if" block. I do not feel that this change
was well-thought-out, and I think it should be rethought or reverted
before 2.02. I didn't see anything about it here in advance - did I
miss a thread?
The problems I have with it are illustrated by its effects on the Debian
patch. I must emphasise that I don't think this is unique to the case
of distributions with non-trivial patch sets, and that it's also likely
to affect users who have made reasonable changes to /etc/grub.d/ locally
(as they're entitled to do); distributions are just a useful
early-warning system here.
1) Awkward hardcoded list; poor configurability
00_header, 30_os-prober, 40_custom, and 41_custom are run only once.
The Debian patch set has an additional script which is not
platform-dependent and should be run only once, namely
05_debian_theme, so I had to add another case here. Users will surely
have other such files; not only do they have to know on upgrade that
they need to take care of this, but they have no recourse that doesn't
involve editing $prefix/bin/grub-mkconfig, which is not a file that
should normally be edited by the system administrator; changes to that
file will not normally persist on upgrades.
This should be redesigned so that there is some way to declare in a
grub.d script that it requires multi-platform support and should be
run multiple times. (It *must* be this way round so that upgrades
work properly.)
2) Strange ad-hoc platform names
The platform names used in grub-mkconfig (x86 i386-xen-pae x86_64-xen
mips mipsel sparc64 powerpc ia64 arm arm64) are not the same as the
platform names used in the GRUB build system, but yet they're exported
across the interface to /etc/grub.d/ as GRUB_PLATFORM. This is messy
and confusing, and it's not clear what promises we make about future
changes here.
We should rationalise this before issuing anything as part of a stable
release, perhaps by adopting the same target_cpu/platform terminology
used in the build system. Furthermore, if we made the namespaces
match up then it would be fairly straightforward to only run grub.d
scripts for platforms for which we have installed GRUB modules, which
seems as though it would be sensible.
3) Breaks function definitions
In the GRUB script language, "function" is only permitted at the top
level. This may be an oversight since bash doesn't share this
restriction and GRUB script generally tries to look like bash;
nevertheless it exists today. Part of the Debian patch set causes
10_linux to emit a function definition, which now causes a syntax
error.
I think my preferred fix here would be to implement functions other
than at the top level, but it seems a bit rash to try to cram that
into 2.02.
4) Smaller bugs
Aside from the bug fixed in 77ec462a568fc9c89ef45e960bf33b5de73140fb,
I'm pretty sure that the condition for the "x86" platform is buggy;
shouldn't it have an extra "-a" in there? This sort of thing makes me
worried about the level of testing these changes have had.
The grub-file tool seems reasonable and useful to me, but can we just
revert the grub-mkconfig parts of these changes until after 2.02 so that
the effects of these interface changes can be considered more carefully?
It looks like it was part of a new feature rather than a bug-fix.
I noticed this while testing Debian packages of 2.02~beta1.
Unfortunately, I don't think I can salvage this into something I can
upload without extensive interface changes of the sort that I'm not
comfortable making downstream.
Thanks,
--
Colin Watson [cjwatson@ubuntu.com]
next reply other threads:[~2013-12-23 22:01 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-12-23 22:01 Colin Watson [this message]
2013-12-23 22:21 ` Breakage from grub-mkconfig changes for grub-file Vladimir 'φ-coder/phcoder' Serbinenko
2013-12-24 0:34 ` Colin Watson
2013-12-24 0:38 ` Vladimir 'φ-coder/phcoder' Serbinenko
2013-12-24 1:27 ` Colin Watson
2013-12-24 2:56 ` Andrey Borzenkov
2013-12-24 3:07 ` Vladimir 'φ-coder/phcoder' Serbinenko
2013-12-24 12:10 ` pfsmorigo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20131223220141.GA25169@riva.ucam.org \
--to=cjwatson@ubuntu.com \
--cc=grub-devel@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.