Re: [patch]Re: debootstrap on Debian GNU/kFreeBSD + questions
On Mon, Mar 16, 2009 at 12:17:03PM +0100, Luca Favatella wrote:
> Index: debootstrap
> ===================================================================
> --- debootstrap (revision 57816)
> +++ debootstrap (working copy)
> @@ -9,6 +9,9 @@
> if [ "$DEBOOTSTRAP_DIR" = "" ]; then
> if [ -x /debootstrap/debootstrap ]; then
> DEBOOTSTRAP_DIR=/debootstrap
> + elif [ -x $(pwd)/debootstrap ]; then
> + echo "Warning: Make sure that 'fakeroot make devices.tar.gz' was done"
> + DEBOOTSTRAP_DIR=$(pwd)
> else
> DEBOOTSTRAP_DIR=/usr/share/debootstrap
> fi
> @@ -93,6 +96,8 @@
> (requires --second-stage)
> --boot-floppies used for internal purposes by boot-floppies
> --debian-installer used for internal purposes by debian-installer
> +
> + --extra-* used by porters, see the manpage
> EOF
> }
>
Options should be documented individually in --help rather than
referring to the manual page. --help should describe what the options do
rather than saying who they're used by.
> @@ -283,6 +288,26 @@
> error 1 NEEDARG "option requires an argument %s" "$1"
> fi
> ;;
> + --extra-mirror)
> + if [ -n "$2" ] ; then
> + EXTRA_MIRROR="$2"
> + shift 2
> + else
> + error 1 NEEDARG "option requires an argument %s" "$1"
> + fi
> + ;;
I think this is a reasonable generic facility to have, although I
question the interface. See below.
> + --extra-suite)
> + if [ -n "$2" ] ; then
> + EXTRA_SUITE="$2"
> + shift 2
> + else
> + error 1 NEEDARG "option requires an argument %s" "$1"
> + fi
> + ;;
What happens if you have multiple extra mirrors and suites? The
interface in your patch is very confusing to me.
Perhaps we should have something like --overlay=MIRROR[:SUITE], so that
the fact that the mirror and suite are bundled up into one unit and need
to be specified together is obvious, and to make it easier to understand
how one might go about adding multiple overlays?
> + --extra-include*)
> + extra_debs="$(echo $1 | cut -f2 -d"="|tr , " ")"
> + shift 1
> + ;;
I don't like this. Why not just try to fetch all packages successively
from all available mirror/suite combinations, and use the newest version
of each in cases of duplication? Priority fields in the overlay
repository should be used to control what debootstrap uses by default,
just as they are for the main repository. For custom additions, we have
--include.
I don't think this will be easy to do right; but the result will be a
much cleaner interface. It's important to design clean interfaces as
they're very hard to change later.
> +if [ "$extra_debs" != "" -a "$EXTRA_SUITE" = "" ]; then
> + EXTRA_SUITE="unreleased"
> +fi
[ ... ] && [ ... ] is better style than [ ... -a ... ], although
debootstrap is not entirely consistent about this.
The default suite on each overlay mirror should be the same as the suite
originally specified to debootstrap, I think. I can't see anything else
as a reasonable default.
> +if [ "$EXTRA_MIRROR" = "" ]; then
> + EXTRA_MIRROR="$MIRRORS"
> +fi
Use [ -z ... ].
> @@ -445,7 +478,12 @@
> if am_doing_phase finddebs; then
> if [ "$FINDDEBS_NEEDS_INDICES" = "true" ] || \
> [ "$RESOLVE_DEPS" = "true" ]; then
> - download_indices
> + download_indices "$SUITE" "$MIRRORS"
> + # The test might be skipped if download_indices() can detect that
> + # it didn't receive two parameters
> + if [ "$extra_debs" != "" ]; then
> + download_indices "$EXTRA_SUITE" "$EXTRA_MIRROR"
> + fi
> GOT_INDICES=true
> fi
We should always download all available indices, rather than this being
conditional on using extra packages (which should not need to be
specified up-front). See above.
I think the iteration over multiple mirror/suite pairs should be pushed
down to download_indices rather than being done in the top-level script.
> +# Could be more fine-grained by handling $extra_debs until the end.
> +# Probably not needed, so merging $required and $extra_debs
> +required="$required $extra_debs"
The semantics here are messy (what if you wanted them to be installed in
debootstrap's second stage instead?) and are another indication that
--extra-include should be discarded.
> @@ -542,12 +591,16 @@
> rm -f "$TARGET/etc/apt/sources.list"
> fi
> if [ "${MIRRORS#http://}"; != "$MIRRORS" ]; then
> - setup_apt_sources "${MIRRORS%% *}"
> - mv_invalid_to "${MIRRORS%% *}"
> + setup_apt_sources $SUITE "${MIRRORS%% *}"
> + mv_invalid_to $SUITE "${MIRRORS%% *}"
> else
> - setup_apt_sources "$DEF_MIRROR"
> - mv_invalid_to "$DEF_MIRROR"
> + setup_apt_sources $SUITE "$DEF_MIRROR"
> + mv_invalid_to $SUITE "$DEF_MIRROR"
> fi
> + if [ "$EXTRA_SUITE" != "" ]; then
> + setup_apt_sources $EXTRA_SUITE $EXTRA_MIRROR
> + mv_invalid_to $EXTRA_SUITE $EXTRA_MIRROR
> + fi
>
> if [ -e "$TARGET/debootstrap/debootstrap.log" ]; then
> if [ "$KEEP_DEBOOTSTRAP_DIR" = true ]; then
Again, this should remember that it's iterating over mirror/suite pairs.
> Index: functions
> ===================================================================
> --- functions (revision 57816)
> +++ functions (working copy)
> @@ -374,12 +374,21 @@
>
> download () {
> mk_download_dirs
> - "$DOWNLOAD_DEBS" $(echo "$@" | tr ' ' '\n' | sort)
> + local suite="$1"
> + local mirrors="$2"
> + shift 2
> + "$DOWNLOAD_DEBS" "$suite" "$mirrors" $(echo "$@" | tr ' ' '\n' | sort)
> }
>
> download_indices () {
> mk_download_dirs
> - "$DOWNLOAD_INDICES" $(echo "$@" | tr ' ' '\n' | sort)
> + local suite="$1"
> + local mirrors="$2"
> + shift 2
> + # It is never called with any additional parameters, why not just
> + # getting rid of the echo/tr/sort stuff? It might have been a
> + # copy/paste from the other function wrapper.
> + "$DOWNLOAD_INDICES" "$suite" "$mirrors" $(echo "$@" | tr ' ' '\n' | sort)
> }
>
> debfor () {
Neither of these appears to take account of the fact that each mirror
you specify might have different layouts (see mirror_style).
> + # Avoid component duplication (which is harmless anyway, components
> + # just appear several times in sources.list in this case) when
> + # download_release_indices() is called more than once.
This seems like a hack. Shouldn't components be calculated independently
for each mirror/suite pair?
> (cd "$TARGET/$APTSTATE/lists"
> - for a in debootstrap.invalid_*; do
> + for a in debootstrap.invalid_dists_${suite}_*; do
> mv "$a" "${m}_${a#*_}"
> done
> )
Your editor seems to have converted a tab to spaces here. Please try to
avoid this.
> setup_apt_sources () {
> + local suite="$1"
> + shift 1
Just 'shift' will do.
> Index: debootstrap.8
> ===================================================================
> --- debootstrap.8 (revision 57816)
> +++ debootstrap.8 (working copy)
> @@ -135,6 +135,30 @@
> .IP "\fB\-\-debian\-installer\fP"
> Used for internal purposes by the debian-installer
> .IP
> +.SH "PORTER OPTIONS"
> +.
> +.PP
> +The following options should be useful only to porters whose arch has
> +not yet been integrating into the official archive, and who need to
> +download additional packages from a suite called \fIunreleased\fR or
> +similar.
I can think of other cases where this is useful. Let the user worry
about who they are - just document the options you're adding together
with the existing options.
> +Note that all dependencies have to be solved manually: the extra
> +included packages should be autosufficient (in \fIEXTRA_SUITE\fR);
> +and their dependencies in \fISUITE\fR have to be added using
> +\fB\-\-include\fP. A helper script is available in debootstrap's
> +sources, see \fIscripts/porters/\fR).
I think we should fix this bug rather than enshrining it in
documentation. :-)
Thanks,
--
Colin Watson [cjwatson@debian.org]
Reply to: