Re: [gentoo-catalyst] patch, fix broken seed stage update

[W. Trevor King]

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(), … for path manipulation

These are mostly “take advantage of Python's standard library”
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
>
> 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
>
> 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).
>
> 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.
>
> That's a long commit summary ;).  Perhaps the “Migrate…” portion
> should go into the commit message body, or that could be split out
> into a separate commit.

This splitting happened in 77eece8, but…

> This might also be a good time to transition from `CatalystError, …`
> to `CatalystError(…)` where you're touching lines.
>
> 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)

… this still applies.

> 7a909b9 cleanup long lines, improve useage() output formatting slightly
>
> Can we just switch from getopt to argparse (Python ≥2.7)?

Still applies to 7870959.

> 5eeb8b1 new minimal start script
>
> 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
>
> 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 contents
>
> 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
>
> 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
>
> After this, it's not clear to me what the difference is between
> catalyst.support and catalyst.util.  Perhaps they should be merged.
>
> 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
>
> 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.
>
> Really?
>
>   $ python3.3 -c "a = {1:2, 3:4}; print([x for x in a.keys()])"
>   [1, 3]
>
> On the other hand, it might be cleaner to just say:
>
>   for x in confdefaults:
>
> 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.
>
> 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:
>
>   confdefaults = {
>     'autoresume': False,
>     'ccache': False,
>     …
> 		}

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
>
> A lot of this is great stuff (Hooray no `import *`!).  However, I'd
> split the `print_traceback=True` changes out into their own commit (or
> just change the default value for CatalystError while you're testing).
>
> It looks like an unrelated change to
> catalyst.defaults.required_build_targets snuck into this commit.
>
> 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 snapshot_target to snapshot
>
> I think I like this.  It's good to use __import__.  You're missing a
> blank line after your import_module docstring summary [1].
>
> 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
>
> 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 paths, add some debug print statements, remove clear & purge code from support.py,
>
> Split the long commit summary.

This was fixed in 9fabaae, but…

> Can we use logging instead of print?
>
> The except (Failure loading " + x + " plugin) should be squashed into
> the commit that caused it and be restricted to only catch
> ImportErrors.
>
> The mountmap fix for 'proc' → '/proc' should be squashed into the
> commit that caused it.
>
> -               mypath=self.settings["chroot_path"]
> +               #mypath=self.settings["chroot_path"]
>
> Just remove this entirely, don't comment it out.

… these were not.

> a847803 Use normpath from support
>
> Why are we not using os.path.normpath?
>
> Also, remove instead of commenting out.

Still applies to be413a4.

> 242c17d rename all target .py file and classes without the "_target" so they are the same as the .sh files and work with teh simplified module loading.
>
> Move “so they …” into the commit message body and s/teh/the/.

With 6a86874, the commit body has “This is so they are the named the
same as the target…”, which should probably be “This is so they are
named the same as the target…” (removing an extra “the”).

> 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
>
> Long commit summary.

This is fixed in 4dd2cb2, but…

> os.path.join!  Also, convert debug prints to use the logging module
> ;).

… these are not.

> e0738c0 rename a make.conf key to make_conf due to bash variable name restrictions
>
> Squash into causing commit.

Still applies to 253dab7.

> ae61e4a reduce 2 operations into one simpler one
>
> Just switch to ConfigParser ;).

Still applies to 92c6745.

> b08a757 extend ParserBase to do variable substitution.
>
> 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
>
> Long commit summary.

This is fixed in b433a82, but…

> Your comment claims catalyst runtime executables for both sharedir and
> shdir.

… this is not.
 
> 55a58ee Add a forced debug print statement in cmd() for better debug output
>
> Python logging.

Still applies to 3791efc.

> 24d69bd Commit my testpath file with instructions to run the git checkout code directly without being installed.
>
> s/caatalyst/catalyst/
>
> It would also probably be a good idea to add an example test.conf with
> relative paths.

Still applies to 786a01c.

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