Hi Adrian,
On 10/19/2017 01:04 PM, John Paul Adrian Glaubitz wrote:
[...]
The patch for d-i/partman-auto increases the size of the NewWorld boot
partition to 10 MB for all selections.
I just looked at your changes:
--- a/recipes-powerpc-powermac_newworld/atomic
+++ b/recipes-powerpc-powermac_newworld/atomic
@@ -1,6 +1,6 @@
partman-auto/text/atomic_scheme ::
-1 1 1 hfs
+10 10001 10 hfs
$bootable{ }
method{ newworld } .
Looking at [1], shouldn't this just be "10 1 1"?
I actually mistook <minimal size> (first column) and <maximal size>
(third column) in this case, but the idea is to have a minimum of 10 MB
for this partition, so setting both cols to 10 should work as intended.
From [1] I don't know what effect a smaller number for maximal size
compared to minimal size would have, but [1] also tells that if
<priority> (second column) "is too small (relative to the priority of
the other partitions) then this partition will have size close to
<minimal size>. That's why it is recommended to give small partitions a
<priority> larger than their <maximal size>.". Hence I chose the
greatest priority value for this partition, although this might not be
needed at all, if both extreme values are the same.
I'll check if there's a difference between "10 1 1" and "10 1 10" and adapt.
[1]:
https://anonscm.debian.org/cgit/d-i/debian-installer.git/tree/doc/devel/partman-auto-recipe.txt
The code for d-i/grub-installer currently executes the
`nw_select_offs_part` part but always selects
`/dev/sda2` as NewWorld boot partition afterwards as I don't yet know
how this will behave with the
debconf templates in place. The main part for NewWorld Power Macs
currently runs before `grub-ieee1275`
is installed in-target, as it was easier to concentrate things at this
specific case clause, but this
can be reordered later.
We can't use it as-is, even for the test CD images, as hard-wiring the
hard-drives can
lead to serious data loss. You have no guarantee at all that your
primary disk will
be "sda", it's completely non-deterministic, unfortunately.
Sure, that's another reason why I first wanted to integrate the new
debconf templates without the other code changes. :-)
Anther thing I have noticed:
+ ;;
+ */powermac_newworld)
+ info "$ARCH selected."
+ #db_progress STEP 1
+ #db_progress INFO grub-installer/part_newworld
+ offs_part=$( nw_select_offs_part )
+ info "offs_part = $offs_part"
You should rather match
"powerpc/powermac_newworld|ppc64/powermac_newworld" here.
Is this for information reasons? I started with your proposed match, but
assumed there wouldn't be any other super-architectures for NewWorld
Power Macs than "powerpc" or "ppc64" and so made it shorter. But I can
change that and also the other occurrences.
This hunk is completely unnecessary and is purely formatting changes
(tabs vs. spaces, most likely):
Is `grub-installer` actually using spaces for identation? I only see
tabs with my local editors, except for the matches of case clauses which
are usually placed after optional tabs (depending on indentation level)
and four spaces, the code corresponding to a match is indented by one
tab from the start of the "case" word.
Hence I think I actually stayed in the style of the other code for the
mentioned part. As I added a case statement to only execute this part if
a grub-efi* package is going to be installed, I had to indent the whole
block with an additional tab. Maybe you missed that new case statement.
I have to admit though that I changed the here document to not disrupt
the indentation, which is not a functional change of course.
- # Should we force a copy of grub-efi to be installed
- # to the removable media path too? Ask at low
- # priority, or can also be pre-seeded of course
- db_input low grub-installer/force-efi-extra-removable ||
[ $? -eq 30 ]
- db_go || exit 10
- db_get grub-installer/force-efi-extra-removable
- if [ "$RET" = true ]; then
- grub_install_params="$grub_install_params
--force-extra-removable"
- # Make sure this happens on upgrades too
- $chroot $ROOT 'debconf-set-selections' <<EOF
-$grub_package grub2/force_efi_extra_removable boolean true
-EOF
- fi
+ case $grub_package in
+ grub-efi*)
+ # Should we force a copy of grub-efi to be
installed
+ # to the removable media path too? Ask at low
+ # priority, or can also be pre-seeded of course
+ db_input low
grub-installer/force-efi-extra-removable || [ $? -eq 30 ]
+ db_go || exit 10
+ db_get grub-installer/force-efi-extra-removable
+ if [ "$RET" = true ]; then
+
grub_install_params="$grub_install_params --force-extra-removable"
+ # Make sure this happens on upgrades too
+ $chroot $ROOT 'debconf-set-selections'
<<-EOF
+ $grub_package
grub2/force_efi_extra_removable boolean true
+ EOF
+ fi
+ ;;
+ esac
For the following hunk I agree:
```
@@ -235,13 +667,32 @@ rootfstype="$(findfstype /)"
case $ARCH in
powerpc/chrp|powerpc/chrp_rs6k|powerpc/chrp_ibm|powerpc/cell)
- ;;
+ ;;
ppc64/chrp|ppc64/chrp_rs6k|ppc64/chrp_ibm|ppc64/cell)
- ;;
- powerpc/*|ppc64/*)
- offs=$(findfs /boot/grub)
- [ -n "$offs" ] || error "GRUB requires that the OF partition is
mounted in /boot/grub" 1>&2
- ;;
+ ;;
```
...I only changed this to match the style of the other code.
Please adjust your editor settings if necessary. Your patches should
only contain actual functional changes.
I'll keep that in mind. :-) How are style "issues" solved then?
[...]
Ok, we're basically with the two issues you already mentioned, i.e. not
being able to write
to NVRAM and not being able to find the proper install partition.
Can you revise your patches above, then we'll look into these two issues
after I have built
new test installation images.
Sure, but could you at least already integrate the new debconf templates
and build a new image, so I can fix the partition selection? I don't yet
know of another way to make the debconf templates available to the code.
Cheers,
Frank
