Todd Lipcon has posted comments on this change. Change subject: Workaround test failures running with MIT krb5 1.10 ......................................................................
Patch Set 7: (8 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? I don't think the thing we're using to detect is is smart enough to know the version number (and doubt it's worth trying to build it since we'll just know when our tests fail) PS7, Line 855: include_directories(${KERBEROS_INCLUDE_DIR}) > nit: if possible, consider adding this for relevant sources only (i.e. movi hrm, that's not very standard compared to what we do with all of the other thirdparty things, so I'd prefer not to. What's the rationale? 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? Done 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 Done Line 32: FIND_PATH(KERBEROS_INCLUDE_DIR krb5.h) > Built-in cmake directives should be lower-cased (find_path, set, etc.). Done 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 fi I copied this from quickstep, but I'll change to just krb5 since that's the only one we're explicitly linking PS7, Line 40: KERBEROS > Should be Kerberos Really? All of the other ones in cmake_modules are all-caps here (eg in FindSnappy, FindBitshuffle etc) 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. Done -- 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
