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

PROTON-1224: Upgrade BouncyCastle #75

Closed
wants to merge 15 commits into from
Closed

PROTON-1224: Upgrade BouncyCastle #75

wants to merge 15 commits into from

Conversation

JemDay
Copy link
Contributor

@JemDay JemDay commented Jun 10, 2016

Support 1.48+ of BouncyCastle
Added unit-test and associated certificate and key test resources.

@@ -99,7 +99,7 @@ mvn test \
<dependency>
<groupId>org.bouncycastle</groupId>
<artifactId>bcpkix-jdk15on</artifactId>
<version>1.47</version>
<version>1.48</version>
Copy link
Member

Choose a reason for hiding this comment

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

Given the idea is to update it probably makes sense to go for the latest version rather than this 3+ year old version (same goes for the other instance).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do..

@gemmellr
Copy link
Member

I still need to have a final look, but if you could address the couple niggles I commented on that would be good. As I mentioned before, if you could also please remove merge commits from PRs that would be good, you have added a second in this version. Feel free to rebase the PR and squash all the commits, you don't need to open a new PR.

@JemDay
Copy link
Contributor Author

JemDay commented Jun 16, 2016

Robbie - thanks for reviewing and sorry for the slow response, i'll look to try and resolve these today.

As previously discussed i'm not an enormous fan of this reflective approach but the cmake build process for java floored me a bit w.r.t how to achieve the maven style dependency at compile time.

@JemDay
Copy link
Contributor Author

JemDay commented Jun 17, 2016

OK ... my lack of high-wire skills with GIT is really showing through now ... i'm completely stumped as to how to 'not' include other commits when pushing back to my fork ..

I've tried squashing but always end up with merge conflicts with the Fork

This is why they don't typically let architects edit code ;-)

astitcher and others added 8 commits June 17, 2016 13:05
The test was comparing exact error message text, changed by previous fix to
include the transport error. Modified to check for a reliable substring.
container::schedule() with simple example of sending messages at a fixed interval.
Examples for C++03 and C++11.
Modified inject() to use the same proton::void_function0 as schedule for C++03.

Note: the example chains schedule() calls at a fixed interval. A precise
fixed-frequency sender should take account of the actual time to correct for
variations.
- Make all member functions in container class pure virtual (except destructor)
- Add a new standard_container class that has the standard behaviour
  of the variant overloads
- Make all container implementations use standard_container class
- Remove unnecessary includes of container_impl
…is bound

Previously the reactor would be put on the transport's attachment list
when a socket selector was created for the transport.  If the socket
setup fails then no selector is created and the reactor is never added
to the transport.  This would result in a crash as the code depends on
the reactor being present in the transport when transport events are
generated.
@JemDay
Copy link
Contributor Author

JemDay commented Jun 17, 2016

i think this is royally screwed up now, the file-change list looks clean but using a rebase rather than a pull (to remove the merge commits) now appears to be having an even worse affect.

I'm open to suggestions - i can close this PR, trash my fork, and then re-create the change if that's going to be a simpler path forward but before that i would like to better understand the fork/pull/commit/push/PR cycle when there may be multiple commits to the fork before the PR is actually raised. Squashing after commits have been pushed seems to just open world of hurt

@gemmellr
Copy link
Member

you just need to ensure you have the upstream repo up to date, then rebase your fork against it, then [force] push to your fork, which should result in your commit(s) sitting at the head of the branch with the current upstream commits preceding them and no unnecessary merge commits. You can repeat that process as many times as you like.

You obviously want to avoid force pushes on branches you are using to directly collaborate with other people on, but typically you commit work on a specific branch for the PR and then since noone else is likely to be depending on it doing such a force push is of little consequence to anyone.

@JemDay
Copy link
Contributor Author

JemDay commented Aug 22, 2016

I would like to close this PR and open a new one with (hopefully) a much cleaner change delta ...

@JemDay
Copy link
Contributor Author

JemDay commented Aug 22, 2016

Closing for re-creation

@JemDay JemDay closed this Aug 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants