From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lists.gentoo.org (pigeon.gentoo.org [208.92.234.80]) by finch.gentoo.org (Postfix) with ESMTP id 0B660138247 for ; Wed, 18 Dec 2013 05:18:22 +0000 (UTC) Received: from pigeon.gentoo.org (localhost [127.0.0.1]) by pigeon.gentoo.org (Postfix) with SMTP id E554EE0ADC; Wed, 18 Dec 2013 05:18:20 +0000 (UTC) Received: from smtp.gentoo.org (smtp.gentoo.org [140.211.166.183]) (using TLSv1 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by pigeon.gentoo.org (Postfix) with ESMTPS id 5D459E0ADC for ; Wed, 18 Dec 2013 05:18:20 +0000 (UTC) Received: from [192.168.1.210] (S010600222de111ff.vc.shawcable.net [96.49.5.156]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) (Authenticated sender: dolsen) by smtp.gentoo.org (Postfix) with ESMTPSA id 8704E33F0E3 for ; Wed, 18 Dec 2013 05:18:19 +0000 (UTC) Message-ID: <1387343884.3897.204.camel@big_daddy.dol-sen.ca> Subject: Re: [gentoo-catalyst] [PATCH 2/2] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary From: Brian Dolbec To: gentoo-catalyst@lists.gentoo.org Date: Tue, 17 Dec 2013 21:18:04 -0800 In-Reply-To: <20131218032857.GD25409@odin.tremily.us> References: <1387328847-25840-1-git-send-email-dolsen@gentoo.org> <1387328847-25840-3-git-send-email-dolsen@gentoo.org> <20131218032857.GD25409@odin.tremily.us> Content-Type: multipart/signed; micalg="pgp-sha512"; protocol="application/pgp-signature"; boundary="=-j9HmL2j6bPVGp1vb6PwZ" X-Mailer: Evolution 3.6.4 Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-Id: Gentoo Linux mail X-BeenThere: gentoo-catalyst@lists.gentoo.org Reply-to: gentoo-catalyst@lists.gentoo.org Mime-Version: 1.0 X-Archives-Salt: 5428cf5b-5d28-4b5c-8b4a-009c17f422e7 X-Archives-Hash: 5afeb2e6709028f4f90b49701416b8f4 --=-j9HmL2j6bPVGp1vb6PwZ Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 =3D { > > + "proc": "/proc", > > + =E2=80=A6 > > + "port_logdir": "/var/log/portage", > > + } > > =E2=80=A6 > > @@ -173,11 +188,12 @@ class generic_stage_target(generic_target): > > file_locate(self.settings,["portage_confdir"],expand=3D0) > > =20 > > """ Setup our mount points """ > > + self.target_mounts =3D target_mounts.copy() > > if "SNAPCACHE" in self.settings: > > self.mounts=3D["proc", "dev", "portdir", "distdir", "port_tmpdir"] > > self.mountmap=3D{"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"} >=20 > 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). >=20 > I don't think the 'portage' =E2=86=92 self.settings["repo_name"] replacem= ent > should go in this commit. =20 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. >=20 > > - self.mounts.append("/var/log/portage") > > - self.mountmap["/var/log/portage"]=3Dself.settings["port_logdir"] > > - self.env["PORT_LOGDIR"]=3D"/var/log/portage" > > + self.mounts.append("port_logdir") > > + self.mountmap["port_logdir"]=3Dself.settings["port_logdir"] > > + self.env["PORT_LOGDIR"]=3Dself.settings["port_logdir"] >=20 > I don't know where we stand on 'x=3Dy' vs 'x =3D y', but I'd prefer the > latter here.=20 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: >=20 > self.env["PORT_LOGDIR"] =3D self.target_mounts["port_logdir"] >=20 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=3D{ "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 =3D { "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 =E2=86=92 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.) >=20 > > 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 =3D", x > > + target =3D normpath(self.settings["chroot_path"] + self.target_moun= ts[x]) > > + if not os.path.exists(target): > > + os.makedirs(target, 0755) > > =20 > > if not os.path.exists(self.mountmap[x]): > > if not self.mountmap[x] =3D=3D "tmpfs": > > - os.makedirs(self.mountmap[x],0755) > > + os.makedirs(self.mountmap[x], 0755) > > =20 > > src=3Dself.mountmap[x] >=20 > 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. > =20 > > - if "SNAPCACHE" in self.settings and x =3D=3D "/usr/portage": > > + #print "bind(); src =3D", src > > + if "SNAPCACHE" in self.settings and x =3D=3D self.settings["portdir= "]: >=20 > 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. >=20 > > + #print "bind(); cmd =3D", cmd >=20 > If it's not useful enough to print, I don't think we should commit it > ;). =20 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. >=20 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 :). >=20 > Cheers, > Trevor >=20 --=20 Brian Dolbec --=-j9HmL2j6bPVGp1vb6PwZ Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQF8BAABCgBmBQJSsTANXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXQ4Njg4RkQxQ0M3MUMxQzA0RUFFQTQyMzcy MjE0RDkwQTAxNEYxN0NCAAoJECIU2QoBTxfLDlsH/00gX3GL3U5GfRwIsIy8kXUC Us/mbZViIoD3nsujXnYxsdmAqEfGLyiPaIPJLqZ5/7QO5jI1bCVFM00TAIG6Rkbo 3ndI5pbjvIo8+g9es9+deFMJZhTGZyUnYRa6DqDvBRhas0W3197elL1suqunlJP3 7xBl16bBHtg+zO5FTTgnR8z7FXSnJnbrBV2xblbowD7iYyA+6x/v+CjY1EUC6RUO 2SoUEkUVP8N6Zu3CZwqZXYcTbCzFYQ6iTve9Td9eccIUjU7wamifd/tQWn0Wh2NS NUZriTgQAk7iK/5UV5IWXkQiyTNcXuZ+HoPd1xv6O/8eYekeRJQQLmmYY64EFEw= =fUUE -----END PGP SIGNATURE----- --=-j9HmL2j6bPVGp1vb6PwZ--