You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2016/10/14 20:53:38 UTC
[kudu-CR] Add krb5 to thirdparty
Hello Adar Dembo, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/4727
to review the following change.
Change subject: Add krb5 to thirdparty
......................................................................
Add krb5 to thirdparty
Change-Id: I444c7da5a24fc9d554f542f300586b921cd65b6e
---
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
4 files changed, 29 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/4727/1
--
To view, visit http://gerrit.cloudera.org:8080/4727
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I444c7da5a24fc9d554f542f300586b921cd65b6e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add krb5 to thirdparty
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Add krb5 to thirdparty
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/4727/1/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
PS1, Line 557: Kudu does not link
: # against the built libraries for security reasons.
Well, Kudu doesn't actually need to build against it; Kudu's security dependency list terminates with gssapi, right?
Line 560: echo $KRB5_BDIR
Nit: drop this, debugging only right?
http://gerrit.cloudera.org:8080/#/c/4727/1/thirdparty/build-thirdparty.sh
File thirdparty/build-thirdparty.sh:
PS1, Line 24: # * /thirdparty/installed/common - prefix directory for libraries and binary tools
: # common to all build types, e.g. CMake,
: # krb5, C dependencies.
We should update this; C dependencies (and libraries) are no longer found here, just tools.
Line 105: if [ "$PREFIX_DIR" != "$PREFIX_COMMON" ]; then
May need to remove this now, since we're actually putting stuff in PREFIX_COMMON/lib once again.
http://gerrit.cloudera.org:8080/#/c/4727/1/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:
Line 256: fetch_and_expand ${KRB5_NAME}.tar.gz
Huh, I wonder why I didn't use FOO_NAME in the rest of the entries here. Seems like an oversight. Would you mind fixing that?
--
To view, visit http://gerrit.cloudera.org:8080/4727
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I444c7da5a24fc9d554f542f300586b921cd65b6e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add krb5 to thirdparty
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Add krb5 to thirdparty
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4727/1/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
Line 564: $KRB5_SOURCE/src/configure --prefix=$PREFIX
> Dan and I discussed this: if Kudu doesn't include any krb5 headers nor link
Do we need to enable/disable any of the default options while running configure? E.g., --without-readline, --without-libedit, --without-tcl?
In other words, are we satisfied with the default set of configuration options and does it give us stable build regardless of the installed/present system packages?
--
To view, visit http://gerrit.cloudera.org:8080/4727
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I444c7da5a24fc9d554f542f300586b921cd65b6e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add krb5 to thirdparty
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has abandoned this change.
Change subject: Add krb5 to thirdparty
......................................................................
Abandoned
We're going to require krb5kdc to be installed on test machines.
--
To view, visit http://gerrit.cloudera.org:8080/4727
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I444c7da5a24fc9d554f542f300586b921cd65b6e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add krb5 to thirdparty
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Add krb5 to thirdparty
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4727/1/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
Line 564: $KRB5_SOURCE/src/configure --prefix=$PREFIX
> I guess the includes actually aren't as problematic (since we wouldn't be i
As long as we avoid -Wl,-rpath=PREFIX_COMMON/lib (which we don't have today) when building the thirdparty dependencies and Kudu itself, that shouldn't happen. But, I can understand wanting more certainty here, to avoid it silently slipping in.
--
To view, visit http://gerrit.cloudera.org:8080/4727
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I444c7da5a24fc9d554f542f300586b921cd65b6e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add krb5 to thirdparty
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Add krb5 to thirdparty
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4727/1/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
Line 564: $KRB5_SOURCE/src/configure --prefix=$PREFIX
> Do we need to enable/disable any of the default options while running confi
Probably, it's already known and read, but just in case: there is nice doc on the configuration options in doc/html/build/options2configure.html
--
To view, visit http://gerrit.cloudera.org:8080/4727
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I444c7da5a24fc9d554f542f300586b921cd65b6e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add krb5 to thirdparty
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Add krb5 to thirdparty
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4727/1/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
Line 564: $KRB5_SOURCE/src/configure --prefix=$PREFIX
> does this end up putting the includes on our build's include path? I think
Dan and I discussed this: if Kudu doesn't include any krb5 headers nor link against krb5 libraries, what difference does it make?
--
To view, visit http://gerrit.cloudera.org:8080/4727
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I444c7da5a24fc9d554f542f300586b921cd65b6e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add krb5 to thirdparty
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Add krb5 to thirdparty
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4727/1/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
Line 564: $KRB5_SOURCE/src/configure --prefix=$PREFIX
does this end up putting the includes on our build's include path? I think we should avoid doing that (maybe install into a separate prefix entirely, or somehow pass a flag to avoid actually installing them)
--
To view, visit http://gerrit.cloudera.org:8080/4727
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I444c7da5a24fc9d554f542f300586b921cd65b6e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add krb5 to thirdparty
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Add krb5 to thirdparty
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4727/1/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
Line 564: $KRB5_SOURCE/src/configure --prefix=$PREFIX
> As long as we avoid -Wl,-rpath=PREFIX_COMMON/lib (which we don't have today
There is also --libdir options for the krb5's configure. But it seems it's no longer relevant anyway.
--
To view, visit http://gerrit.cloudera.org:8080/4727
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I444c7da5a24fc9d554f542f300586b921cd65b6e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add krb5 to thirdparty
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Add krb5 to thirdparty
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4727/1/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
PS1, Line 557: Kudu does not link
: # against the built libraries for security reasons.
> Well, Kudu doesn't actually need to build against it; Kudu's security depen
yea most likely we don't even need to build against GSSAPI, just against SASL (which we already do). The GSSAPI thing is a runtime sasl plugin
--
To view, visit http://gerrit.cloudera.org:8080/4727
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I444c7da5a24fc9d554f542f300586b921cd65b6e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add krb5 to thirdparty
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Add krb5 to thirdparty
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4727/1/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
Line 564: $KRB5_SOURCE/src/configure --prefix=$PREFIX
> BTW, if we still have any concerns regarding this, it's possible to use --i
I guess the includes actually aren't as problematic (since we wouldn't be including them), but libraries are potentially problematic if they end up dynamically loaded. For example, SASL will probably dynamically load libkrb5.so, and we want it to pick up the system version, not the one that we installed.
--
To view, visit http://gerrit.cloudera.org:8080/4727
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I444c7da5a24fc9d554f542f300586b921cd65b6e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add krb5 to thirdparty
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Add krb5 to thirdparty
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4727/1/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
Line 564: $KRB5_SOURCE/src/configure --prefix=$PREFIX
> Dan and I discussed this: if Kudu doesn't include any krb5 headers nor link
BTW, if we still have any concerns regarding this, it's possible to use --includedir option for configure. It's possible to point it to some non-standard subdir (or to a temporary directory and then remove it after build).
I remember Dan didn't like the idea of having manual pages and other stuff installed. To get rid of that, it's possible to point --datarootdir configure's option to some temporary directory and then remove if after build is done.
--
To view, visit http://gerrit.cloudera.org:8080/4727
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I444c7da5a24fc9d554f542f300586b921cd65b6e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes