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
signature.asc
Description: OpenPGP digital signature
