[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: [PATCH 1/5] Install hfsutils in-target for NewWorld Power Macs (adapted from d-i/yaboot-installer).



On 11/06/2017 12:13 PM, Frank Scheiner wrote:
> The installation of the hfsutils package is located in debian/postinst
> similar to d-i/yaboot-installer, from which the code was adapted. But
> d-i/grub-installer does all in-target installations from the
> `grub-installer` script itself. If this is important this can still be
> moved to `grub-installer`.

It should be moved to grub-installer for consistency reasons. Also, you
don't need to mention this code was taken from the yaboot-installer
script.

The commit message should be simply:

"Install hfsutils in-target for powerpc/ppc64 neworld targets"

> 
> ---
>  debian/changelog                |  5 ++++-
>  debian/grub-installer.templates | 12 ++++++++++++
>  debian/postinst                 | 21 +++++++++++++++++++++
>  3 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/debian/changelog b/debian/changelog
> index 0470484..0f1dc54 100644
> --- a/debian/changelog
> +++ b/debian/changelog
> @@ -2,8 +2,11 @@ grub-installer (1.147) UNRELEASED; urgency=medium
>  
>    [ Frank Scheiner ]
>    * Execute EFI related code only for installations of grub-efi* packages.
> +  * Make d-i/grub-installer work for NewWorld Power Macs:

This additional changelog entry is not necessary. It's enough to
document the individual changes.

> +    * Install hfsutils in-target for NewWorld Power Macs (adapted from
> +      d-i/yaboot-installer).
>  
> - -- Frank Scheiner <frank.scheiner@web.de>  Thu, 31 Oct 2017 20:08:00 +0200
> + -- Frank Scheiner <frank.scheiner@web.de>  Thu, 06 Nov 2017 08:42:00 +0200

How did you create the second changelog entry? Normally you should just
run "dch" which should not alter the timestamp at the bottom. Please
recreate the changelog entry with just "dch" so the timestamp doesn't
change.

>  grub-installer (1.146) unstable; urgency=medium
>  
> diff --git a/debian/grub-installer.templates b/debian/grub-installer.templates
> index e294afb..5f82c04 100644
> --- a/debian/grub-installer.templates
> +++ b/debian/grub-installer.templates
> @@ -1,3 +1,15 @@
> +Template: grub-installer/apt_install_hfsutils
> +Type: text
> +# :sl2:
> +_Description: Installing hfsutils
> +
> +Template: grub-installer/apt_install_hfsutils_failed
> +Type: error
> +# :sl2:
> +_Description: hfsutils installation failed
> + The hfsutils package failed to install into /target. Without it the NewWorld
> + boot partition cannot be formatted.
> +
>  Template: grub-installer/with_other_os
>  Type: boolean
>  Default: true
> diff --git a/debian/postinst b/debian/postinst
> index 4b12027..cb18277 100755
> --- a/debian/postinst
> +++ b/debian/postinst
> @@ -31,6 +31,27 @@ mountvirtfs () {
>  	fi
>  }

This is ok.

> +ARCH="$(archdetect)"
> +

This should be removed.

> +case $ARCH in
> +    powerpc/powermac_newworld|ppc64/powermac_newworld)
> +	if ! apt-install hfsutils; then
> +		info "Calling 'apt-install hfsutils' failed"
> +		# Hm, unable to install hfsutils into /target/, what should we do?
> +		db_input critical grub-installer/apt_install_hfsutils_failed || [ $? -eq 30 ]
> +		if ! db_go; then
> +			db_progress STOP
> +			exit 10 # back up to menu
> +		fi
> +		db_get grub-installer/apt_install_hfsutils_failed
> +		if [ "$RET" != true ]; then
> +			db_progress STOP
> +			exit 1
> +		fi
> +	fi
> +	;;
> +esac
> +

This should be moved to the grub-installer script at the appropriate
position. I would suggest placing it into a new case statement after
the case statement to wipe the prep partition which starts around line
400. So, please put it after that one and, again, match with
$ARCH:$grub_package instead of just $ARCH.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913


Reply to: