Opened 9 years ago

Closed 9 years ago

#1030 closed defect (fixed)

ImportError on OSX after merge of #1010

Reported by: jelte Owned by: jelte
Priority: medium Milestone: Sprint-20110628
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 2.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

After I merged #1010, the OSX test builder started reporting a problem:

Running test: master_test.py
Traceback (most recent call last):
  File "/Local/Users/jreed/builder/work/BIND10/20110621095437-MacOSX10.6-x86_64-Clang-Static/build/src/lib/python/isc/datasrc/tests/master_test.py", line 16, in <module>
    from isc.datasrc.master import *
  File "/Local/Users/jreed/builder/work/BIND10/20110621095437-MacOSX10.6-x86_64-Clang-Static/build/src/lib/python/isc/__init__.py", line 3, in <module>
    import isc.config
  File "/Local/Users/jreed/builder/work/BIND10/20110621095437-MacOSX10.6-x86_64-Clang-Static/build/src/lib/python/isc/config/__init__.py", line 1, in <module>
    from isc.config.ccsession import *
  File "/Local/Users/jreed/builder/work/BIND10/20110621095437-MacOSX10.6-x86_64-Clang-Static/build/src/lib/python/isc/config/ccsession.py", line 44, in <module>
    from isc.log import log_config_update
  File "/Local/Users/jreed/builder/work/BIND10/20110621095437-MacOSX10.6-x86_64-Clang-Static/build/src/lib/python/isc/log/__init__.py", line 36, in <module>
    from log import *
ImportError: dlopen(/Local/Users/jreed/builder/work/BIND10/20110621095437-MacOSX10.6-x86_64-Clang-Static/build/src/lib/python/isc/log/.libs/log.so, 2): Library not loaded: /Local/Users/jreed/builder/work/BIND10/20110621095437-MacOSX10.6-x86_64-Clang-Static/install/lib/liblog.0.dylib
  Referenced from: /Local/Users/jreed/builder/work/BIND10/20110621095437-MacOSX10.6-x86_64-Clang-Static/build/src/lib/python/isc/log/.libs/log.so
  Reason: image not found

This seems to go wrong for a lot of tests, so the patch is a bit more than trivial (in terms of size), therefore it seems wise to make a reviewable ticket for it.

Subtickets

Change History (5)

comment:1 Changed 9 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from new to reviewing

3 commits, the first two are small; the init.py (which is not installed and only used for in-tree running) now adds the modified path in the beginning of the pythonpath list, instead of at the end. Second change fixed the bus error regression (my merge had reintroduced the unnamed-namespace InternalError?).

Big change in terms of number of lines is the third; I've added that LIBRARY_PATH code to nearly every makefile that runs python tests (or at least every python test that includes config, which includes log since the merge of #1010)

Please review :)

comment:2 Changed 9 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:3 follow-up: Changed 9 years ago by jinmei

It basically looks okay, but I have some minor comments and questions:

log/init.py

  • not actually for this branch, but don't we need these in log/init.py?
    cwd = os.getcwd()
    base = os.path.split(cwd)[0]
    
  • what's the purpose of changing sys.path to sys.path[:]?
    -for base in sys.path:                                                       
    +for base in sys.path[:]:
    

changes to Makefile.am

There is some inconsistency in using space and tab when adding $(LIBRARY_PATH_PLACEHOLDER). For example, in bind10/tests/Makefile.am:

 	echo Running test: $$pytest ; \
+        $(LIBRARY_PATH_PLACEHOLDER) \
 	env PYTHONPATH=$(abs_top_srcdir)/src/lib/python:$(abs_top_builddir)/src/lib/python:$(abs_top_builddir)/src/bin/bind10 \

The added line is indented with space characters, while others were with a tab.

Not a big deal, but as always I generally prefer a consistent style:-)

comment:4 Changed 9 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:5 in reply to: ↑ 3 Changed 9 years ago by jelte

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

Replying to jinmei:

It basically looks okay, but I have some minor comments and questions:

log/init.py

  • not actually for this branch, but don't we need these in log/init.py?
    cwd = os.getcwd()
    base = os.path.split(cwd)[0]
    

oh, no they aren't necessary anymore, removed them.

  • what's the purpose of changing sys.path to sys.path[:]?
    -for base in sys.path:                                                       
    +for base in sys.path[:]:
    

it's a shorthand for making a (shallow) copy instead of assigning the reference; in the loop we modify the list, which happened to work with append(), but wouldn't with insert()

changes to Makefile.am

There is some inconsistency in using space and tab when adding $(LIBRARY_PATH_PLACEHOLDER). For example, in bind10/tests/Makefile.am:

 	echo Running test: $$pytest ; \
+        $(LIBRARY_PATH_PLACEHOLDER) \
 	env PYTHONPATH=$(abs_top_srcdir)/src/lib/python:$(abs_top_builddir)/src/lib/python:$(abs_top_builddir)/src/bin/bind10 \

The added line is indented with space characters, while others were with a tab.

Not a big deal, but as always I generally prefer a consistent style:-)

yeah i agree, my bad (i blame the vi on macmini :p)

merged and pushed, thanks for the quick look.

Note: See TracTickets for help on using tickets.