Re: [gentoo-catalyst] [PATCH 1/2] modules/generic_stage_target.py, Create SOURCE_MOUNTS_DEFAULTS

["W. Trevor King" <wking@tremily.us>]

[-- Attachment #1: Type: text/plain, Size: 2972 bytes --]

On Thu, Jan 02, 2014 at 10:52:00PM -0800, Brian Dolbec wrote:
> On Thu, 2014-01-02 at 22:20 -0800, W. Trevor King wrote:
> > On Thu, Jan 02, 2014 at 10:00:43PM -0800, Brian Dolbec wrote:
> > > +SOURCE_MOUNTS_DEFAULTS = {
> > > …
> > > +	"distdir": "/usr/portage/distfiles",
> > > +	"portdir": "/usr/portage",
> > > …
> > > -				"distdir": self.settings["distdir"],
> > > -				"portdir": normpath("/".join([
> > > -					self.settings["snapshot_cache_path"],
> > > -					self.settings["repo_name"],
> > > -					])),
> > > …
> > > +		# initialize our source mounts
> > > +		self.mountmap = SOURCE_MOUNTS_DEFAULTS.copy()
> > > +		# update them from settings
> > > +		self.mountmap["distdir"] = self.settings["distdir"]
> > > +		self.mountmap["portdir"] = normpath("/".join([
> > > +			self.settings["snapshot_cache_path"],
> > > +			self.settings["repo_name"],
> > > +			]))
> > 
> > Why create dummy initial values and then blow them away?  Wouldn't:
> > 
> >   SOURCE_MOUNTS_DEFAULTS = {
> >     …
> >     'distdir': None,  # initialized from settings
> >     'portdir': None,  # initialized from settings
> >     }
> > 
> > make more sense?  We'll blow away this default dict once we're loading
> > it via ConfigParser, so this doesn't have to be super elegant, but
> > adding values just to clobber them seems misleading.
> 
> We probably could, they are defaulted in config_defaults.  But at the
> time I was just thinking that catalyst.conf or a spec file may not have
> the value declared, so was better to give it a default here, just in
> case.

I'm fine with that, but then we should either use this to setup
settings' defaults, or check to make sure the key is in settings
before clobbering the version from SOURCE_MOUNTS_DEFAULTS.

> > > -		if "SNAPCACHE" in self.settings:
> > > -			self.mounts = ["proc", "dev", "portdir", "distdir", "port_tmpdir"]
> > > …
> > > -		else:
> > > -			self.mounts = ["proc", "dev", "distdir", "port_tmpdir"]
> > > …
> > > +		self.mounts = ["proc", "dev", "portdir", "distdir", "port_tmpdir"]
> > > …
> > > +		if "SNAPCACHE" not in self.settings:
> > > +			self.mounts.remove("portdir")
> > 
> > I'd prefer:
> > 
> >   self.mounts = ["proc", "dev", "distdir", "port_tmpdir"]
> >   if "SNAPCACHE" in self.settings:
> >       self.mounts.append("portdir")
> 
> I didn't do that because I didn't know if the mount order was important,
> so erred on the side of the current order.

Ah, good point.  Mounting portdir after distdir would probably not
work well ;).  I now think the preloaded mounts is a good idea, and
that we should fill in the missing keys here too for consistency
('devpts', 'ccache', 'icecream', …).  Then remove entries as
appropriate for the OS/settings.

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]