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

[Brian Dolbec <dolsen@gentoo.org>]

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

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:
> > +target_mounts = {
> > +	"proc": "/proc",
> > + …
> > +	"port_logdir": "/var/log/portage",
> > +	}
> > …
> > @@ -173,11 +188,12 @@ class generic_stage_target(generic_target):
> >  			file_locate(self.settings,["portage_confdir"],expand=0)
> >  
> >  		""" Setup our mount points """
> > +		self.target_mounts = target_mounts.copy()
> >  		if "SNAPCACHE" in self.settings:
> >  			self.mounts=["proc", "dev", "portdir", "distdir", "port_tmpdir"]
> >  			self.mountmap={"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",
> > -				"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.


> It also looks like there is some leading
> non-tab whitespace at the beginning of your new distdir line, which
> was otherwise unchanged.


Ah, yes, an errant space got in at the beginning of the line.  I have
rebased it away.  I will repost the fixed patch when any other dust
settles.


> 
> > -			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 :/

>  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.

code snipit from defaults.py to come later.

# please try to avoid using "-", "/", "." in key names
# due to them not being compatible in the bash environment
confdefaults={
	"storedir": "/var/tmp/catalyst",
	"sharedir": "/usr/lib/catalyst",
	"shdir": "%(sharedir)s/targets",
	"PythonDir": "./catalyst",
	"archdir": "%(PythonDir)s/arch",
	"distdir": "/usr/portage/distfiles",
	"repo_name": "portage",
	"portdir": "/usr/portage",
	"packagedir": "/usr/portage/packages",
	"port_tmpdir": "/var/tmp/portage",
	"local_overlay": "/usr/local/portage",
	"port_conf": "/etc/portage",
	"make_conf": "%(port_conf)s/make.conf",
	"options": set(),
	"snapshot_name": "portage-",
	"snapshot_cache": "/var/tmp/catalyst/snapshot_cache",
	"hash_function": "crc32",
	}


target_mounts = {
	"proc": "/proc",
	"dev": "/dev",
	"pts": "/dev/pts",
	"portdir": "/usr/portage",
	"distdir": "/usr/portage/distfiles",
	"packagedir": "/usr/portage/packages",
	"port_tmpdir": "/var/tmp/portage",
	"kerncache": "/tmp/kerncache",
	"ccache": "/var/tmp/ccache",
	"icecream": "/var/cache/icecream",
	"port_logdir": "/var/log/portage",
	}



> 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.)

> 
> >  	def bind(self):
> >  		for x in self.mounts:
> > -			if not os.path.exists(self.settings["chroot_path"] + self.mountmap[x]):
> > -				os.makedirs(self.settings["chroot_path"]+x,0755)
> > +			#print "bind(); x =", x
> > +			target = normpath(self.settings["chroot_path"] + self.target_mounts[x])
> > +			if not os.path.exists(target):
> > +				os.makedirs(target, 0755)
> >  
> >  			if not os.path.exists(self.mountmap[x]):
> >  				if not self.mountmap[x] == "tmpfs":
> > -					os.makedirs(self.mountmap[x],0755)
> > +					os.makedirs(self.mountmap[x], 0755)
> >  
> >  			src=self.mountmap[x]
> 
> Might as well shift this src declaration up and use it instead of
> self.mountmap[x] earlier.  Not exactly required for this commit, but
> you are defining target, so it's not too much of a stretch ;).

Yeah, I did target when trying to get all the path issues straightened
out.  I never really looked at src to be honest.  That code was not
causing me grief.  So, yeah, do it later in a separate commit.

>  
> > -			if "SNAPCACHE" in self.settings and x == "/usr/portage":
> > +			#print "bind(); src =", src
> > +			if "SNAPCACHE" in self.settings and x == self.settings["portdir"]:
> 
> Oops.  x is the mountmap key, so I think this should be 'portdir'.

Good catch, I missed this one when converting from paths as keys.

> 
> > +					#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.

> I'll be happy once we're using subprocess :p.
> 

I have a number of improvements coming to the cmd().  but yeah, it'll
need more.

> A few typos, but I think this makes mount handling much more sane :).
> 
> Cheers,
> Trevor
> 

-- 
Brian Dolbec <dolsen@gentoo.org>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 620 bytes --]