Re: [gentoo-catalyst] [PATCH 1/5] Some options cleanup, unifying their use, reducing redundancy.

[W. Trevor King]

On Wed, Sep 10, 2014 at 08:43:40PM -0700, Brian Dolbec wrote:
> -valid_config_file_values.extend(["PKGCACHE", "KERNCACHE", "CCACHE", "DISTCC",
> -	"ICECREAM", "ENVSCRIPT", "AUTORESUME", "FETCH", "CLEAR_AUTORESUME",
> -	"options", "DEBUG", "VERBOSE", "PURGE", "PURGEONLY", "SNAPCACHE",
> -	"snapshot_cache", "hash_function", "digests", "contents", "SEEDCACHE"
> +valid_config_file_values.extend([ "distcc", "envscript",
> +	"options", "DEBUG", "VERBOSE",
> +	"snapshot_cache", "hash_function", "digests", "contents"

I think you meant to drop 'distcc' from valid_config_file_values,
since it's just a boolean, and you do have it in the new
option_messages:

> +# legend:  key: message
> +option_messages = {
> +	"autoresume": "Autoresuming support enabled.",
> +	"ccache": "Compiler cache support enabled.",
> +	"clear-autoresume": "Cleaning autoresume flags support enabled.",
> +	#"compress": "Compression enabled.",
> +	"distcc": "Distcc support enabled.",
> +	"icecream": "Icecream compiler cluster support enabled.",
> +	"kerncache": "Kernel cache support enabled.",
> +	"pkgcache": "Package cache support enabled.",
> +	"purge": "Purge support enabled.",
> +	"seedcache": "Seed cache support enabled.",
> +	"snapcache": "Snapshot cache support enabled.",
> +	#"tarball": "Tarball creation enabled.",
> +	}

I'd also remove the tarball entry, since we haven't used TARBALL since
9be28ed1 (Fixed up action_sequence when using --fetchonly to not
create tarballs or IS O images for bug #143392, 2006-11-22).

> -	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 may be prohibited by law."
> -		print "Please see the use description for bindist on any package you are including."

Hmm, I think you need a 'bindist' entry in option_messages.  And I'd
probably tack on the legal warning to the enabled message.

> +	#print "MAIN: cli options =", options

You don't need to add this ;).

> +	conf_values["options"].update(options)
> +	#print "MAIN: conf_values['options'] =", conf_values["options"]

Or this #print.

> @@ -501,8 +501,8 @@ class generic_stage_target(generic_target):
>  				"base_dirs","bind","chroot_setup","setup_environment",\
>  				"run_local","preclean","unbind","clean"]
>  #		if "TARBALL" in self.settings or \
> -#			"FETCH" not in self.settings:
> -		if "FETCH" not in self.settings:
> +#			"fetch" not in self.settings["options"]:
> +		if "fetch" not in self.settings["options"]:
>  			self.settings["action_sequence"].append("capture")
>  		self.settings["action_sequence"].append("clear_autoresume")

I'd just remove the whole comment, since we don't use TARBALL (see my
earlier comment).
  
> @@ -1285,7 +1292,14 @@ class generic_stage_target(generic_target):
>  		fixed. We need this to use the os.system() call since we can't
>  		specify our own environ
>  		"""
> -		for x in self.settings.keys():
> +		#print "setup_environment(); settings =", list(self.settings)
> +		for x in list(self.settings):
> +			#print "setup_environment(); processing:", x
> +			if x == "options":
> +				#self.env['clst_' + x] = ' '.join(self.settings[x])
> +				for opt in self.settings[x]:
> +					self.env['clst_' + opt.upper()] = "true"
> +				continue
>  			""" Sanitize var names by doing "s|/-.|_|g" """
>  			varname="clst_"+string.replace(x,"/","_")
>  			varname=string.replace(varname,"-","_")

No need to add the commented lines.  I don't understand the switch
from self.settings.keys() to list(self.settings) either, is that just
churn?

>  	def build_kernel(self):
> -		"Build all configured kernels"
> +		'''Build all configured kernels'''

This seems like unnecessary churn ;).

>  #		if "TARBALL" in self.settings or \
> -#			"FETCH" not in self.settings:
> -		if "FETCH" not in self.settings:
> +#			"fetch" not in self.settings['options']:
> +		if "fetch" not in self.settings['options']:
>  			self.settings["action_sequence"].append("capture")
>  		self.settings["action_sequence"].append("clear_autoresume")

Goodbye comment (no more TARBALL).

I haven't tested this one yet (and it would be hard to do thoroughly
with so many options on the line), but it looks good other than the
things I mentioned ;).  I'm very positive on the options set for
boolean configs.

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy