Re: [gentoo-catalyst] [PATCH] make bindist optional

[Matt Turner]

On Fri, Dec 7, 2012 at 1:35 PM, Rick "Zero_Chaos" Farina
<zerochaos@g.o> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
>
> From: "Rick Farina (Zero_Chaos)" <zerochaos@g.o>
>
> After the recent fixes which ensure the bindist use flag
> is always set, users now have no way to disable this flag.
> This patch introduces the new "bindist" feature, enabled by
> default, which will allow users to turn off bindist if
> they are not going to redistribute the builds (or for
> tinderbox testing, etc).
> - ---
>  catalyst                        |    7 +++++++
>  files/catalyst.conf             |    4 +++-
>  modules/generic_stage_target.py |    5 +++--
>  modules/grp_target.py           |   11 ++++++-----
>  modules/livecd_stage1_target.py |    6 ++++--
>  targets/stage1/stage1-chroot.sh |   10 +++++++---
>  6 files changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/catalyst b/catalyst
> index 3d31599..8485984 100755
> - --- a/catalyst
> +++ b/catalyst
> @@ -112,6 +112,13 @@ def parse_config(myconfig):
>                 print "Autoresuming support enabled."
>                 conf_values["AUTORESUME"]="1"
>
> +       if "bindist" in string.split(conf_values["options"]):
> +               print "Binary redistribution enabled"
> +               conf_values["BINDIST"]="1"
> +       else:
> +               print "Bindist is not enabled in catalyst.conf"
> +               print "Binary redistribution of generated stages/isos is prohibited
> by law."
> +
>         if "ccache" in string.split(conf_values["options"]):
>                 print "Compiler cache support enabled."
>                 conf_values["CCACHE"]="1"
> diff --git a/files/catalyst.conf b/files/catalyst.conf
> index ea74eb1..18c6ab8 100644
> - --- a/files/catalyst.conf
> +++ b/files/catalyst.conf
> @@ -49,6 +49,8 @@ hash_function="crc32"
>  #      the -a option to the catalyst cmdline.  -p will clear the autoresume
> flags
>  #      as well as your pkgcache and kerncache
>  #      ( This option is not fully tested, bug reports welcome )
> +# bindist = enable this option when the builds will be publicly
> redistributed to prevent
> +       building of patent encumbered and redistribution restricted items.
>  # ccache = enables build time ccache support (highly recommended)
>  # distcc = enable distcc support for building. You have to set
> distcc_hosts in
>  #      your spec file.
> @@ -64,7 +66,7 @@ hash_function="crc32"
>  #      your cache. The cache is unlinked before any empty or rm processing,
> though.
>  #
>  # (These options can be used together)
> - -options="autoresume kerncache pkgcache seedcache snapcache"
> +options="autoresume bindist kerncache pkgcache seedcache snapcache"
>
>  # portdir specifies the source portage tree used by the snapshot target.
>  portdir="/usr/portage"
> diff --git a/modules/generic_stage_target.py
> b/modules/generic_stage_target.py
> index 3597680..89082c5 100644
> - --- a/modules/generic_stage_target.py
> +++ b/modules/generic_stage_target.py
> @@ -490,8 +490,9 @@ class generic_stage_target(generic_target):
>                 if type(self.settings["use"])==types.StringType:
>                         self.settings["use"]=self.settings["use"].split()
>
> - -             # Force bindist for all targets
> - -             self.settings["use"].append("bindist")
> +               # Force bindist when options ask for it
> +               if self.settings.has_key("BINDIST"):
> +                       self.settings["use"].append("bindist")
>
>         def set_stage_path(self):
>                 self.settings["stage_path"]=normpath(self.settings["chroot_path"])
> diff --git a/modules/grp_target.py b/modules/grp_target.py
> index 12eab08..5d3af2e 100644
> - --- a/modules/grp_target.py
> +++ b/modules/grp_target.py
> @@ -62,11 +62,12 @@ class grp_target(generic_stage_target):
>                                 raise CatalystError,"GRP build aborting due to error."
>
>         def set_use(self):
> - -         generic_stage_target.set_use(self)
> - -         if self.settings.has_key("use"):
> - -             self.settings["use"].append("bindist")
> - -         else:
> - -             self.settings["use"]=["bindist"]
> +               generic_stage_target.set_use(self)
> +               if self.settings.has_key("BINDIST"):
> +                       if self.settings.has_key("use"):
> +                               self.settings["use"].append("bindist")
> +                       else:
> +                               self.settings["use"]=["bindist"]
>
>         def set_mounts(self):
>             self.mounts.append("/tmp/grp")
> diff --git a/modules/livecd_stage1_target.py
> b/modules/livecd_stage1_target.py
> index 17254bb..a27cfe4 100644
> - --- a/modules/livecd_stage1_target.py
> +++ b/modules/livecd_stage1_target.py
> @@ -48,10 +48,12 @@ class livecd_stage1_target(generic_stage_target):
>                 generic_stage_target.set_use(self)
>                 if self.settings.has_key("use"):
>                         self.settings["use"].append("livecd")
> - -                     self.settings["use"].append("bindist")
> +                       if self.settings.has_key("BINDIST"):
> +                               self.settings["use"].append("bindist")
>                 else:
>                         self.settings["use"]=["livecd"]
> - -                     self.settings["use"].append("bindist")
> +                       if self.settings.has_key("BINDIST"):
> +                               self.settings["use"].append("bindist")

