Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9715 )
Change subject: [build] Reduce the chances of a build abort corrupting the build ...................................................................... Patch Set 1: (10 comments) http://gerrit.cloudera.org:8080/#/c/9715/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9715/1//COMMIT_MSG@14 PS1, Line 14: Also introduces a “kudu-download-complete” file that : is used similarly to the patch level file, but indicates if : the build was interrupted during the download/patch : process. > It seems like kudu-download-complete has the same semantics as patchlevel: You're right. When I added this, every dependency didn't have a patch level. But now that I set patchlevel to 0 for any unpatched dependencies I can remove kudu-download-complete. http://gerrit.cloudera.org:8080/#/c/9715/1//COMMIT_MSG@20 PS1, Line 20: rapid-json > Nit: rapidjson Done http://gerrit.cloudera.org:8080/#/c/9715/1/thirdparty/download-thirdparty.sh File thirdparty/download-thirdparty.sh: http://gerrit.cloudera.org:8080/#/c/9715/1/thirdparty/download-thirdparty.sh@35 PS1, Line 35: kudu-download-complete > I don't think it's as important for files that are placed within the depend Like Adar mentioned I followed the patchlevel files "convention". If I changed on I would likely change both. http://gerrit.cloudera.org:8080/#/c/9715/1/thirdparty/download-thirdparty.sh@52 PS1, Line 52: unzip_to_source > nit: this looks a bit strange to me. Maybe, rename into unzip_to_src_dir ? I don't want to imply it's going to "src" when it's actually going to that dependencies source dir. http://gerrit.cloudera.org:8080/#/c/9715/1/thirdparty/download-thirdparty.sh@57 PS1, Line 57: head -n1 | tr -s ' ' | cut -d' ' -f5- | tr -d '/' > I'm not sure this gives the directory name. I tried this with archives whi Right, this wouldn't work on single file archives. It gives the first file, which for archives with directories is the the directory. See the old logic for rapidjson to demonstrate how this works. http://gerrit.cloudera.org:8080/#/c/9715/1/thirdparty/download-thirdparty.sh@124 PS1, Line 124: install_and_patch > How about 'fetch_and_patch' ? Done http://gerrit.cloudera.org:8080/#/c/9715/1/thirdparty/download-thirdparty.sh@128 PS1, Line 128: # local PATCH_COMMANDS A list of patch commands is expected last > Errant comment? I was trying to say that the remaining args are commands. I will clarify that a bit. http://gerrit.cloudera.org:8080/#/c/9715/1/thirdparty/download-thirdparty.sh@140 PS1, Line 140: touch kudu-download-complete > Shouldn't this be written after the download step? Otherwise doesn't it beh Done http://gerrit.cloudera.org:8080/#/c/9715/1/thirdparty/download-thirdparty.sh@218 PS1, Line 218: $CMAKE_PATCHES \ > Does this need to be double quoted? can't hurt http://gerrit.cloudera.org:8080/#/c/9715/1/thirdparty/download-thirdparty.sh@218 PS1, Line 218: $CMAKE_PATCHES \ > Does this need to be double quoted? Done -- To view, visit http://gerrit.cloudera.org:8080/9715 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I34b5065ebaf09503b0dc6daf11b962f5a41f7d76 Gerrit-Change-Number: 9715 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Mon, 19 Mar 2018 19:27:48 +0000 Gerrit-HasComments: Yes
