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