Adar Dembo has posted comments on this change.

Change subject: cmake: add support for and require out-of-tree builds
......................................................................


Patch Set 3:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/1755/3/README.adoc
File README.adoc:

Line 66: _build/debug/latest/_.
> why keep this? The latest does not makes sense anymore with oos builds or a
You're correct. See the commit description: I kept it for easier compatibility 
with existing scripts, many of which expect to find output in "build/latest".

If people feel strongly about it, though, I'm happy to change it to "output" or 
something equivalent.


Line 131: $ ctest -R failing-test
> Doesn't this need to be executed from within the build dir?
Yes, I'll modify it. Thanks.


http://gerrit.cloudera.org:8080/#/c/1755/3/docs/installation.adoc
File docs/installation.adoc:

Line 413: anywhere in your filesystem except for the `kudu` directory itself. 
> ws
Fixed.


http://gerrit.cloudera.org:8080/#/c/1755/3/src/kudu/client/CMakeLists.txt
File src/kudu/client/CMakeLists.txt:

Line 172:   ../util/monotime.h
> why is the path prefix not needed for the other files here?
kudu_export.h is an autogenerated file (see L137-139) and thus lives in the 
build directory. The rest are regular source files.


http://gerrit.cloudera.org:8080/#/c/1755/3/src/kudu/scripts/benchmarks.sh
File src/kudu/scripts/benchmarks.sh:

Line 199:   [ -d "$LOGDIR" ] || mkdir -p "$LOGDIR"
> mkdir -p will simply do nothing if it does exist so the test is not needed.
Yeah, I had it this way because that's how I found the code. But I'll change it.


-- 
To view, visit http://gerrit.cloudera.org:8080/1755
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76e17a1102b79ac0e25a505b54347db3bb436ede
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <[email protected]>
Gerrit-Reviewer: Adar Dembo <[email protected]>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Martin Grund <[email protected]>
Gerrit-Reviewer: Mike Percy <[email protected]>
Gerrit-Reviewer: Todd Lipcon <[email protected]>
Gerrit-HasComments: Yes

Reply via email to