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

common: fix building against libcryptopp #14949

Merged
merged 1 commit into from May 4, 2017

Conversation

Projects
None yet
2 participants
@zhsj
Contributor

zhsj commented May 4, 2017

libnspr is only needed for libnss, move it to if defined(USE_NSS)
section.

ceph::crypto::shutdown's params should be same with declaration.

Signed-off-by: Shengjing Zhu zhsj@umcloud.com

@@ -28,7 +27,7 @@ void ceph::crypto::init(CephContext *cct)
{
}
void ceph::crypto::shutdown()
void ceph::crypto::shutdown(bool shared)

This comment has been minimized.

@tchaikov

tchaikov May 4, 2017

Contributor

nit, remove shared if it's not used.

This comment has been minimized.

@zhsj

zhsj May 4, 2017

Contributor

It's needed since other functions will call it with parameter.
https://github.com/ceph/ceph/blob/master/src/common/ceph_context.cc#L663

This comment has been minimized.

@tchaikov

tchaikov May 4, 2017

Contributor

@zhsj what i meant is:

void ceph::crypto::shutdown(bool)
@tchaikov

lgtm modulo the nit.

@@ -27,7 +27,7 @@ void ceph::crypto::init(CephContext *cct)
{
}
void ceph::crypto::shutdown(bool shared)
void ceph::crypto::shutdown(bool)

This comment has been minimized.

@tchaikov

tchaikov May 4, 2017

Contributor

please fold this commit into the previous one.

This comment has been minimized.

@zhsj

zhsj May 4, 2017

Contributor

Sorry for habits from some community which force push in pr not allowed :)

This comment has been minimized.

@tchaikov

tchaikov May 4, 2017

Contributor

@zhsj i'd be surprised if a community enforce this on PR! because i think this is how it is supposed to be updated.

This comment has been minimized.

@zhsj

zhsj May 4, 2017

Contributor

Yes, CPython enforces this since the core dev will do the squash when merging.
http://cpython-devguide.readthedocs.io/pullrequest.html#submitting

This comment has been minimized.

@tchaikov

tchaikov May 4, 2017

Contributor

ic. it does help to keep the history of review and amendments. but we rely on our contributors to do the squash, before merging. but if the change can be split into a series of logically separated changes, it would be ideal to keep them as they are in our commit history.

if your change is non trivial, and you are addressing the reviewer's comment in your change, it's advisable to have the fix addressing the comments on top of existing change, and squash it into the related commit, once the reviewer acks your fix.

common: fix building against libcryptopp
libnspr is only needed for libnss, move it to if defined(USE_NSS)
section.

ceph::crypto::shutdown's params should be same with declaration.

Signed-off-by: Shengjing Zhu <zhsj@umcloud.com>
@tchaikov

This comment has been minimized.

Contributor

tchaikov commented May 4, 2017

merging, as libcryptopp build is not tested by jenkins' "make check" or integration tests.

@tchaikov tchaikov merged commit b6da0d3 into ceph:master May 4, 2017

2 of 3 checks passed

default Build started sha1 is merged.
Details
Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details

@zhsj zhsj deleted the zhsj:fix-cryptopp-build branch May 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment