Adar Dembo has posted comments on this change. Change subject: Workaround test failures running with MIT krb5 1.10 ......................................................................
Patch Set 7: (7 comments) http://gerrit.cloudera.org:8080/#/c/4990/7/CMakeLists.txt File CMakeLists.txt: Line 854: find_package(Kerberos REQUIRED) Is there a particular minimum version we should enforce? http://gerrit.cloudera.org:8080/#/c/4990/7/cmake_modules/FindKerberos.cmake File cmake_modules/FindKerberos.cmake: PS7, Line 23: # KERBEROS_INCLUDE_DIR - where to find krb5.h, etc. : # KERBEROS_LIBRARY - List of libraries when using krb5. : # KERBEROS_FOUND - True if krb5 found. Nit: mind fixing the alignment and capitalization here? PS7, Line 27: IF (KERBEROS_INCLUDE_DIR) : # Already in cache, be silent : SET(KERBEROS_FIND_QUIETLY TRUE) : ENDIF (KERBEROS_INCLUDE_DIR) I think we can drop this; not sure why we'd care to avoid another find call if the result was cached. Line 32: FIND_PATH(KERBEROS_INCLUDE_DIR krb5.h) Built-in cmake directives should be lower-cased (find_path, set, etc.). Line 34: SET(KERBEROS_NAMES krb5 k5crypto com_err) Do we need to find all of these? Or just one of them? I think the single find_library() call passes if just one is found, but I imagine you wanted all of them? PS7, Line 40: KERBEROS Should be Kerberos Line 42: MARK_AS_ADVANCED(KERBEROS_LIBRARY KERBEROS_INCLUDE_DIR) I've never understood what this actually does, and I think we can omit it. -- To view, visit http://gerrit.cloudera.org:8080/4990 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
