You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Henry Robinson (Code Review)" <ge...@cloudera.org> on 2017/01/13 18:54:54 UTC
[kudu-CR] Add NO NVM SUPPORT cmake var to suppress linking or compiling with nvm support
Henry Robinson has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5712
Change subject: Add NO_NVM_SUPPORT cmake var to suppress linking or compiling with nvm support
......................................................................
Add NO_NVM_SUPPORT cmake var to suppress linking or compiling with nvm support
Change-Id: Ib915b0c9e08d0e3eae0bf778dca6f6138e83bc21
---
M CMakeLists.txt
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache.cc
3 files changed, 34 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/5712/1
--
To view, visit http://gerrit.cloudera.org:8080/5712
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib915b0c9e08d0e3eae0bf778dca6f6138e83bc21
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
[kudu-CR] Add NO NVM SUPPORT cmake var to suppress linking or compiling with nvm support
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5712
to look at the new patch set (#3).
Change subject: Add NO_NVM_SUPPORT cmake var to suppress linking or compiling with nvm support
......................................................................
Add NO_NVM_SUPPORT cmake var to suppress linking or compiling with nvm support
Change-Id: Ib915b0c9e08d0e3eae0bf778dca6f6138e83bc21
---
M CMakeLists.txt
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache.cc
3 files changed, 31 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/5712/3
--
To view, visit http://gerrit.cloudera.org:8080/5712
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib915b0c9e08d0e3eae0bf778dca6f6138e83bc21
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Add NO NVM SUPPORT cmake var to suppress linking or compiling with nvm support
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5712
to look at the new patch set (#2).
Change subject: Add NO_NVM_SUPPORT cmake var to suppress linking or compiling with nvm support
......................................................................
Add NO_NVM_SUPPORT cmake var to suppress linking or compiling with nvm support
Change-Id: Ib915b0c9e08d0e3eae0bf778dca6f6138e83bc21
---
M CMakeLists.txt
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache.cc
3 files changed, 31 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/5712/2
--
To view, visit http://gerrit.cloudera.org:8080/5712
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib915b0c9e08d0e3eae0bf778dca6f6138e83bc21
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Add NO NVM SUPPORT cmake var to suppress linking or compiling with nvm support
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Add NO_NVM_SUPPORT cmake var to suppress linking or compiling with nvm support
......................................................................
Patch Set 2:
> We consume kudu_util by dropping it in our source dir, and building
> it like it were another local library. This patch https://github.com/henryr/Impala/commit/5958d6e3211e113fd5e42562ef27cfae021f5f23
> might shed more light on the situation.
>
> We don't consume thirdparty at all. Any new dependencies (libev,
> crcutil, proto*) have gone into our toolchain. Hence, with this
> change, we don't touch nvm libraries at all.
Makes sense, thanks. Will +2 once you address that one issue in cache.cc.
--
To view, visit http://gerrit.cloudera.org:8080/5712
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib915b0c9e08d0e3eae0bf778dca6f6138e83bc21
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] Add NO NVM SUPPORT cmake var to suppress linking or compiling with nvm support
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has abandoned this change.
Change subject: Add NO_NVM_SUPPORT cmake var to suppress linking or compiling with nvm support
......................................................................
Abandoned
--
To view, visit http://gerrit.cloudera.org:8080/5712
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: Ib915b0c9e08d0e3eae0bf778dca6f6138e83bc21
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Add NO NVM SUPPORT cmake var to suppress linking or compiling with nvm support
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Add NO_NVM_SUPPORT cmake var to suppress linking or compiling with nvm support
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5712/3/src/kudu/util/CMakeLists.txt
File src/kudu/util/CMakeLists.txt:
Line 236: target_compile_definitions(kudu_util PRIVATE
On second thought, could you move this to the main CMakeLists.txt and set it up via add_definitions(), so it'll apply universally?
That way if something like "#ifndef NO_NVM_SUPPORT" finds its way into some code outside of kudu_util, we won't have to worry about accidentally forgetting to expand the scope of this statement.
--
To view, visit http://gerrit.cloudera.org:8080/5712
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib915b0c9e08d0e3eae0bf778dca6f6138e83bc21
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] Add NO NVM SUPPORT cmake var to suppress linking or compiling with nvm support
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Add NO_NVM_SUPPORT cmake var to suppress linking or compiling with nvm support
......................................................................
Patch Set 2:
(1 comment)
Hmm. How does Impala consume kudu_util. Does it do so without building the thirdparty tree? If not, I don't understand how this simplifies anything, because you'd still be _building_ nvml.
As an aside, we should really start documenting our CMake options via option(). Not your problem (and not in scope for this change) though.
http://gerrit.cloudera.org:8080/#/c/5712/2/src/kudu/util/cache.cc
File src/kudu/util/cache.cc:
Line 503: #if !defined(__APPLE__) && !defined(NO_NVM_SUPPORT)
__APPLE__ implies NO_NVM_SUPPORT given your changes to CMakeLists.txt, right? So the first case can be removed?
--
To view, visit http://gerrit.cloudera.org:8080/5712
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib915b0c9e08d0e3eae0bf778dca6f6138e83bc21
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] Add NO NVM SUPPORT cmake var to suppress linking or compiling with nvm support
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: Add NO_NVM_SUPPORT cmake var to suppress linking or compiling with nvm support
......................................................................
Patch Set 2:
We consume kudu_util by dropping it in our source dir, and building it like it were another local library. This patch https://github.com/henryr/Impala/commit/5958d6e3211e113fd5e42562ef27cfae021f5f23 might shed more light on the situation.
We don't consume thirdparty at all. Any new dependencies (libev, crcutil, proto*) have gone into our toolchain. Hence, with this change, we don't touch nvm libraries at all.
--
To view, visit http://gerrit.cloudera.org:8080/5712
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib915b0c9e08d0e3eae0bf778dca6f6138e83bc21
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] Add NO NVM SUPPORT cmake var to suppress linking or compiling with nvm support
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: Add NO_NVM_SUPPORT cmake var to suppress linking or compiling with nvm support
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5712/2/src/kudu/util/cache.cc
File src/kudu/util/cache.cc:
Line 503: #if !defined(__APPLE__) && !defined(NO_NVM_SUPPORT)
> __APPLE__ implies NO_NVM_SUPPORT given your changes to CMakeLists.txt, righ
Done
--
To view, visit http://gerrit.cloudera.org:8080/5712
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib915b0c9e08d0e3eae0bf778dca6f6138e83bc21
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes