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 F1441198005 for ; Tue, 26 Feb 2013 18:04:37 +0000 (UTC) Received: from pigeon.gentoo.org (localhost [127.0.0.1]) by pigeon.gentoo.org (Postfix) with SMTP id 64A54E0029; Tue, 26 Feb 2013 18:04:36 +0000 (UTC) Received: from vms173003pub.verizon.net (vms173003pub.verizon.net [206.46.173.3]) by pigeon.gentoo.org (Postfix) with ESMTP id C17C3E0029 for ; Tue, 26 Feb 2013 18:04:35 +0000 (UTC) Received: from odin.tremily.us ([unknown] [72.68.84.219]) by vms173003.mailsrvcs.net (Sun Java(tm) System Messaging Server 7u2-7.02 32bit (built Apr 16 2009)) with ESMTPA id <0MIU00187A7GLQ00@vms173003.mailsrvcs.net> for gentoo-catalyst@lists.gentoo.org; Tue, 26 Feb 2013 12:04:31 -0600 (CST) Received: by odin.tremily.us (Postfix, from userid 1000) id AC3778C69FD; Tue, 26 Feb 2013 13:04:28 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tremily.us; s=odin; t=1361901868; bh=QfBP5BG5vQ/nXRmCRINRP4/yYte3VJv+zLkAKhI+c24=; h=Date:From:To:Cc:Subject:In-Reply-To; b=KPZW1S1XWYRQV0iYkTZfiQ2am0BQAK7Z0cESFzVpLuxCQXyCVaCp+pkg2quix8D+O +OF41q2rluinHjVoSIFmax5VINSPdY2aADM04U45auZkoEiGVW2gNDcxjmyei/Xr84 poxVLYPsC3Ucyxjk1fiaAFEI4/x9qfmEISWcqi8Q= Date: Tue, 26 Feb 2013 13:04:28 -0500 From: "W. Trevor King" To: gentoo-catalyst@lists.gentoo.org Cc: zmedico@gentoo.org, fuzzyray@gentoo.org Subject: Re: [gentoo-catalyst] patch, fix broken seed stage update Message-id: <20130226180428.GA22651@odin.tremily.us> 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 Content-type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="PEIAKu/WMn1b1Hv9" Content-disposition: inline In-reply-to: <1361897234.3997.272.camel@big_daddy.dol-sen.ca> <20130203124436.GB27214@odin.tremily.us> <20130131194653.GA4540@odin.tremily.us> <20130203122056.GA27214@odin.tremily.us> <20130131183922.GA3946@odin.tremily.us> OpenPGP: id=39A2F3FA2AB17E5D8764F388FC29BDCDF15F5BE8; url=http://tremily.us/pubkey.txt User-Agent: Mutt/1.5.21 (2010-09-15) X-Archives-Salt: 93c8c64f-c195-47ec-b598-a929834106b5 X-Archives-Hash: 88c6e75ee364d309164a3bf695ab350b --PEIAKu/WMn1b1Hv9 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Feb 26, 2013 at 08:47:14AM -0800, Brian Dolbec wrote: > Also I've rebased everything on current master That should make things easier to merge :). It looks like some of my earlier comments were addressed by this reroll, but some are still applicable. Apologies if we'd resolved any of this earlier and I just missed the reference in my mailbox. I map my old comments onto the rebased commits below, but the bulk of the outstanding suggestions revolve around: * ConfigParser-based configuration * Argparse-based command line parsing * Logging-based debugging output * os.path.join(), normpath(), =E2=80=A6 for path manipulation These are mostly =E2=80=9Ctake advantage of Python's standard library=E2=80= =9D changes, and I'd be happy to help implement them on top of the current master if folks feel like that has a chance of getting merged ;). On Thu, Jan 31, 2013 at 01:39:22PM -0500, W. Trevor King wrote: > 38cd3bb add more configured defaults >=20 > List the new settings (distdir, repo_name, packagedir, port_tmpdir, > options, snapshot_name) in the commit message so I don't have to read > the diff. Probably explain why you think they should be configurable > as well. `options` is also a pretty ambiguous name. This still applies to 0a25452. I made a few comments about using separate boolean options instead of an aggregate `options` set. Fixing this should be part of the ConfigParser transition. > 016704a use the new configured snapshot_name and portdir settings >=20 > Rather than a separate add (38cd3bb) and use (016704a), I think it > would be better if new options had their addition and use rolled into > a single commit (e.g. add snapshot_name and use it as one commit, add > portdir and use it as another commit). >=20 > This might also be a good place to move the path construction over to > use os.path.join(). This still applies to be2f820. > 63a25eb Remove self.mounts and self.mountmap's use of paths for keys and = paths. Migrate more hardcoded paths to use settings. >=20 > That's a long commit summary ;). Perhaps the =E2=80=9CMigrate=E2=80=A6= =E2=80=9D portion > should go into the commit message body, or that could be split out > into a separate commit. This splitting happened in 77eece8, but=E2=80=A6 > This might also be a good time to transition from `CatalystError, =E2=80= =A6` > to `CatalystError(=E2=80=A6)` where you're touching lines. >=20 > I'm not sure which versions of Python Catalyst is trying to support, > but I'd be happy to see the beginings of a migration to '{}'.format() > for building strings (vs. the current `'' + ''` concatenation) =E2=80=A6 this still applies. > 7a909b9 cleanup long lines, improve useage() output formatting slightly >=20 > Can we just switch from getopt to argparse (Python =E2=89=A52.7)? Still applies to 7870959. > 5eeb8b1 new minimal start script >=20 > I think __version__ and __maintainer__ should live in > catalyst/__init__.py. Still applies to baae19f. > 299e35d update the module loading paths for the new locations >=20 > Can we just drop this import manipulation and use __all__? Still applies to 79f73a3. > a7206bb rename files directory to etc to better reflect the directories c= ontents >=20 > Actually, files/ is also used for other things (e.g. built man pages, > see MAN_PAGES in the Makefile). If we keep the rename to etc/, we > might to just build the man pages under doc/. Still applies to 25f6f1b. > f9f18be update gitignore >=20 > The Scratch, catalystc, *.geany, and test* entries would appear to be > specific to your local installation and usage. I'd drop them from the > reroll. They're still in 7fdecf4, but it will be easy to drop this before the merge with master. On Thu, Jan 31, 2013 at 02:46:53PM -0500, W. Trevor King wrote: > 968d818 Initial creation of a defaults file. Split out hash and contents = to their own classes, files >=20 > After this, it's not clear to me what the difference is between > catalyst.support and catalyst.util. Perhaps they should be merged. >=20 > I'd also use catalyst.targets.__all__ instead of coding a list of > targets in the apparently unrelated > catalyst.defaults.valid_build_targets. Still applies to c97dc3d. > 9d752a7 move confdefaults out of main.py >=20 > Looks good, except, I'm not sure why you changed from > `confdefaults.keys()` to `list(confdefaults)` in parse_config() (which > should probably be living in catalyst.config anyway). Still applies to 0c2302a. Additional discussion from a sub-thread: On Sun, Feb 03, 2013 at 07:44:36AM -0500, W. Trevor King wrote: > On Sat, Feb 02, 2013 at 12:41:32PM -0800, Brian Dolbec wrote: > > As for list(confdefaults), py3 compatibility. dict.keys() isn't usable > > and 2to3 converts it to list(dict)... something about needing to specify > > the return type. So is a preemptive change. One less thing to change > > later. >=20 > Really? >=20 > $ python3.3 -c "a =3D {1:2, 3:4}; print([x for x in a.keys()])" > [1, 3] >=20 > On the other hand, it might be cleaner to just say: >=20 > for x in confdefaults: >=20 > But this should still go into a separate commit. I still think this is true ;). On Thu, Jan 31, 2013 at 02:46:53PM -0500, W. Trevor King wrote: > c303dae some options cleanup, unifying their use, reducing redundancy. >=20 > While I like the general thrust of this, I'd be happier with explicit > boolean options instead of a set of boolean options. For example: >=20 > confdefaults =3D { > 'autoresume': False, > 'ccache': False, > =E2=80=A6 > } Still applies to ca85cd4. Like I said above, I'm happy to work up a ConfigParser-based solution. > 04068a1 massive pyflakes import cleanup and broken CatalystError calls >=20 > A lot of this is great stuff (Hooray no `import *`!). However, I'd > split the `print_traceback=3DTrue` changes out into their own commit (or > just change the default value for CatalystError while you're testing). >=20 > It looks like an unrelated change to > catalyst.defaults.required_build_targets snuck into this commit. >=20 > The hash_map changes should probably be squashed into the earlier hash > reorganization, rather than going into this commit. Still applies to eed08b1. > 5c689a8 update module loading for the new python structure, rename snapsh= ot_target to snapshot >=20 > I think I like this. It's good to use __import__. You're missing a > blank line after your import_module docstring summary [1]. >=20 > I think that the import_module API should match Python 3's > importlib.import_module [2]. Still applies to still applies to f733b3f. > 669451d fix options being reset by a config file >=20 > See my comments on c303dae. We should really be using ConfigParser's > defaults here, instead of rolling our own default system. ConfigParser :). > c5eb7f7 fix mounts, mountmap hardcoding removal, use normpath to fix path= s, add some debug print statements, remove clear & purge code from support.= py, >=20 > Split the long commit summary. This was fixed in 9fabaae, but=E2=80=A6 > Can we use logging instead of print? >=20 > The except (Failure loading " + x + " plugin) should be squashed into > the commit that caused it and be restricted to only catch > ImportErrors. >=20 > The mountmap fix for 'proc' =E2=86=92 '/proc' should be squashed into the > commit that caused it. >=20 > - mypath=3Dself.settings["chroot_path"] > + #mypath=3Dself.settings["chroot_path"] >=20 > Just remove this entirely, don't comment it out. =E2=80=A6 these were not. > a847803 Use normpath from support >=20 > Why are we not using os.path.normpath? >=20 > Also, remove instead of commenting out. Still applies to be413a4. > 242c17d rename all target .py file and classes without the "_target" so t= hey are the same as the .sh files and work with teh simplified module loadi= ng. >=20 > Move =E2=80=9Cso they =E2=80=A6=E2=80=9D into the commit message body and= s/teh/the/. With 6a86874, the commit body has =E2=80=9CThis is so they are the named the same as the target=E2=80=A6=E2=80=9D, which should probably be =E2=80=9CThi= s is so they are named the same as the target=E2=80=A6=E2=80=9D (removing an extra =E2=80=9C= the=E2=80=9D). > e05cc34 Break out a few more repeated (path1 + path2)'s to doing it once= and using the temp variable. comment out some debug print's >=20 > Long commit summary. This is fixed in 4dd2cb2, but=E2=80=A6 > os.path.join! Also, convert debug prints to use the logging module > ;). =E2=80=A6 these are not. > e0738c0 rename a make.conf key to make_conf due to bash variable name res= trictions >=20 > Squash into causing commit. Still applies to 253dab7. > ae61e4a reduce 2 operations into one simpler one >=20 > Just switch to ConfigParser ;). Still applies to 92c6745. > b08a757 extend ParserBase to do variable substitution. >=20 > ConfigParser does that to ;). Still applies to 104c387. > 265c8ed add a "shdir" setting to make moving the bash code around easier = and have less hardcoded paths in the bash scripts >=20 > Long commit summary. This is fixed in b433a82, but=E2=80=A6 > Your comment claims catalyst runtime executables for both sharedir and > shdir. =E2=80=A6 this is not. =20 > 55a58ee Add a forced debug print statement in cmd() for better debug outp= ut >=20 > Python logging. Still applies to 3791efc. > 24d69bd Commit my testpath file with instructions to run the git checkout= code directly without being installed. >=20 > s/caatalyst/catalyst/ >=20 > It would also probably be a good idea to add an example test.conf with > relative paths. Still applies to 786a01c. Cheers, Trevor --=20 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 --PEIAKu/WMn1b1Hv9 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.19 (GNU/Linux) iQIcBAEBAgAGBQJRLPkkAAoJEEUbTsx0l5OMiroP/RM9ET/B/GwHyXHOZujG54u9 AxaI3G2FxMQFIgPpHL+DD9U6pMraHrlPvojYg224dApEjIh91pZ6d3IQgmBR5ApR x7PuEukzNzGiLFTKUji2be06ICxgQHbodmJJ7boslWqa/cbiQ/sFb4u/HdNPbypV zAoV67Mzp1Qg2Pb2g2LOZypkVVCkW++6UNK6ePEEAKR3XasOvt0y+M9uryH393s6 olkRfSGY+Y4nAdWBumFeBhc+h5afOPptY4SUkXfCnCTYENtFu71qUo2RpMkDNX3m yK92t7DEJ5GQ2i/x8hoTzU5f70EdDfFSnazchb+MMWxjqu6AGJF8W6d9shg1+Fy3 Mfcwywh4XxWEL6KCMfwwlOpy8izDQ6GB/ZSDeVnmJP4JRc0y8UygZ+vhjB5TeKFx goaOTdEi05lZn+nFS7e3LK/ZfEk/VI6x+Rs2wB90X88NhpbiALnO8YmFqOtTQN+H 8or6dHtC8PjT6eeOASkpxRjuK2dvRt65X2N5ro/mKP1wJImIlU9Uw2NuywTlAvTZ Q0f6eSw6lwyfiiKB63WcQFbvThiaQ0TtyhYNJ80K3Lwujzkn8KRMaZHYTWiOVDNT Wvaz6Z3U8TsbjxTf3PjCgf8FZ8zyhi7fXb2x6w6R1HAOZfYXFO1nPVDFinqWELNG 3BlsL4vPmnApYTtG8eaJ =Sh9L -----END PGP SIGNATURE----- --PEIAKu/WMn1b1Hv9--