Adar Dembo has posted comments on this change. Change subject: KUDU-1336. Add a simple preflight check for thirdparty build ......................................................................
Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/2450/2/thirdparty/preflight.py File thirdparty/preflight.py: Line 25: more complicated config tests using something like autoconf or cmake. You say that now...in five years take another look and tell me how it hasn't become a Python implementation of autoconf. Line 52: with NamedTemporaryFile(suffix=".cc") as f: Wouldn't you rather pass what you want to compile via STDIN? Cleaner, fewer Python dependencies, more street cred, etc. Line 68: for tool in ["automake", "autoconf", "libtool", "pkg-config", "patch", Nit: this list is probably the most likely thing to change in this script over time. Could you move it to a global variable, sort the entries alphabetically, and put them one on each line? That'll make it easiest to add/remove/find entries. Line 70: try_do( If possible, it would be great to try all of these tools and provide one list at the end of what's missing, rather than having to run the preflight script over and over until it passes. Line 85: int main(int argc, char** argv) { Don't need a main if you use -c in compile(). Not even sure if you need anything apart from the #include to be honest, the header would probably be missing if C++11 wasn't supported, no? Same goes for the other compile tests. Line 117: print "Pre-flight checks succeeded." This sequence of checks is likely to grow over time. Could you decompose each into a separate function, so that main() reads like "try this, try that, try this"? That way someone could learn about all of the checks without getting bogged down in the details of how they work. -- To view, visit http://gerrit.cloudera.org:8080/2450 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I93ee4165bd560f9cd3f03877bd3011decc7e1a6f Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
