Drop apputils dependency #276
Conversation
|
@haberman green :) |
|
Hi there, I wanted to add Travis tests for the Python build before committing this. Would you mind rebasing and fixing .travis.yml for the new testing stuff (ie. replace google_test with test)? |
|
@haberman yep, already done. Only trouble is they're not passing for the moment :( |
|
@haberman this is failing because https://github.com/tamird/protobuf-1/blob/drop-apputils-dependency/python/google/protobuf/internal/descriptor_python_test.py#L37 seems not to be doing anything - it's not really setting the implementation to |
|
Oh did you figure out the other weird errors related to duplicate symbols in .proto files? |
|
|
||
| $ (cd .. && make) | ||
|
|
||
| On OS X: |
There was a problem hiding this comment.
I think DYLD_LIBRARY_PATH on OS X behaves like LD_LIBRARY_PATH on Linux? http://stackoverflow.com/questions/3146274/is-it-ok-to-use-dyld-library-path-on-mac-os-x-and-whats-the-dynamic-library-s
There was a problem hiding this comment.
It does, but dyld (the OS X linker) links the full path at link-time, so
DYLD_LIBRARY_PATH isn't needed in the simple case described here.
On Apr 10, 2015 4:11 PM, "Joshua Haberman" notifications@github.com wrote:
In python/README.md
#276 (comment):
To build the C++ implementation run:$ python setup.py build --cpp_implementationTo build, test, and use the C++ implementation, you must first compilelibprotobuf.so:
$ (cd .. && make)
On OS X:I think DYLD_LIBRARY_PATH on OS X behaves like LD_LIBRARY_PATH on Linux?
http://stackoverflow.com/questions/3146274/is-it-ok-to-use-dyld-library-path-on-mac-os-x-and-whats-the-dynamic-library-s—
Reply to this email directly or view it on GitHub
https://github.com/google/protobuf/pull/276/files#r28176356.
There was a problem hiding this comment.
I guess what I'm getting at is that it would be nice if the README didn't have to have an entire separate section to address OS X, brew, etc. Your caveat about Brew doesn't apply if the user sets DYLD_LIBRARY_PATH, right? Instead it could just say something like:
You must make libprotobuf.so dynamically available. You can either
install libprotobuf you built earlier, or set LD_LIBRARY_PATH
(DYLD_LIBRARY_PATH on OS X):
$ export LD_LIBRARY_PATH=../src/.libs
$ export DYLD_LIBRARY_PATH=../src/.libs # for OS X
or
$ (cd .. && make install)
There was a problem hiding this comment.
Nah, that caveat still applies because DYLD_LIBRARY_PATH is not even
consulted when the full path of the library is linked in. I tried every
variation I could think of, and this is the only thing that worked. If you
find another solution, I'm all ears, really.
On Apr 10, 2015 7:39 PM, "Joshua Haberman" notifications@github.com wrote:
In python/README.md
#276 (comment):
To build the C++ implementation run:$ python setup.py build --cpp_implementationTo build, test, and use the C++ implementation, you must first compilelibprotobuf.so:
$ (cd .. && make)
On OS X:I guess what I'm getting at is that it would be nice if the README didn't
have to have an entire separate section to address OS X, brew, etc. Your
caveat about Brew doesn't apply if the user sets DYLD_LIBRARY_PATH, right?
Instead it could just say something like:You must make libprotobuf.so dynamically available. You can either
install libprotobuf you built earlier, or set LD_LIBRARY_PATH
(DYLD_LIBRARY_PATH on OS X):$ export LD_LIBRARY_PATH=../src/.libs
$ export DYLD_LIBRARY_PATH=../src/.libs # for OS X
or
$ (cd .. && make install)—
Reply to this email directly or view it on GitHub
https://github.com/google/protobuf/pull/276/files#r28188273.
|
Looks like this is almost passing! It's just distcheck failing now, to fix that I think you just need to remove these lines? https://github.com/google/protobuf/blob/master/Makefile.am#L210 |
We already run all tests with and without `--cpp_implementation`
Use stdlib's 'unittest' instead.
|
Ah, nice. I'll push that change in about 2 hours, when I land.
|
|
Huzzah! Green. |
Migrate Python tests to stdlib unittest, drop apputils dependency.
|
Thanks! |
@haberman this is #165 rebased on #275 -- the tests pass!
closes #165.