This if has_key -> append otherwise create and append pattern is ugly.
I wonder if there's a cleaner way to do this. I'll see what I can
find.

In any case, you can factor the bindist part out of the if has_key block.

>         def set_packages(self):
>                 generic_stage_target.set_packages(self)
> diff --git a/targets/stage1/stage1-chroot.sh
> b/targets/stage1/stage1-chroot.sh
> index e40982b..8e5a492 100644
> - --- a/targets/stage1/stage1-chroot.sh
> +++ b/targets/stage1/stage1-chroot.sh
> @@ -6,7 +6,11 @@ source /tmp/chroot-functions.sh
>  export clst_buildpkgs="$(/tmp/build.py)"
>
>  # Setup our environment
> - -BOOTSTRAP_USE="$(portageq envvar BOOTSTRAP_USE)"
> +if [ -n "${clst_BINDIST}" ]; then
> +       BOOTSTRAP_USE="$(portageq envvar BOOTSTRAP_USE) bindist"
> +else
> +       BOOTSTRAP_USE="$(portageq envvar BOOTSTRAP_USE)"
> +fi

I would change this to just

BOOTSTRAP_USE="$(portageq envvar BOOTSTRAP_USE)"
if [ -n "${clst_BINDIST}" ]; then
        BOOTSTRAP_USE="$BOOTSTRAP_USE bindist"
fi

>  FEATURES="${clst_myfeatures} nodoc noman noinfo -news"
>
>  ## Sanity check profile
> @@ -50,8 +54,8 @@ sed -i '/USE="${USE} -build"/d' /etc/portage/make.conf
>
>  # Now, we install our packages
>  [ -e /etc/portage/make.conf ] && \
> - -     echo "USE=\"-* bindist build ${BOOTSTRAP_USE} ${clst_HOSTUSE}\"" \
> +       echo "USE=\"-* build ${BOOTSTRAP_USE} ${clst_HOSTUSE}\"" \
>         >> /etc/portage/make.conf
>  run_merge "--oneshot ${clst_buildpkgs}"
> - -sed -i "/USE=\"-* bindist build ${BOOTSTRAP_USE} ${clst_HOSTUSE}\"/d" \
> +sed -i "/USE=\"-* build ${BOOTSTRAP_USE} ${clst_HOSTUSE}\"/d" \
>         /etc/portage/make.conf
> - --
> 1.7.8.6

Overall, the patch looks good. With the couple of little style changes
and the wording change suggested by Mike & Jorge, this is Reviewed-by:
Matt Turner <mattst88@×××××.com>