Skip to content
This repository has been archived by the owner on Sep 1, 2023. It is now read-only.

update capnp to 0.5.3.1 #1322

Closed
wants to merge 2 commits into from
Closed

update capnp to 0.5.3.1 #1322

wants to merge 2 commits into from

Conversation

breznak
Copy link
Member

@breznak breznak commented May 24, 2017

which includes fix for clang 4.x support,
matches the pycapnp version used.

Fixes #1308
Replaces #1318

which includes fix for clang 4.x support,
matches the pycapnp version used.
@rhyolight
Copy link
Member

We're going backwards with the version?

@breznak
Copy link
Member Author

breznak commented May 24, 2017

Why would you think so? :)

ea9fff1#diff-5a4f0c419aafa86b9781d93f30964421R49

@rhyolight
Copy link
Member

rhyolight commented May 24, 2017

Never mind, at first read I thought we were going from 0.5.3 to 0.5.1. Now I see that it is 0.5.3.1.

@vitaly-krugl
Copy link
Member

vitaly-krugl commented May 24, 2017

@breznak: FYI - he nupic.core/nupic.bindings compilation no longer relies on pycapnp at all. pycapnp is only used by some of the Python tests in nupic.core/bindings/py/tests. No action required - just FYI.

@rhyolight
Copy link
Member

@vitaly-krugl do you approve this PR?

Copy link
Member

@vitaly-krugl vitaly-krugl left a comment

Choose a reason for hiding this comment

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

@breznak, thank you for your work. Please address feedback concerning pycapnp and spelling of external/CapnProto.cmake in README.

Otherwise, it looks good to me.

capnproto-c++-win32-0.5.3.zip: from https://capnproto.org/capnproto-c++-win32-0.5.3.1.zip

When pycapnp (in requirements.txt) is updated, please check with which version of capnproto it ships and update the files also here
and in external/Capnproto.cmake !
Copy link
Member

Choose a reason for hiding this comment

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

@breznak, why does it matter if pycapnp's capnproto version matches capnproto version here? See additional info in the paragraph below. Also, there might be different reasons for updating pycapnp that would not necessitate update of nupic.core's resident capnproto. If there is still a real reason, please add a comment in the README explaining it. Otherwise, let's remove the reference to pycapnp from this README. The comment describing coordination between the zip names here and CapnProto.cmake is good.

We now pass serialization data via data buffers instead of trying to pass capnproto objects back and forth between potentially-incompatible builds of capnproto in python and nupic.bindings (there used to be ABI issues, incompatible symbol preemption, etc.)

Copy link
Member Author

Choose a reason for hiding this comment

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

why does it matter if pycapnp's capnproto version matches capnproto version here?

Sorry, I'm back from holiday, so I may have lost context in here, but the issue with this started from :
capnproto/pycapnp#152 (comment)

So I think the incompatibility was the compiler picked up the (older) capnp version shipped in nupic.core, while newer (by only a x.y.z.1) version was required. I would ask otherwise, do we need this custom installed capnp binary, could we just use the one shipped as dependency of pycapnp?

TL:DR: clang 4.x breaks when these two capnp versions are not in sync, and it isn't documented in nupic.core readmes

Copy link
Member

Choose a reason for hiding this comment

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

clang 4.x breaks when these two capnp versions are not in sync, and it isn't documented in nupic.core readmes

@breznak, you should be able to build nupic.core without having pycapnp on your system at all. The nupic.core master branch build should only depend on headers and from capnproto that is bundled inside nupic.core (https://github.com/numenta/nupic.core/tree/master/external/common/share/capnproto) and link with archives produced from the same.

Could you please look into why the cmake build breaks with clang 4.x? Perhaps there is some unintentional reference to pycapnp (headers?) accidentally left in nupic.core's cmake configuration that should be removed.

Also, we cannot depend on the capnp .so that is shipped with pycapnp for nupic.core builds, because we have no control over which version of pycapnp (and by extension capnp .so) the user has on their system. The APIs might be incompatible, etc. Also, we cannot use the .so, because we run into duplicate schema ID issue related to how capnproto registers templated schema representations of same schema that is built/used by more than one extension. Capnproto buffer passing was intended specifically to decouple the capnp used by the python layer and the capnp used by the C++ extensions in nupic.bindings and eliminate the various compatibility issues that we faced.

capnproto-c++-win32-0.5.3.zip: from https://capnproto.org/capnproto-c++-win32-0.5.3.1.zip

When pycapnp (in requirements.txt) is updated, please check with which version of capnproto it ships and update the files also here
and in external/Capnproto.cmake !
Copy link
Member

Choose a reason for hiding this comment

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

Spelling - use correct case external/Capnproto.cmake ==> external/CapnProto.cmake

@breznak
Copy link
Member Author

breznak commented Jun 8, 2017

@vitaly-krugl Thank you for review! And sorry for the delay. Fixed and commented on your question.

@breznak
Copy link
Member Author

breznak commented Jan 5, 2018

Apparently even newer version is currently used, closing as obsoleted.

@breznak breznak closed this Jan 5, 2018
@breznak breznak deleted the update_capnp branch January 5, 2018 00:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix compilation with clang 4.x compiler - capnp error
3 participants