Steve McIntyre <steve@einval.com> writes:
> On Sat, Dec 24, 2016 at 02:25:48AM +0100, Philip Hands wrote:
>>Raphael Hertzog <hertzog@debian.org> writes:
>>...
>>> So I agree with Cyril and the d-i team, we should be cautious here.
>>>
>>> Let's focus everybody's energy on getting Phil's patch merged instead
>>> of continuing this discussion.
>>
>>The latest incarnation of which I think is close to ready:
>>
>> https://anonscm.debian.org/cgit/d-i/pkgsel.git/log/?h=pu/simple_tasksel
>>
>>I've squashed the commits together, so we now have the first (aae3196)
>>which implements the feature, and would probably be fine as it is (once
>>comments to the translators have been added as appropriate).
>>
>>The second commit (1bb1feb) adds a level of indirection in the
>>template, with code to populate it from some new debconf settings,
>>which allows one to then customise the menu via preseeding. This is not
>>in any way essential to the task in hand, but might well be useful for
>>others.
>
> I'll be honest - that code scares me right now. If this was simple,
> obvious stuff then I'd be pushing to try and get this in. But it's
> not. Comments like
>
> + # there is no need to do this twice, and it breaks [back] behaviour if you do
>
> don't help, and I honestly don't understand what
Fair point, and actually the code that comment applies to is only useful
when 'db_capb backup' is enabled, which for complicated reasons[0] it is
not at present, so I should just comment the lot out to avoid doubt.
> + db_subst pkgsel/simplified-tasksel $(echo $i | tr "a-z" "A-Z") "$subst"
>
> is doing when I read the code at 2am. Can you explain this better
> please?
The template that's going to present the question to the user, is this:
=-=-=-=-
Template: pkgsel/simplified-tasksel
Type: select
Choices-C: ${CHOICES-C}
Choices: ${CHOICES}
Description: Choose type of system to install
${LONGDESC}
=-=-=-=-
so the loop you're looking at:
=-=-=-=-
for i in choices choices-c longdesc ; do
db_get pkgsel/simplified-tasksel/$i
local subst=$(echo $RET | sed "s#[$]{DESKTOP}#$desktop#g")
db_subst pkgsel/simplified-tasksel $(echo $i | tr "a-z" "A-Z") "$subst"
done
=-=-=-=-
does the following for each of the template variables.
. Get the preseedable default value using db_get
. substitute any occurrences of ${DESKTOP} with the current value of
$desktop (so gnome unless you preseeded a different desktop)
. do the actual substitution, which currently needs the variable name to
be uppercased
I could make that rather simpler by naming the preseedable defaults with
uppercase names (e.g. pkgsel/simplified-tasksel/CHOICES) and uppercasing
everywhere, which would eliminate the need to do the tr.
Alternatively one could use lowercase variables in the template to get
the same effect (I didn't do that, to avoid confusing translators who
will be used to upper-case)
Anyway, it's far from clear that this code is needed, and if the extra
complication is a barrier, let's forget the preseedability aspect, and
simply concentrate on the first patch.
> To make this kind of change for stretch, we'll also need updates to
> translations directly in the installer and in the installation
> guide. I'm worried that we're doing this too late in the cycle - as
> KiBi says.
I agree. This is the wrong time to be doing this. If I'd had time
earlier in the cycle, I might well have done something about it then,
but ... kids, basically ;-)
Given that we're starting from where we're at, we seem to have a choice
of either adding translatable strings at this late stage, or dumping the
blends for another cycle -- neither option is perfect.
I happen to think that what I've knocked up results in a better user
experience, but then I never liked tasksel and almost never use the
defaults it presents, so I'm not really the target audience for this.
Cheers, Phil.
[0] I was trying to make it so that tasksel would have a [BACK] button,
and one could thus go:
Oh, I wonder what "other use cases" means ...
Argh! My eyes!
[BACK]
Phew! That's better, I'll have a Desktop in that case.
If you do that, and you allow the db_subst bit to be executed a second
time, it breaks, hence the:
if [ -z "$desk_subst" ] ; then
...
desk_subst=true
code.
However this all turns out to be irrelevant because the 'db_go' in here:
https://anonscm.debian.org/cgit/tasksel/tasksel.git/tree/tasksel-debconf
where is says "intentionally unguarded" and _should_ cause the script to
abort (because of set -e) with an error code of 30 if one selects
"BACK", doesn't.
It always returns 0, regardless.
So you never find out that the user wanted to go back.
I presume this is a bug in debconf somewhere, but none of the related
code seems to have changed since Joey wrote it, and back buttons work
elsewhere, so this is quite puzzling.
To avoid the problem I got rid of the 'db_capb backup' so we don't have
back buttons anyway -- I should just comment out the related code for
now, with a comment about the breakage in tasksel/db_go, so that it
might get fixed in future without someone (me probably) having to go
through all the same diagnosis to track down what's wrong.
--
|)| Philip Hands [+44 (0)20 8530 9560] HANDS.COM Ltd.
|-| http://www.hands.com/ http://ftp.uk.debian.org/
|(| Hugo-Klemm-Strasse 34, 21075 Hamburg, GERMANY
Attachment:
signature.asc
Description: PGP signature