[jira] [Commented] (PROTON-895) Rely on python to build the downloaded tarball
[ https://issues.apache.org/jira/browse/PROTON-895?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14574519#comment-14574519 ] ASF subversion and git services commented on PROTON-895: Commit 3db3cf1783f1b91211028842389801316d10b8ba in qpid-proton's branch refs/heads/master from [~flaper87] [ https://git-wip-us.apache.org/repos/asf?p=qpid-proton.git;h=3db3cf1 ] PROTON-895: Rely on python to build downloaded tarball Instead of using cmake to build the downloaded tarball, rely on python for building and installing the library. This process is not only cleaner but also plays nicer with the environment we're trying to give support to. In addition to the above, this plays nicer with the user and removes a requirement for having the library installed. Note that the built library will no longer overwrite system libraries but it'll be placed along to the python bindings we're building. (cherry picked from commit f2e97708a9c0da049bbc5089909fc31ab092edf8) Rely on python to build the downloaded tarball -- Key: PROTON-895 URL: https://issues.apache.org/jira/browse/PROTON-895 Project: Qpid Proton Issue Type: Bug Reporter: Flavio Percoco Assignee: Ken Giusti Attachments: 0001-Rely-on-python-to-build-downloaded-tarball.patch Recently, a patch that made python-qpid-proton's setup.py download proton-c and build it whenever it's not found in the system. The patch relied on cmake to build the library as everyone would do when building proton-c. While that works, it's not the right, most pythonic, reliable way to do it. Some reasons why it's not a good thing: 1. It might overwrite a system qpid install if there's one and if the python-qpid-proton library is being installed in the system 2. It requires users - including CI systems, etc - to have cmake installed. 3. It does everything from outside the Python mechanism. The patch proposed in this bug changes the current behavior in favor of using Python's build extensions to compile the library. The patch follows the same steps as you'd do with cmake and it does it *just* for the downloaded tarball. If there's an installed proton-c library, there's nothing to do. If you're building it using cmake from a proton-c dir, it'll keep using cmake normally. The built library doesn't have the same name as the global one and it's installed along with the python binding instead of installing header files and the library itself system wide. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[GitHub] qpid-proton pull request: C++ binding for proton.
Github user alanconway commented on a diff in the pull request: https://github.com/apache/qpid-proton/pull/35#discussion_r31842854 --- Diff: proton-c/bindings/cpp/README.md --- @@ -0,0 +1,24 @@ +# C++ binding for proton. + +This is a C++ wrapper for the proton reactor API. --- End diff -- Agreed, fixed. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] qpid-proton pull request: C++ binding for proton.
Github user alanconway commented on a diff in the pull request: https://github.com/apache/qpid-proton/pull/35#discussion_r31840954 --- Diff: examples/cpp/example_test.py --- @@ -0,0 +1,105 @@ +# --- End diff -- No, it is a test script that runs the C++ examples and verifies they work. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[jira] [Created] (PROTON-901) No constants defined for Termins.expiry_policy
Gordon Sim created PROTON-901: - Summary: No constants defined for Termins.expiry_policy Key: PROTON-901 URL: https://issues.apache.org/jira/browse/PROTON-901 Project: Qpid Proton Issue Type: Bug Components: python-binding Affects Versions: 0.9, 0.10 Reporter: Gordon Sim Assignee: Gordon Sim Priority: Minor Fix For: 0.10 The other c enumerations have python constants defined for them, but not pn_expiry_policy_t. This means applications either have to use magic numbers based on reading the .h or import from cproton which is ugly. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[GitHub] qpid-proton pull request: C++ binding for proton.
Github user astitcher commented on a diff in the pull request: https://github.com/apache/qpid-proton/pull/35#discussion_r31834366 --- Diff: proton-c/bindings/CMakeLists.txt --- @@ -109,6 +109,11 @@ if (EMSCRIPTEN_FOUND) set (DEFAULT_JAVASCRIPT ON) endif (EMSCRIPTEN_FOUND) +# Prerequisites for C++ --- End diff -- I don't understand what this block is doing --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] qpid-proton pull request: C++ binding for proton.
Github user alanconway commented on a diff in the pull request: https://github.com/apache/qpid-proton/pull/35#discussion_r31842778 --- Diff: proton-c/bindings/CMakeLists.txt --- @@ -109,6 +109,11 @@ if (EMSCRIPTEN_FOUND) set (DEFAULT_JAVASCRIPT ON) endif (EMSCRIPTEN_FOUND) +# Prerequisites for C++ --- End diff -- There is a foreach at the end of the file that sets up all the user-settable binding options. For each binding you must set a DEFAULT_FOO to be on or off depending if the requirements to build the binding are available, then the foreach sets up the BUILD_FOO option. CMAKE_CXX_COMPILER is set if a C++ compiler is available. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[jira] [Commented] (PROTON-901) No constants defined for Termins.expiry_policy
[ https://issues.apache.org/jira/browse/PROTON-901?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14575095#comment-14575095 ] ASF subversion and git services commented on PROTON-901: Commit 5a6c8da659c7e052e86591a3522e71b2e9e0fda8 in qpid-proton's branch refs/heads/master from [~gsim] [ https://git-wip-us.apache.org/repos/asf?p=qpid-proton.git;h=5a6c8da ] PROTON-901: add constants in python for pn_expiry_policy_t values No constants defined for Termins.expiry_policy -- Key: PROTON-901 URL: https://issues.apache.org/jira/browse/PROTON-901 Project: Qpid Proton Issue Type: Bug Components: python-binding Affects Versions: 0.9, 0.10 Reporter: Gordon Sim Assignee: Gordon Sim Priority: Minor Fix For: 0.10 The other c enumerations have python constants defined for them, but not pn_expiry_policy_t. This means applications either have to use magic numbers based on reading the .h or import from cproton which is ugly. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Resolved] (PROTON-901) No constants defined for Termins.expiry_policy
[ https://issues.apache.org/jira/browse/PROTON-901?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Gordon Sim resolved PROTON-901. --- Resolution: Fixed No constants defined for Termins.expiry_policy -- Key: PROTON-901 URL: https://issues.apache.org/jira/browse/PROTON-901 Project: Qpid Proton Issue Type: Bug Components: python-binding Affects Versions: 0.9, 0.10 Reporter: Gordon Sim Assignee: Gordon Sim Priority: Minor Fix For: 0.10 The other c enumerations have python constants defined for them, but not pn_expiry_policy_t. This means applications either have to use magic numbers based on reading the .h or import from cproton which is ugly. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (PROTON-895) Rely on python to build the downloaded tarball
[ https://issues.apache.org/jira/browse/PROTON-895?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14575172#comment-14575172 ] ASF subversion and git services commented on PROTON-895: Commit 801052159a18c6e68256dcfb9a9eca8a48b6732c in qpid-proton's branch refs/heads/0.9.x from [~flaper87] [ https://git-wip-us.apache.org/repos/asf?p=qpid-proton.git;h=8010521 ] PROTON-895: Rely on python to build downloaded tarball Instead of using cmake to build the downloaded tarball, rely on python for building and installing the library. This process is not only cleaner but also plays nicer with the environment we're trying to give support to. In addition to the above, this plays nicer with the user and removes a requirement for having the library installed. Note that the built library will no longer overwrite system libraries but it'll be placed along to the python bindings we're building. (cherry picked from commit 4ed130ebd051a64b6aaf99c4adfdc0f7a56b4707) Rely on python to build the downloaded tarball -- Key: PROTON-895 URL: https://issues.apache.org/jira/browse/PROTON-895 Project: Qpid Proton Issue Type: Bug Reporter: Flavio Percoco Assignee: Ken Giusti Attachments: 0001-Rely-on-python-to-build-downloaded-tarball.patch Recently, a patch that made python-qpid-proton's setup.py download proton-c and build it whenever it's not found in the system. The patch relied on cmake to build the library as everyone would do when building proton-c. While that works, it's not the right, most pythonic, reliable way to do it. Some reasons why it's not a good thing: 1. It might overwrite a system qpid install if there's one and if the python-qpid-proton library is being installed in the system 2. It requires users - including CI systems, etc - to have cmake installed. 3. It does everything from outside the Python mechanism. The patch proposed in this bug changes the current behavior in favor of using Python's build extensions to compile the library. The patch follows the same steps as you'd do with cmake and it does it *just* for the downloaded tarball. If there's an installed proton-c library, there's nothing to do. If you're building it using cmake from a proton-c dir, it'll keep using cmake normally. The built library doesn't have the same name as the global one and it's installed along with the python binding instead of installing header files and the library itself system wide. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (PROTON-865) C++ reactor client binding
[ https://issues.apache.org/jira/browse/PROTON-865?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14575077#comment-14575077 ] ASF subversion and git services commented on PROTON-865: Commit 7bca703a9daf4a37f93e0b748cd1bcc8b62b6f6b in qpid-proton's branch refs/heads/cjansen-cpp-client from [~aconway] [ https://git-wip-us.apache.org/repos/asf?p=qpid-proton.git;h=7bca703 ] PROTON-865: Remove .go examples added in error. C++ reactor client binding -- Key: PROTON-865 URL: https://issues.apache.org/jira/browse/PROTON-865 Project: Qpid Proton Issue Type: New Feature Environment: C++ Reporter: Cliff Jansen Assignee: Cliff Jansen This JIRA tracks initial work on a C++ binding to the Proton reactor event based model with an eye to providing an API very similar to the python client. As stated on the Proton list, the broad goals are: to make it easy to write simple programs natural to build out more complicated ones very similar API to the Python client lean and performant The initial introduction with accompanying HelloWorld can be found at https://github.com/apache/qpid-proton/pull/18 Ongoing work is proceeding in my github account in branch cpp-work https://github.com/cliffjansen/qpid-proton/tree/cpp-work commit 1453f57ca3f446450ef654505548f1947cb7a0f1 adds listener support, exceptions and a logging interface. The initial implementation will provide no thread safety guarantees, but the bone structure should allow that to be added later with no API change. I still hold out hope of eventually allowing multiple threads processing separate connections concurrently. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[GitHub] qpid-proton pull request: C++ binding for proton.
Github user astitcher commented on a diff in the pull request: https://github.com/apache/qpid-proton/pull/35#discussion_r31844356 --- Diff: CMakeLists.txt --- @@ -20,16 +20,15 @@ cmake_minimum_required (VERSION 2.6) project (Proton C) +# Enable C++ now for examples and bindings subdirectories, but make it optional. +enable_language(CXX OPTIONAL) + --- End diff -- You are correct. I think that we could now move the block below somewhere else - it was only here to get the project command in the top level file. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] qpid-proton pull request: C++ binding for proton.
Github user astitcher commented on the pull request:
https://github.com/apache/qpid-proton/pull/35#issuecomment-109421363
Another general point (I can't exactly see where to attach it in the
changes):
As well as using a class hierarchy to get callbacks, we should think about
using functions, as C++11 has a nice lambda syntax and std::function too.
Well when I say nice lambda syntax I mean succinct and reasonably readable
syntax. `[](){}` anyone?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: C++ binding for proton.
Github user astitcher commented on a diff in the pull request: https://github.com/apache/qpid-proton/pull/35#discussion_r31844888 --- Diff: proton-c/bindings/CMakeLists.txt --- @@ -109,6 +109,11 @@ if (EMSCRIPTEN_FOUND) set (DEFAULT_JAVASCRIPT ON) endif (EMSCRIPTEN_FOUND) +# Prerequisites for C++ --- End diff -- [Of course I should have known that, as I wrote that foreach logic!] --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] qpid-proton pull request: C++ binding for proton.
Github user astitcher commented on the pull request: https://github.com/apache/qpid-proton/pull/35#issuecomment-109423155 There are a number of places in the API where the API has something like: `void setXXX(const char* bytes, size_t size);` this isn't very C++ like - in fact it adds very little value over the original API whilst still adding an extra layer wrapping it. If we are going to add a wrapper like this we should be making it more C++ like either using `void (const std::string);` or `void (const std::vectorchar);` (one or other or both depending on the context). This is equally true for getters as setters, although for these we might need to create some wrapping types to avoid copying if the value returned is a pointer into an existing value. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] qpid-proton pull request: C++ binding for proton.
Github user alanconway commented on the pull request: https://github.com/apache/qpid-proton/pull/35#issuecomment-109426860 On Fri, 2015-06-05 at 13:05 -0700, Andrew Stitcher wrote: There are a number of places in the API where the API has something like: void setXXX(const char* bytes, size_t size); this isn't very C++ like - in fact it adds very little value over the original API whilst still adding an extra layer wrapping it. If we are going to add a wrapper like this we should be making it more C++ like either using void (const std::string); or void (const std::vectorchar); (one or other or both depending on the context). This is equally true for getters as setters, although for these we might need to create some wrapping types to avoid copying if the value returned is a pointer into an existing value. Agreed. I would be inclined to do std::string in the C++ API and provide access to the underlying pn_foo_t* for edge cases. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] qpid-proton pull request: C++ binding for proton.
Github user astitcher commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/35#discussion_r31844585
--- Diff: proton-c/bindings/cpp/include/proton/cpp/Acceptor.h ---
@@ -0,0 +1,50 @@
+#ifndef PROTON_CPP_ACCEPTOR_H
+#define PROTON_CPP_ACCEPTOR_H
+
+/*
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * License); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * AS IS BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+#include proton/cpp/ImportExport.h
+#include proton/cpp/ProtonHandle.h
+#include proton/reactor.h
+
+struct pn_connection_t;
+
+namespace proton {
+namespace reactor {
+
+class Acceptor : public ProtonHandlepn_acceptor_t
+{
+ public:
+PROTON_CPP_EXTERN Acceptor();
+PROTON_CPP_EXTERN Acceptor(pn_acceptor_t *);
+PROTON_CPP_EXTERN Acceptor(const Acceptor);
--- End diff --
Not quite - that syntax tells the compiler to definitely generate the
default implementation. It won't always otherwise, subject to irritating ules
that might change as they are a little contraversial
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: C++ binding for proton.
Github user alanconway commented on the pull request:
https://github.com/apache/qpid-proton/pull/35#issuecomment-109412800
On Fri, 2015-06-05 at 11:13 -0700, Andrew Stitcher wrote:
Some overall comments (from a not yet thorough) read through:
I really don't like using set/get in a C++ API. It's not idiomatic
C++ API style; it's not necessary; and it doesn't read as well (IMO)
So instead of
Proton::transport::set('blah')/Proton::transport::get(), can
we please use
Proton::transport::('blah')/Proton::transport::().
We use idiomatic properties in Python we should use them in C++ too.
100% agree, will fix. Those get/sets in Qpid have annoyed me for years.
I'm not sure that we really need to use the Pimpl pattern for this
binding. The real need for a Pimpl is to insulate this API from
changes to code that it depends on - in this case once the C API is
stable then its own API stability requirements should ensure that
nothing gets broken in the C++ API too. I may not have thought this
through hard enough though [is that a sentence with too many 'ough's
or what ;-) ]
This will save the indirection and extra resource use that Pimpl
brings, which would force 2 allocations for each of the structures.
Agreed. Cliff also raised this question. He added some extra state to
those classes to make things more user-friendly, but I think we should
push such helper functionality to separate classes and keep a layer
that is simply a C++ wrapper of a C pointer.
This strikes me as important because the C++ API can be as efficient
as the C API if we do this work well, and it will be much easier to
write and read code in good C++ than C. So I think that no one should
need to do any application work in C even for applications that need
the full efficiency of low level control of the library.
Yup, agreed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: C++ binding for proton.
Github user alanconway commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/35#discussion_r31844238
--- Diff: proton-c/bindings/cpp/include/proton/cpp/Acceptor.h ---
@@ -0,0 +1,50 @@
+#ifndef PROTON_CPP_ACCEPTOR_H
+#define PROTON_CPP_ACCEPTOR_H
+
+/*
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * License); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * AS IS BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+#include proton/cpp/ImportExport.h
+#include proton/cpp/ProtonHandle.h
+#include proton/reactor.h
+
+struct pn_connection_t;
+
+namespace proton {
+namespace reactor {
+
+class Acceptor : public ProtonHandlepn_acceptor_t
+{
+ public:
+PROTON_CPP_EXTERN Acceptor();
+PROTON_CPP_EXTERN Acceptor(pn_acceptor_t *);
+PROTON_CPP_EXTERN Acceptor(const Acceptor);
--- End diff --
C++ is unbelievable. You have a default so you don't need to constantly
re-type repetitive completely uninformative code, and now we have a special
syntax so you can type something to show that you know you don't have to type
it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[jira] [Updated] (PROTON-901) No constants defined for Terminus.expiry_policy
[ https://issues.apache.org/jira/browse/PROTON-901?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Gordon Sim updated PROTON-901: -- Summary: No constants defined for Terminus.expiry_policy (was: No constants defined for Termins.expiry_policy) No constants defined for Terminus.expiry_policy --- Key: PROTON-901 URL: https://issues.apache.org/jira/browse/PROTON-901 Project: Qpid Proton Issue Type: Bug Components: python-binding Affects Versions: 0.9, 0.10 Reporter: Gordon Sim Assignee: Gordon Sim Priority: Minor Fix For: 0.10 The other c enumerations have python constants defined for them, but not pn_expiry_policy_t. This means applications either have to use magic numbers based on reading the .h or import from cproton which is ugly. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
C++ versions for the C++ binding
I think this has been discussed before, but is there a consensus on what C++ version we want to target for the C++ binding? C++03 is old and smelly. C++11 is better, C++14 better still and afaik any compiler that does 11 will do 14. So the sensible choices would seem to be: 1. Smelly C++03 code only 2. C++03 compatible with #ifdef optional bits of C++14 niceness. 3. C++14 first, add C++03 later if somebody whines about it. 3. is the most fun for me obviously.
[GitHub] qpid-proton pull request: C++ binding for proton.
Github user alanconway commented on the pull request: https://github.com/apache/qpid-proton/pull/35#issuecomment-109413137 On Fri, 2015-06-05 at 10:37 -0700, Andrew Stitcher wrote: There seem to be a couple of extraneous go example files that have snuck into this PR â Ooops, removed. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
