Opened 9 years ago

Closed 9 years ago

#322 closed enhancement (fixed)

Process name in Python

Reported by: fujiwara Owned by: vorner
Priority: medium Milestone:
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket: 0
Total Hours: 3.75 Internal?: no

Description

Process names of Python programs are not set and "ps" command shows "python3.1" only for BIND 10 components written by Python.
setproctitle() solves this problem.
setproctitle() is not included python 3.1 distribution,
but there are some implementations.
setproctitle 1.1 can run on Linux, BSD, MacOS X, Winows.

http://pypi.python.org/pypi/setproctitle

Subtickets

Change History (11)

comment:1 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to vorner
  • Status changed from new to accepted

I'll take it as a junior job to look trough the code.

comment:2 follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to UnAssigned
  • Status changed from accepted to reviewing

I think it is somehow implemented. It does not fail at runtime if the library is not installed (as it is only a minor function).

There are two think I'm not really sure of. One is, the test that checks all python processes rename themself does it by examining the code, not running anything.

The second, if the library is not installed, configure complains and has an option to skip this check. It was recomended to me in the mailling list, other option would be to bundle the library and install it as third-party, but it seems too small feature to do so for some people.

It is the trac322 branch, branchpoint 2863, head 2997.

comment:3 in reply to: ↑ 2 ; follow-up: Changed 9 years ago by jinmei

  • Owner changed from UnAssigned to vorner

general comments

  • please provide suggested ChangeLog? entry.
  • I'm afraid the library name "bind10" maybe misleading because it sounds like something related to the bind10 process. I have no specific idea about a better name, but something like "util"?
  • Likewise, I suspect "rename" is too generic. We see in the program code:
    import isc.bind10.rename
    isc.bind10.rename.rename()
    
    and it's not obvious that it renames the process title.

configure.ac

  • I'd clarify "--disable-setproctitle-check" is about a python library, either or both in the option name and option description.

rename.py

  • please add the copyright notice
  • please describe this is a wrapper interface to the setproctitle library.
  • 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.

bind10/tests/Makefile.am

  • you don't need the LIBRARY_PATH hack for this test. it's necessary only for those that require our C++ loadable modules.

tests/rename.py

  • 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.
  • some methods are named "xxx". in other python code we generally seem to use "_xxx". I don't know which one is the standard way, but it's better to be consistent.
  • scan(): variable repl doesn't seem to be used.
  • scan(): according to the manual assert_ is deprecated by assertTrue().

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.
  • 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.
  • 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.
  • os.walk(): I don't like to hardcode the path for the 1st argument. I'd use top_srcdir and have the configure script replace it.
  • os.walk(): do we really need to follow symbolic links (despite the risk of causing an infinite loop)?
  • 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)

comment:4 Changed 9 years ago by jinmei

  • Add Hours to Ticket changed from 0.0 to 2.0
  • Total Hours changed from 0.0 to 2.0

comment:5 in reply to: ↑ 3 ; follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jinmei

Replying to jinmei:

general comments

Sorry, I forgot:

[func]		Michal Vaner
Python processes: support naming of python processes so
they're not all called python3. (Trac #322, svn rTBD)
  • 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).

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?

  • 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?

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.

  • 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 :-).

  • 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.

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)

  • 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.

Changes are on the branches/trac322 branch.

Thanks for your input.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 9 years ago by jinmei

  • Add Hours to Ticket changed from 0.0 to 1.0
  • Owner changed from jinmei to vorner
  • Total Hours changed from 2.0 to 3.0

Replying to vorner:

Replying to jinmei:

general comments

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.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jinmei

Replying to jinmei:

Replying to vorner:

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.

Well, there seem to be two (or more) process names or something, it cut the path if it is the name automatically assigned to it by system. But if I try to assign it by the setproctitle library, it does not cut the path, it cuts it from the end if it is too long, no matter if it is just some text or real path.

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.

Ok, BoB now has full path/arg 0, others have a basename and BoB has the --pretty-name parameter.

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:

Ah, sorry, I misunderstood what you ment by vanilla tree. I moved it into src/bin/tests and made sure the scripts are created first. I didn't reallize anyone would like to run make check and not run make first.

The make check fails now, if it is run directly, but with something unrelated that is fixed in trunk. If I merge them locally, it is fine (I didn't put the merge in SVN, since then the diff of a branch is harder to create).

Thanks

comment:8 in reply to: ↑ 7 Changed 9 years ago by jinmei

  • Add Hours to Ticket changed from 0.0 to 0.75
  • Owner changed from jinmei to vorner
  • Total Hours changed from 3.0 to 3.75

The latest code looks mostly okay. Some minor points:

  • please describe the new option in the man page of bind10
  • no test for the new option?
  • src/bin/tests is quite special in that the programs there are not expected to be used by normal users. maybe we need some notes about that in a README file or something
  • in src/bin/Makefile.am the order of SUBDIRS has a meaning (tests must be the last), which is not obvious. Please add comment about it.

comment:9 follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jinmei

I need to know better what is where, I didn't have an idea we have man pages already (I should have expected them).

The test is in args_test.py now (I overlooked the file, so I thought no options are tested), which is not run by default. The args_test.py was reported to fail without real reason sometimes and the test of the option is similar to the ones there, so it might have similar tendencies (because it actually runs bind10).

And no, the order does not matter. The next line states that all the code must be compiled before tests are run.

comment:10 in reply to: ↑ 9 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

Replying to vorner:

I need to know better what is where, I didn't have an idea we have man pages already (I should have expected them).

The test is in args_test.py now (I overlooked the file, so I thought no options are tested), which is not run by default. The args_test.py was reported to fail without real reason sometimes and the test of the option is similar to the ones there, so it might have similar tendencies (because it actually runs bind10).

And no, the order does not matter. The next line states that all the code must be compiled before tests are run.

Okay, I think the branch is ready to merge. Whether to test argst_test automatically may be an issue, but I personally think it can be deferred.

comment:11 Changed 9 years ago by vorner

  • Resolution set to fixed
  • Status changed from reviewing to closed

Thanks, merged.

Note: See TracTickets for help on using tickets.