Re: [gentoo-catalyst] [PATCH 2/2] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[W. Trevor King]

On Tue, Dec 17, 2013 at 09:18:04PM -0800, Brian Dolbec wrote:
> On Tue, 2013-12-17 at 19:28 -0800, W. Trevor King wrote:
> > On Tue, Dec 17, 2013 at 05:07:27PM -0800, Brian Dolbec wrote:
> > > -				"portdir":self.settings["snapshot_cache_path"]+"/portage",\
> > > -				"distdir":self.settings["distdir"],"port_tmpdir":"tmpfs"}
> > > +				"portdir":normpath(self.settings["snapshot_cache_path"]+"/" + self.settings["repo_name"]),
> > > + 				"distdir":self.settings["distdir"],"port_tmpdir":"tmpfs"}
> >
> > Can we do this for the mountmap defaults too, and just override the
> > mountmap special cases in the SNAPCACHE branch?  I think this would be
> > a good commit for that (or we can do it in a commit after this).
> >
> > I don't think the 'portage' → self.settings["repo_name"] replacement
> > should go in this commit.
>
> repo_name variable is already defined. This one got missed in the
> change.  You want it in the next commit by itself?  Can do.

That's what I want ;).  Just to avoid making this one more complicated
than it needs to be.

> > > -			self.mounts.append("/var/log/portage")
> > > -			self.mountmap["/var/log/portage"]=self.settings["port_logdir"]
> > > -			self.env["PORT_LOGDIR"]="/var/log/portage"
> > > +			self.mounts.append("port_logdir")
> > > +			self.mountmap["port_logdir"]=self.settings["port_logdir"]
> > > +			self.env["PORT_LOGDIR"]=self.settings["port_logdir"]
> >
> > I don't know where we stand on 'x=y' vs 'x = y', but I'd prefer the
> > latter here.
>
> I do too, but I thought it was agreed to fix all that separately
> later :/

Matt had me work some ' = ' spacing into the last series, but later
works for me.

> >  I also think that the PORT_LOGDIR environment variable
> > should be:
> >
> >   self.env["PORT_LOGDIR"] = self.target_mounts["port_logdir"]
>
> meh, but I also prefer some uniformity.  I prefer all-caps for
> constants.  In this case, the end result later in a new defaults.py
> file...  since it is internal, why mix case in keys.  They are also
> pushed into the bash environment for the chroot scripts to use.  So,
> again, uniformity can be your friend when making changes.
> Especially with a large complex app like catalyst.

I don't care about caps, I was trying to fix settings → target_mounts,
since settings holds the mount source ;).

> > Nothing in the chroot should care where the source comes from ;).
> > I'd considering renaming mountmap → source_mounts for clarity,
> > using existing settings to override source_mounts at
> > initialization, and using source_mounts thereafter.
>
> mounts and mountmap were existing variable names.  With some of the
> flack I've gotten over my choice of variable name already, now you
> want me to go through even more???  Do it in a separate commit.
> (after the rewrite patches are done for.)

Fair enough ;).

> > > +					#print "bind(); cmd =", cmd
> >
> > If it's not useful enough to print, I don't think we should commit it
> > ;).
>
> There are various of these added to the code.  They are EXTREMELY
> helpful and even necessary to debug major code changes.  Of which the
> mounts, mountmap changes were one of the toughest to debug.
>
> Both you and I have already stated that we want to convert to using
> python's logging.  These are just a first step towards that goal.  All
> that is needed is to search out the #print's and replace them with teh
> proper logging statement.

Ok.

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