Github user astitcher commented on the pull request:
https://github.com/apache/qpid/pull/7#issuecomment-101935319
This implementation doesn't follow the usual qpid portability strategy for
*not* introducing inline conditional compilation.
The way we do do this in qpid
Github user astitcher commented on the pull request:
https://github.com/apache/qpid/pull/7#issuecomment-170631466
As a way to implement clock_gettime() this seems like a good approach.
However the bigger problem is the timekeeping between clock_gettime() and
condition vars
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/95
Also, in general I'm not usually happy to check in generated (especially)
binary files to a source code repository.
I understand the purpose here, but it seems messy to me
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/95
In general though it's more important to get this into the repo so it can
be used going forward and then maybe it can be tidied up later.
---
If your project is set up for it, you can reply
Github user astitcher commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/95#discussion_r102975218
--- Diff: proton-c/src/tests/fuzz/fuzz-sniff-header.c ---
@@ -0,0 +1,15 @@
+#include
+#include
+
+#include "core/autodet
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/95
I'd be happy with just hex files, but the binary files are virtually
impossible to use sensibly without extra tools as a developer.
It might be a bit better with the fuzzing code
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/95
I really like the idea of putting fuzzing tests into the source tree
(guarded appropriately by cmake tests).
I'm not very keen about putting binary only files into the corpus. Could we
Github user astitcher commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/95#discussion_r102986492
--- Diff: proton-c/src/tests/fuzz/fuzz-sniff-header.c ---
@@ -0,0 +1,15 @@
+#include
+#include
+
+#include "core/autodet
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/95
Ok, if the function is *useful* without the fuzzer then it makes sense to
compile it without the fuzzer.
Sold.
---
If your project is set up for it, you can reply to this email
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/95
Assuming that any compatible fuzzer has to have the same interface
(assuming they all use LLVMFuzzerTestOneInput) then they all need to supply a
compatible definition.
It's
GitHub user astitcher opened a pull request:
https://github.com/apache/qpid-proton/pull/81
PROTON-1303: Replace go binding URL parser with version that doesn't â¦
This PR is specially for @alanconway to review.
You can merge this pull request into a Git repository by running
GitHub user astitcher opened a pull request:
https://github.com/apache/qpid-proton/pull/80
PROTON-1304: Changed OpenSSL interface code to not need time at all
This PR is specially for @kgiusti to review
- Used the openssl session client cache without having own cache that needs
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/81
@alanconway Did you note the change in meaning for a few URLs?
---
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
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/81
":1234" is not allowed by the go URL parser - that is what is rejecting it.
It gets rejected before our code has anything to do with it.
[That is because it is invalid as a
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/85
A couple of comments:
* It seems entirely arbitrary (to me anyway) to move generating encoding.h
and protocol.h from the C build process to generating them when producing the C
part
Github user astitcher closed the pull request at:
https://github.com/apache/qpid-proton/pull/87
---
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
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/87
@gemmellr I've been rebasing continually, so it's a surprise the dates are
so long ago - I'll reset them before commit.
Thanks for pointing this out.
---
If your project is set up
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/88
I like this approach.
I was expecting to see the file lists substituted in setup.py.in somewhere.
I didn't see that - how are the CMake file lists used inside setup.py to
generate
Github user astitcher commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/90#discussion_r88679501
--- Diff: proton-c/bindings/python/proton/reactor.py ---
@@ -565,7 +565,7 @@ def _connect(self, connection, reactor
GitHub user astitcher opened a pull request:
https://github.com/apache/qpid-proton/pull/87
Proton-c core split
This is mostly for comment by @alanconway and @cliffjansen.
It is a rearrangement of the Proton-c source tree, and an introduction of a
core OS and IO independent
Github user astitcher commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/88#discussion_r88113144
--- Diff: proton-c/bindings/python/CMakeLists.txt ---
@@ -122,3 +129,55 @@ install(TARGETS ${SWIG_MODULE_cproton_REAL_NAME}
COMPONENT
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/84
Is this against master?
I'm pretty sure @alanconway already fixed this by completely avoiding 8
byte atomics on 32 bit platforms.
The place where 64 bit atomics are used just
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/84
this should be fixed in SHA 76285f04e36cdbff68e98877cbc01bf4fb53 (which
should be in the 0.15 release).
---
If your project is set up for it, you can reply to this email and have your
GitHub user astitcher opened a pull request:
https://github.com/apache/qpid-proton/pull/83
[Go binding] Replaced c handler based flowcontroller with native go
- The go binding now has no dependency on the proton-c reactor code
@alanconway - could you take a look
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/83
@alanconway Thanks for pointing out the stupid Cism in my go code.
I've fixed it now.
I'd much porefer to check this iin with sone sort of test - any clue how I
should go
Github user astitcher closed the pull request at:
https://github.com/apache/qpid-proton/pull/83
---
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
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/91
This PR should be fixed by 79d06f07
---
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
Github user astitcher commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/93#discussion_r95459311
--- Diff: proton-c/bindings/python/proton/__init__.py ---
@@ -2581,6 +2581,9 @@ def close(self):
"""
se
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/92
Yep this looks like a mistake - I 'll pull it in.
---
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
Github user astitcher commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/99#discussion_r108212167
--- Diff: proton-c/src/core/transport.c ---
@@ -82,9 +82,11 @@ void pn_delivery_map_free(pn_delivery_map_t *db)
pn_free(db->deliver
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/99
Having looked a bit more it seems less clear that the bug is in pn_hash!
Essentially the lower 32 bit masking is used as a more determinate way to
cast the int32_t of the pn_sequence_t
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/99
Also I *strongly suggest* you test this code on both 32 and 64 bit machines
to make sure that the casting works equally well when uintptr_t is 32 vs 64
bits.
This is independent
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/101
@alanconway If the calls are documented to always return a value in the
string (even upon failure) I won't complain.
As a matter of API design though, a filing call should not alter
Github user astitcher commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/99#discussion_r108192064
--- Diff: proton-c/src/core/transport.c ---
@@ -1620,12 +1622,12 @@ static int pn_scan_error(pn_data_t *data,
pn_condition_t *condition, const char
Github user astitcher commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/99#discussion_r107256041
--- Diff: proton-c/src/core/transport.c ---
@@ -1648,7 +1659,7 @@ int pn_do_disposition(pn_transport_t *transport,
uint8_t frame_type, uint16_t ch
Github user astitcher commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/99#discussion_r107255885
--- Diff: proton-c/src/core/transport.c ---
@@ -1619,6 +1619,17 @@ static int pn_scan_error(pn_data_t *data,
pn_condition_t *condition, const char
Github user astitcher commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/99#discussion_r107257116
--- Diff: proton-c/src/core/transport.c ---
@@ -1619,6 +1619,17 @@ static int pn_scan_error(pn_data_t *data,
pn_condition_t *condition, const char
Github user astitcher commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/99#discussion_r107258218
--- Diff: proton-c/src/core/transport.c ---
@@ -1648,7 +1659,7 @@ int pn_do_disposition(pn_transport_t *transport,
uint8_t frame_type, uint16_t ch
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/95
Ah, sorry, I didn't read it through to the end (it is perhaps a little
wordy).
If the libs are interchangeable then which one is used need to be
selectable from CMake. I'll see if I
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/95
@jdanekrh I'm looking at this PR now. And I don't understand the purpose in
renaming libFuzzer -> libFuzzingEngine.
Could you explain why you do that.
---
If your project is set
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/98
Looks like a good idea to me
---
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
Github user astitcher commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/115#discussion_r132225370
--- Diff: proton-c/bindings/php/CMakeLists.txt ---
@@ -34,7 +34,7 @@ list(APPEND SWIG_MODULE_cproton_EXTRA_DEPS
${CMAKE_SOURCE_DIR}/proton-c
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/116
@alanconway you may want to take a look at this.
---
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
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/111
I applied this fix - that's why it's closed.
---
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
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/112
@matlo607 Sorry this change looks good, but it makes the regular CI build
fail because now it can't find the same binaries!
So Look at the Travis output above and try to figure out
Github user astitcher closed the pull request at:
https://github.com/apache/qpid-proton/pull/104
---
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
GitHub user astitcher opened a pull request:
https://github.com/apache/qpid-proton/pull/105
Port of C++ binding to Proton Proactor
This set of changes reimplements the C++ binding do that it no longer uses
the Proton Reactor library at all and now only links to the Proton Core
GitHub user astitcher opened a pull request:
https://github.com/apache/qpid-proton/pull/104
Port of thre C++ binding to the Proton Proactor library
This set of changes reimplements the C++ binding do that it no longer uses
the Proton Reactor library at all and now only links
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/108
@grs @rgodfrey @alanconway @ssorj You may want to take a look at this.
---
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
GitHub user astitcher opened a pull request:
https://github.com/apache/qpid-proton/pull/108
PROTON-1500: Implement plugin interface for SASL implementations
- Allow sasl implementation to be selected per connection
- Compile both default and cyrus implementation (if we can
Github user astitcher closed the pull request at:
https://github.com/apache/qpid-proton/pull/108
---
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
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/105
I've now rebased and slightly modified some the previous commits.
Substantively I've addressed the work_queue header guard issue; finished
the work dealing with scheduling work
Github user astitcher commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/105#discussion_r117554762
--- Diff: proton-c/bindings/cpp/include/proton/work_queue.hpp ---
@@ -0,0 +1,368 @@
+#ifndef PROTON_EVENT_LOOP_HPP
--- End diff
Github user astitcher commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/105#discussion_r117555738
--- Diff: proton-c/bindings/cpp/include/proton/message.hpp ---
@@ -329,11 +329,7 @@ class message {
mutable annotation_map
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/105
I've now updated this with what I intend to be the final rebase before
merging.
Everything is functionally there except for reconnect, which will be
addressed in the 0.19 timescale
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/126
Looks good to me
---
-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/119
I've merged this now (with an extra fix)
---
-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/119
Do you need me to merge these changes? (I forget if you are a committer or
not)
---
-
To unsubscribe, e-mail: dev
Github user astitcher commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/119#discussion_r137036761
--- Diff: proton-c/src/proactor/libuv.c ---
@@ -1179,7 +1179,10 @@ pn_proactor_t *pn_proactor() {
pn_proactor_t *p = (pn_proactor_t*)calloc(1
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/119
@michaelandrepearce can you paste in the JIRA report urls here then so they
can be connected.
---
-
To unsubscribe, e
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/119
@jdanekrh ok - I'll merge and commit this - also note we need a JIRA for
this too - it's not a simple typo (or something trivial) that doesn't need it -
I'll create one unless
Github user astitcher commented on the pull request:
https://github.com/apache/qpid-proton/commit/29272ca3948233a417ebc04ba80dedaefc7a4104#commitcomment-24146519
In CMakeLists.txt:
In CMakeLists.txt on line 177:
I agree with @jdanekrh - very many (most really) of the proton-c
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/120
Note the #pragma is portable in that compilers are required to ignore
pragmas they don't understand.
You can use _Pragma to use macros to expand to #pragma to make it more
platform
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/121
I'm not sure what the logic of this change is:
This parser API is not used internally (as far as I can tell) in proton-c
so removing the external API whilst leaving the internal code
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/120
This is the CMake generated version of the deprecation macros:
```
# ifndef PN_DEPRECATED
#if defined(PN_COMPILER_CXX_ATTRIBUTE_DEPRECATED
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/120
I'm not sure that it's actually worth putting out deprecation messages for
including the files but not using the deprecated APIs there.
If the APIs aren't used it will be trivial
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/121
Note the build failures are from internal code including proton/parser.h.
So even though I didn't find anything internally using the parser maybe my
code browser isn't as good as I
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/118
I'm assuming you are proposing to check this in branching from the 0.17
release? (It's difficult to tell from the PR). Note that the CI builds both
fail - you will want to check this isn't
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/121
looks good.
---
-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/120
Now that I've merged the work_queue/work API changes I think you should
deprecate the use of void_function0 and the work_queue/container APIs that use
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/120
Otherwise this looks about right to me.
---
-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional
Github user astitcher commented on the pull request:
https://github.com/apache/qpid-proton/commit/de3fd617210b5d5a2f2c3e384c33905dbf75ad58#commitcomment-25557336
i can see why you might think the way you do. But actually this is not
correct.
You are assuming that the final
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/128
A few points -
* I'm not sure how familiar you are with git/github: but there seem to be a
lot of fairly meaningless changes here. Could you squash all of these changes
into a single
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/128
I like this latest version, but 2 remaining things:
* Please, please rebase and squash this to be a single commit - this chain
of development makes no sense as part of the project
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/131
@RoddieKieley I've fixed this issue now - can you close the PR (stupidly I
forgot to add that to the commit
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/131
The tests should still pass if SSL_IMPL is none as there is a stub
implementation. If not then we need to fix that bug
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/131
Oops - I guess I did that when I added in the ssl tests.
---
-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/118
@kgiusti I think this PR is now irrelevant - as 0.18 is out and supports
openssl 1.0 and 1.1.
Could you close
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/144
Pushed a new version of these changes according to feedback from @ssorj
---
-
To unsubscribe, e-mail: dev-unsubscr
GitHub user astitcher opened a pull request:
https://github.com/apache/qpid-proton/pull/144
Python proactor
This is a fairly large change to the structure of the Proton python binding.
* Initially it removes support for really old versions of python - anything
before 2.5
GitHub user astitcher opened a pull request:
https://github.com/apache/qpid-proton/pull/148
PROTON-1354: Don't allow SASL mechanisms GSSAPI or GSS-SPNEGO by default
Small change that fixes a pain point if you have kerberos on you client
machine but aren't using it for proton
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/146
Ok - I see this is dependant on @alanconway 's recent work.
---
-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/146
I'm a little confused by this change - what creates the
libqpid-proton-static lib that this changes uses? It doesn't seem that this
changes creates that library anywhere
Github user astitcher commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/135#discussion_r159936304
--- Diff: .travis.yml ---
@@ -23,29 +23,37 @@ compiler:
- gcc
- clang
+env:
+- PIP='pip'
+
matrix:
include
Github user astitcher commented on a diff in the pull request:
https://github.com/apache/qpid-proton/pull/135#discussion_r160062571
--- Diff: .travis.yml ---
@@ -23,29 +23,37 @@ compiler:
- gcc
- clang
+env:
+- PIP='pip'
+
matrix:
include
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/135
Hmm, not quite sure about this:
* The actual change seems fine, although ISTR putting in an explicit
turning off for PERL for apple builds some years ago, so I'm surprised you
needed
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/132
Can I suggest breaking this down into 3 separate changes (PRs branches
etc.):
* Removal of deprecated bindings
* Removal of obsolete doc
* Moving the remainder to new locations
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/134
Looks good to me
---
-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/135
Maybe I'm the one who misunderstands how env and matrix interact then.
Only one other thing I might suggest is to just use pip2 everywhere without
a variable at all - It looks like
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/135
But failing that I'm happy with what's there now.
---
-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/154
This looks fine, but you have ignored the c code in the tree - is there a
reason for that?
---
-
To unsubscribe, e-mail
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/154
That's curious: When I do the same against upstream master I find WIN32
used in `c/include/proton/import_export.h`. Are you checking against the
upstream master?
I did think it was used
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/112
@matlo607 I think this may be fixed with change
b172262577a79d8764b9fe06e27f71663446c309. If you have a chance can you check
and close this PR if it is fixed
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/152
This looks good - I'll pull it in.
I wonder though if there is scope for some refactoring of the ssl error
logic to make this error logic in common
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/139
This seems like an entirely unnecessary change - the Travis build job
already only uses python2 - why do we need to change it?
Homebrew building has no relation with Travis building does
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/95
I've now committed a slightly modified version of this PR. So it can be
closed.
---
-
To unsubscribe, e-mail: dev-unsubscr
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/161
Another thought - is there any way to pin the python version using brew?
That would make the CI build instructions more robust
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/161
This looks good to me - it makes me wonder if we could do the same for the
Xcode 9 build too.
---
-
To unsubscribe, e
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/157
Basically don't like this approach because it exposes implementation
details to the library interface.
As the unit tests don't only need to be built in the build environment.
Myself
Github user astitcher commented on the issue:
https://github.com/apache/qpid-proton/pull/157
Honestly given the alternative, adding gets for the options seems fairly
innocuous!
To be (a little) less flippant Unit testing is actually a good reason to
make the options readable
1 - 100 of 103 matches
Mail list logo
