Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop apputils dependency #276

Merged
merged 5 commits into from
Apr 11, 2015
Merged

Drop apputils dependency #276

merged 5 commits into from
Apr 11, 2015

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Apr 8, 2015

@haberman this is #165 rebased on #275 -- the tests pass!

closes #165.

@tamird
Copy link
Contributor Author

tamird commented Apr 9, 2015

@haberman green :)

@haberman
Copy link
Member

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

@tamird
Copy link
Contributor Author

tamird commented Apr 10, 2015

@haberman yep, already done. Only trouble is they're not passing for the moment :(

@tamird
Copy link
Contributor Author

tamird commented Apr 10, 2015

@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 python, causing double-test-run errors and of course testImplementationSetting itself to fail. Any ideas how to force this to reload?

@haberman
Copy link
Member

Oh did you figure out the other weird errors related to duplicate symbols in .proto files?


$ (cd .. && make)

On OS X:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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" [email protected] wrote:

In python/README.md
#276 (comment):

  • To build the C++ implementation run:
    
  • $ python setup.py build --cpp_implementation
    
  • To build, test, and use the C++ implementation, you must first compile
    
  • libprotobuf.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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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" [email protected] wrote:

In python/README.md
#276 (comment):

  • To build the C++ implementation run:
    
  • $ python setup.py build --cpp_implementation
    
  • To build, test, and use the C++ implementation, you must first compile
    
  • libprotobuf.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.

@haberman
Copy link
Member

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
https://github.com/google/protobuf/blob/master/Makefile.am#L217
https://github.com/google/protobuf/blob/master/Makefile.am#L220

We already run all tests with and without `--cpp_implementation`
Use stdlib's 'unittest' instead.
@tamird
Copy link
Contributor Author

tamird commented Apr 10, 2015

Ah, nice. I'll push that change in about 2 hours, when I land.
On Apr 10, 2015 7:41 PM, "Joshua Haberman" [email protected] wrote:

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
https://github.com/google/protobuf/blob/master/Makefile.am#L217
https://github.com/google/protobuf/blob/master/Makefile.am#L220


Reply to this email directly or view it on GitHub
#276 (comment).

@tamird
Copy link
Contributor Author

tamird commented Apr 11, 2015

Huzzah! Green.

haberman added a commit that referenced this pull request Apr 11, 2015
Migrate Python tests to stdlib unittest, drop apputils dependency.
@haberman haberman merged commit be89e62 into protocolbuffers:master Apr 11, 2015
@haberman
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants