Adar Dembo has posted comments on this change. Change subject: cmake: require out-of-tree builds ......................................................................
Patch Set 9: > What is the point of this "requirement"? I've actually gone back to having in > tree builds and add the missing entries to my gitignore_global, since the > sibling out of tree stuff seriously broke my workflow (need to go back and > forth between directories, eclipse git integration doesn't work with sibling > out-of-tree) and the child out-of-tree option doesn't work with eclipse. > Would prefer this is not made a requirement. Let's take a step back first. For the purpose of this discussion, we want our build system to have the following properties: 1. Concurrent builds of different build types (e.g. ASAN, TSAN, debug). Yes, we're used to a world where this isn't the case, where you have to clobber your make/ninja files in order to switch build types, but that doesn't mean we can't do better. By having the cmake's generated output live in the build directory (of which there can be more than one), we can achieve that. 2. Related to #1, the ability to work on different git branches without clobbering your build output. With some care (if you checkout branch-x, don't run make in the branch-y build directory), you can do this with a single tree. Note that it doesn't work 100% well yet because thirdparty/installed is a single monolithic directory, but I expect that will change in a future round of build infrastructure work, and besides, it's only a problem if the difference between the branches includes a thirdparty change. 3. Checked-in files are well isolated from untracked files. This generally applies to build output, but it also applies to generated code, generated ninja/cmake files, etc. The experience of doing something "make clean" or even "rm -rf build/" is better than "git clean -xdf" (because you don't trust the build system to clean itself up property). Not to mention it makes for smaller .gitignore files, lowering the likelihood that files accidentally get ignored (we had this happen once in CM's Python build, it led to several hours of debugging, not fun). 4. As consumers of cmake, it's good for our build system to behave predictably w.r.t. other cmake projects. It lowers the barrier to entry for making cmake changes by anyone in the community. I understand that this is a very minor improvement, but I think it's good to list it for completeness. BTW, you can read more about the official cmake position on out-of-tree builds here: https://cmake.org/Wiki/CMake_FAQ#Out-of-source_build_trees. I clearly misjudged the number of customized workflows that we have, though, in my defense, I was practically begging for code reviews from everyone on the team, and that would have been a good venue to discuss the various customizations that everyone uses. As the author of this work, it won't surprise you to hear that I'm emotionally invested in it, and thus my preferred solution is to "have my cake and eat it too." That is, I'd like to learn about and fix all the broken workflows, without compromising on out-of-tree builds if I can. At the very least, I'd like to understand which broken workflows simply cannot be fixed before I compromise. Having talked to Todd about this over the past few days, I'm thinking something like this is a reasonable middle ground: 1. Keep the build patches that enable support for out-of-tree builds. 2. Strongly encourage developers to put all of their build directories in "build/". Provide a script to create this directory structure and prepopulate it with popular options like debug, release, tsan, etc. 3. When cmake is invoked in build/<foo>, update a build/latest symlink to point to build/<foo>. 4. The above means there will be a build/latest/latest directory which is confusing. Deal with this by either renaming the grandchild to "output", or going with Dan's suggestion of putting binaries in "bin", libraries in "lib", etc. I like the latter but I wonder if it'll affect rpaths, dist-test, etc. 5. Make sure everything works well with the directory structure from #2. By "everything" I mean all scripts, Python tests, Java tests, dist-test, and as many custom workflows as possible. There should be no additional steps necessary (i.e. environment variables or command line options) to get stuff to just work. It should be just like today, well, today minus one week. 6. (Best effort) make sure everything works well with any build directory. As part of the original patch I know that 'make test' (ctest) and 'make docs' both work regardless of where the build directories are, because they execute via cmake and thus have build directory awareness. That's probably good enough for every day development needs. I'm also fine with scripts expecting --build-dir arguments for this use case, which is basically how the Python/Java tests continue to work. In #2, why "strongly encourage/recommend" and not "require"? I'll admit I still have ideological reasons about granting people flexibility, but there's also a concrete reason: the Eclipse cmake generator warns against running out-of-tree when that means being in a child subdirectory. It claims this can lead to problems within Eclipse. So I've got a kudu_eclipse sibling build directory that I use just for generating Eclipse projects, and I'd like to keep using that. The remaining question is whether to forbid builds where the source and build directories are the same. I'd like to because I think it makes things overall simpler, safer, and more predictable. If the above is satisfactory, the only remaining hurdle to forbidding those builds is the Eclipse git integration that David mentioned. I suggest we look at that together to see if it can't be fixed before declaring this patch a lost cause. -- To view, visit http://gerrit.cloudera.org:8080/1762 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I57a9eab7d35419a3668b9dce384a049cddd1afef Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: No
