Opened 9 years ago

Closed 8 years ago

#1101 closed defect (complete)

log message .py's should be installed under site-packages/isc/

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20110927
Component: logging Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 4.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Currently, log messages in the form of python scripts (such as
cfgmgr_messages.py) are installed in $(pyexec), which would be
something like /usr/local/lib/python3.1/site-packages and is
shared by other third-party python modules.

As I commented in #764 (http://bind10.isc.org/ticket/764#comment:8),
I don't think it a good practice. To summarize the points:

  • to be in such a generic package directory, the file names are (generally) too generic, which would confuse users and might cause conflict with other modules.
  • It would also look awkward in the caller (such as xfrout.py) side: most of BIND 10 modules are imported with a prefix of "isc", but python modules for the log messages are missing it.

I suggest making a separate directory under "isc", e.g,
"isc/log_message" and install all *_messages.py files there.
Note that we'd need something like what I did for #983 (commit
b9bc51b): we'd need a separate variable in configure.ac, and
specify the destination of the log files as
$(PYTHON_SITEPKG_DIR)/isc/log_message

Subtickets

Change History (16)

comment:1 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 4

comment:2 Changed 9 years ago by shane

  • Milestone changed from Next-Sprint-Proposed to Sprint-20110830

comment:3 Changed 9 years ago by jelte

we'd also need some way to gank the pythondir path searching if we want to be able to run from source tree (or make the target dir a separate directory like isc_messages on the same level as isc)

comment:4 Changed 8 years ago by jinmei

  • Owner set to jinmei
  • Status changed from new to accepted

comment:5 Changed 8 years ago by jinmei

Branch trac1101 is ready for review.

See src/lib/python/isc/log_messages/README for implementation details.

Many files have been modified, but they are basically straightforward
application of what's written in README.

Proposed changelog entry:

286.?	[bug]*		jinmei
	Python script files for log messages (xxx_messages.py) should have
	been installed under the "isc" package.  This fix itself should
	be a transparent change without affecting existing configurations
	or other operational practices, but you may want to clean up the
	python files from the common directly (such as "site-packages").
	(Trac #1101, git TBD)

comment:6 Changed 8 years ago by jinmei

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

comment:7 follow-up: Changed 8 years ago by jelte

There is one problem; it works with a build directory when unpacked from the tarball, but not with a build directory straight from source, since the 'import-work' intermediate files end up in the build dir...

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

Replying to jelte:

There is one problem; it works with a build directory when unpacked from the tarball, but not with a build directory straight from source, since the 'import-work' intermediate files end up in the build dir...

Hmm, I just confirmed that 'make check' failed if I built it in a
separate directory (starting from "mkdir _build; cd _build;
../configure <options>; make") as follows:

Running test: master_test.py
Traceback (most recent call last):
  File "/Users/jinmei/src/isc/git/bind10-1101/_build/../src/lib/python/isc/datasrc/tests/master_test.py", line 16, in <module>
    from isc.datasrc.master import *
  File "/Users/jinmei/src/isc/git/bind10-1101/src/lib/python/isc/__init__.py", line 3, in <module>
    import isc.config
  File "/Users/jinmei/src/isc/git/bind10-1101/src/lib/python/isc/config/__init__.py", line 1, in <module>
    from isc.config.ccsession import *
  File "/Users/jinmei/src/isc/git/bind10-1101/src/lib/python/isc/config/ccsession.py", line 46, in <module>
    from isc.log_messages.config_messages import *
ImportError: No module named log_messages.config_messages

Is this what you meant?

(I thought this should work as 'make distcheck' succeeded, but I have to confess I haven't checked this case explicitly).

comment:9 follow-up: Changed 8 years ago by jelte

yeah that is the problem i meant. I haven't looked closely at a possible solution yet. Perhaps we can make the original init.py in srcdir log_messages/ include all of work/ from build (instead of individually per log_xxx_.py file)? (something like that was done in the lib/isc/log bindings iirc, though a somewhat cleaner solution would be nice)

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

Replying to jelte:

yeah that is the problem i meant.

Okay I switched to Plan B. Instead of creating the in-source .py's
that just forwards the import to the corresponding .py undr work/,
I've created these .py's under the management of git. I've also changed
init.py.in to init.py on the srcdir (I also noticed it's not
actually needed on builddir).

I chose the original way because that would allow everything to reside
in the module src directly. Plan B requests the developer to update
both the module and log_messages, which I wanted to avoid. But on
the other hand this approach is more straightforward and probably
easier to understand. And, of course, this solves the build problem
you found. So I think it's acceptable.

Does that work for you, too, and if so you think it's okay?

If so, I'll also update the README file.

comment:11 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to jelte

comment:12 follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

Yes, I think this is a reasonable compromise.

The changes so far look ok, btw, and I can verify that it does work for me now

comment:13 in reply to: ↑ 12 Changed 8 years ago by jinmei

Replying to jelte:

Yes, I think this is a reasonable compromise.

The changes so far look ok, btw, and I can verify that it does work for me now

Okay, thanks. I've updated README and made some cleanups. I hope
it's ready to merge.

comment:14 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:15 follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

Yes, looks good, please go ahead.

comment:16 in reply to: ↑ 15 Changed 8 years ago by jinmei

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

Replying to jelte:

Yes, looks good, please go ahead.

Thanks, merged, closing ticket.

Note: See TracTickets for help on using tickets.