Hi!
On 10/19/2017 02:17 PM, Frank Scheiner wrote:
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.
Just keep in mind that the other partitioning scheme already does the right
thing and
is consistently used across the rest of the code. Whenever you introduce a
change that
is not consistent with the rest of the code, you should always ask yourself why
you
are doing this and if it's actually necessary.
Always try to keep your changes minimal-invasive and consistent.
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. :-)
Ok, I will look into the template changes and add them for test packages.
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 is for code readability, consistency and robustness. You are changing
production
code here which is deployed on potentially hundreds of thousands of computers
(Ubuntu
uses grub-installer as well!). Hence, you always try to make your change
consistent
with the rest of the code, as robust as possible and as readable as possible.
It's a different story when you make such changes in your personal repository.
This hunk is completely unnecessary and is purely formatting changes
(tabs vs. spaces, most likely):
Is `grub-installer` actually using spaces for identation?
No, of course not. It's a shell script, tabs and spaces don't make a difference.
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.
Yes, I missed that part. Would have made more sense if you had split that into
a separate
patch though. Changes should be separated from each other functionally. In this
case, you
fixed the issue that grub-installer is prompting the
"grub-installer/force-efi-extra-removable"
question and that should definitely be a separate patch.
As you can see, it's very important to make changes easily to understand and
parse.
- # 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.
Yes, but please don't throw unrelated changes into the same patch. This is
confusing
people who review your patches.
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.
Yes, I will add the templates later today after reviewing them. The new images
will be
available tomorrow.
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer - glaub...@debian.org
`. `' Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
