Hi Stan,
Apologies for taking so long to get back to this... (had to dig through
my old mailbox just to find it!).
On 31 July 2012 07:25, Stan Cox <scox@xxxxxxxxxx> wrote:
> The python bindings:
> 1) Now include Nathan Scott's cleanups for current headers for pcp.py
> 2) Uses Nathan Scott's suggestion to check pmDesc.sem for the metric type
> for pm-collectl.py
Cool, thanks - looks good.
> 3) pm-collectl.py now supports -sd -sc -sj -sn -sD -sC -sJ -sN --verbose
> 4) GNUmakefile uses setup.py for building and installing
>
>
> 1) Does it seem reasonable to include python upstream?
Nobody else seems to have a strong opinion, and I think there's some
good advantages (esp. related to ensuring it stays up to date with new
macros/functions/features, as well as getting it out to developers).
> 2) If yes are there any "to do" items folks think are required before
> pushing?
Looks good. I've done another review - and a few more suggestions follow,
but no need to wait on merging for these (its in dev branch now, we should
shoot for 3.6.6, possiblly late next week, preferably with the below things
discussed and updated where need be).
src/python/p*.py
- I fixed a few comments up a little, please double check for me
- I was unsure about pmNumberStr being marked as a NOOP, does the
suffix addition happen automatically in python? If so, perhaps we don't
need any entry point here at all?
- should we be using the reentrant versions of pmTypeStr, pmAtomStr,
pmNumberStr? (i.e. the _r suffixed versions). I suspect we should, no?
- still makes reference to being "pre-alpha" - is it worth moving that to
a different state?
- there's references to "127.0.0.1" in a couple of places ... hmmmm. In
libpcp, theres a couple of references to "localhost" - can we use that
instead? (its at least not an IPv4 address then).
setup and test scripts:
- setup script - says Pcp is gplv3, which differs to rest of PCP (refer to
top-level COPYING). We either need to make these match up (2), or
we need an additional COPYING file for the python module).
- setup script also has a url pointer to ftp://oss.sgi... download area - it
might be better to point that at http://oss.sgi.com/projects/pcp/ now?
- test_pcp.py - awesome, can we automate the running of this? a new
test in qa/XXX (next free number) that checks for the Pcp module in the
system root (not in the source tree) and runs this if found (use _notrun
if not).
src/python/GNUmakefile
- I made a couple of minor changes here, be worth checking 'em for me :)
- looks like the makefile will need further tweaks around Franks prefix work?
comments there certainly suggest so. Sounds like Franks close on that.
- need to consider platforms that have no python installed - the build will
likely fail there now, and thats going to need (hopefully trivial) fixing.
- usually the toolchain used in the build is abstracted by macros that can
be overridden (e.g. via configure) - we should introduce a $(PYTHON) like
$(AWK) and $(SED) and friends, if we need to invoke it directly (appears
we do). might be able to use this macro to help with the above point too.
cheers.
--
Nathan
|