Re: BIND 10 #322: Process name in Python

[BIND 10 Development]

#322: Process name　in Python
-------------------------------+--------------------------------------------
      Reporter:  fujiwara      |        Owner:  vorner   
          Type:  enhancement   |       Status:  reviewing
      Priority:  major         |    Milestone:           
     Component:  Unclassified  |   Resolution:           
      Keywords:                |    Sensitive:  0        
Estimatedhours:  0.0           |        Hours:  1        
      Billable:  1             |   Totalhours:  2.0      
      Internal:  0             |  
-------------------------------+--------------------------------------------
Changes (by jinmei):

  * hours:  0.0 => 1.0
  * owner:  jinmei => vorner
  * totalhours:  2.0 => 3.0


Comment:

 Replying to [comment:5 vorner]:
 > Replying to [comment:3 jinmei]:
 > > '''general comments'''
 > >  - please provide suggested ChangeLog entry.
 >
 > Sorry, I forgot:
 > {{{
 > [func]                Michal Vaner
 > Python processes: support naming of python processes so
 > they're not all called python3. (Trac #322, svn rTBD)
 > }}}
 >
 This looks fine.

 > >  - it strips path to the program name by default, so we'll see
 "bind10" instead of "/usr/local/sbin/bind10", etc.  Personally, I'd like
 to know the latter, because I may have multiple different versions
 installed in different places and I'd like to distinguish them.
 >
 > That will be a little bit of a problem. Many tools (top, pstree, …) cut
 too long names, so we would have many processes that would look like
 „/usr/share/local/libe“ and the important information which actual process
 it is would be not visible (and I think this is more common than having
 two binds).
 >
 Hmm, but from quick checks the top command I can use (including one
 available on bind10.isc.org) seems to cut the path itself.  I've never
 used pstree, but the one installed on bind10.isc.org seems to do the same
 thing.

 > On the other hand, which installation it is is valuable only for the
 boss. So maybe boss could have some more clever naming (like bind10
 <version>) with possibility to provide custom name by --prettyname, while
 other „slave“ processes would have just their basename? Would that be
 suitable?
 >
 Perhaps, and one possible compromise would be to add the path to bind10
 with --prettyname while keeping others short.

 But in any case I see this is probably a matter of preference.  So I'd
 leave the decision to you.

 > >  - we normally call files for tests "xxx_test.py" (it's redundant
 because the directory name indicates it's related to tests.  but on the
 other hand it helps to separate real test code and other helper files).
 it's not a strong requirement, but unless you strongly want to keep this
 naming I'd suggest follow the convention for consistency.
 >
 > Actually, both are standard, single underscore is more or less
 equivalent to C++ „protected“, while two is „private“ (and does name
 mangling). Because I do not expect this class to be inherited, I can
 change that, but I try to use lowest possible permissions. Should I change
 it?
 >
 You seem to reply to a different point:-)  But I understand your
 explanation and I'm fine with __.  I also see you renamed the test file
 name.

 > > ''' tests/rename.py:TestRename.test_calls()'''
 > >  - I respect the paranoia:-), and I actually like it, but I suspect
 this is beyond the scope of unit tests.
 >
 > I see „make check“ more like a code-level check than just unittests,
 „make distcheck“ seems to me a check if it can be packed and stuff (more
 related to the build system than to the actual code). So I think this
 should belong there. I could probably find a better place (like top-level
 check/tests directory, since I test the whole source tree), but this
 seemed like less effort.
 >
 'make distcheck' also tries build and test.  anyway, I didn't necessarily
 mean 'make distcheck' itself.  By "some kind of" I meant something like a
 meta test (which may not fit in the automake scheme).

 > >  - more practically, this (at least currently) doesn't work if you try
 'make check' for a vanilla tree.  so, I'd suggest this test is separated
 as part of some kind of "distcheck" level check.
 >
 > It does now :-).
 >
 No it doesn't (or I was not clear in my previous comment and we are
 talking about different things).  For example, in bind10/Makefile.in we
 have
 {{{
 sbin_SCRIPTS = bind10
 }}}

 So the process_test looks for {top_dir}/src/bin/bind10/bind10.

 But if we do ./configure then 'make check' immediately, "bind10" doesn't
 exist at the time the test is invoked because make first goes down to
 src/lib and then src/bin.  As a result you'd see:
 {{{
 ======================================================================
 ERROR: test_calls (__main__.TestRename)
 ----------------------------------------------------------------------
 Traceback (most recent call last):
   File
 "/Users/jinmei/src/isc/git/bind10/src/lib/python/isc/utils/tests/process_test.py",
 line 74, in test_calls
     self.__scan(d, script, fun)
   File
 "/Users/jinmei/src/isc/git/bind10/src/lib/python/isc/utils/tests/process_test.py",
 line 44, in __scan
     data = ''.join(open(filename).readlines())
 IOError: [Errno 2] No such file or directory:
 '../../../../../../src/bin/bind10/bind10'
 }}}

 > >  - the regular expression "lines" is difficult to understand.  Why \\w
 instead of \w?  same for \\s, and what does \\\\\\n mean?  If it can be
 simplified, please do so.  And in any case, I think this requires a
 descriptive comment.
 >
 > The other ones were wrong, not this one (but thanks for pointing out the
 difference) ‒ one backslash is eaten by python string parsing. I found the
 r'string' prefix, that does not eat anything, so it looks little bit more
 sane.
 >
 Ah, right.  Hmm, I forgot to add 'r' or escape backslashes in some of my
 python scripts, but it seems to work without a problem...strange.  But
 you're right in terms of the language specification, and the latest code
 looks okay.

 > The \\\\\\n is „backslash and newline“ (the backslash had to be double-
 backslashed).
 >
 > (I added the comment, I'm too used to perl code I forget regexps are
 hard to read for many people)
 >
 The comment helps, thanks.

 > >  - what's the purpose of second variable (_) in the for ... findall
 idiom? e.g.
 {{{
 +                for (var, _) in lines.findall(makefile):
 }}}
 > >  it seems to me it could simply be for var in lines.findall(makefile)
 >
 > findall returns list touples if there is more than one group in the
 regexp, in this case list of pairs. Since the second one is not
 interesting to me, only to the regexp, I unpack the pairs by (, ) and the
 _ is a „throwavay variable“. If I used just „for var in …“, var would
 contain the pair, not just its first component.
 >
 Ah, I didn't realize the inner group ("(.|\\\n)") affects the findall
 result.  The code okay.

 BTW, if you intentionally use Makefile.in, you should also modify the
 comment:
 {{{
         # Find all Makefile.am and extract names of scripts
 }}}
 i.e. s/Makefile.am/Makefile.in/ (but note the more fundamental problem
 that the immediate 'make check' doesn't work whether or not it's
 Makefile.in)

 One more thing: there's still a redundant DYLD hack in Makefile.am:
 {{{
         $(LIBRARY_PATH_PLACEHOLDER) \
 }}}

 This line can simply be removed.

-- 
Ticket URL: <http://bind10.isc.org/ticket/322#comment:6>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development