You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2020/02/07 08:43:35 UTC
[kudu-CR] [cmake modules] shared libs for yaml and gumbo on macOS
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15182
Change subject: [cmake_modules] shared libs for yaml and gumbo on macOS
......................................................................
[cmake_modules] shared libs for yaml and gumbo on macOS
I noticed that even in DEBUG build yaml and gumbo libraries were
linked in statically when building on macOS. It turned out the
system could not find shared libraries since their pattern contained
non-macOS '.so' suffix.
This patch address the issue: I verified that with with patch non-debug
binaries are linked dynamically with the libraries mentioned above.
Change-Id: Ie1e56b9dd988501af010bbd1a256671b0460f526
---
M cmake_modules/FindGumboParser.cmake
M cmake_modules/FindGumboQuery.cmake
M cmake_modules/FindYaml.cmake
3 files changed, 6 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/82/15182/1
--
To view, visit http://gerrit.cloudera.org:8080/15182
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie1e56b9dd988501af010bbd1a256671b0460f526
Gerrit-Change-Number: 15182
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [cmake modules] shared libs for yaml and gumbo on macOS
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Volodymyr Verovkin,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15182
to look at the new patch set (#3).
Change subject: [cmake_modules] shared libs for yaml and gumbo on macOS
......................................................................
[cmake_modules] shared libs for yaml and gumbo on macOS
I noticed that even in DEBUG build yaml and gumbo libraries were
linked in statically when building on macOS. It turned out the
system could not find shared libraries since their pattern contained
non-macOS '.so' suffix.
This patch address the issue. I verified that non-debug binaries
are linked dynamically with the libraries mentioned above.
I also added some extra code to protect against using static libraries
in place of the shared ones. It's possible to apply this approach
to other thirdparty libraries used by Kudu, but I think it's better
to do so in a separate changelist.
Change-Id: Ie1e56b9dd988501af010bbd1a256671b0460f526
---
M cmake_modules/FindGumboParser.cmake
M cmake_modules/FindGumboQuery.cmake
M cmake_modules/FindYaml.cmake
3 files changed, 36 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/82/15182/3
--
To view, visit http://gerrit.cloudera.org:8080/15182
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie1e56b9dd988501af010bbd1a256671b0460f526
Gerrit-Change-Number: 15182
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] [cmake modules] shared libs for yaml and gumbo on macOS
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15182 )
Change subject: [cmake_modules] shared libs for yaml and gumbo on macOS
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15182/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15182/1//COMMIT_MSG@14
PS1, Line 14: with
> Nit: too many with
Done
http://gerrit.cloudera.org:8080/#/c/15182/1/cmake_modules/FindGumboParser.cmake
File cmake_modules/FindGumboParser.cmake:
http://gerrit.cloudera.org:8080/#/c/15182/1/cmake_modules/FindGumboParser.cmake@25
PS1, Line 25: gumbo
> If libgumbo.so (or libgumbo.dylib on macOS) is missing, does this match aga
Yup, it happily consumes the static library as a dynamic one here if the file of the actual shared library is missing:
Added shared library dependency gumbo-parser: /Users/aserbin/Projects/kudu/thirdparty/installed/uninstrumented/lib/libgumbo.a
I sketched a way to handle that in a more reliable way, but it would be necessary to make it more generic and apply to other Find*.cmake files as well.
--
To view, visit http://gerrit.cloudera.org:8080/15182
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1e56b9dd988501af010bbd1a256671b0460f526
Gerrit-Change-Number: 15182
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Feb 2020 01:51:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] [cmake modules] shared libs for yaml and gumbo on macOS
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15182 )
Change subject: [cmake_modules] shared libs for yaml and gumbo on macOS
......................................................................
Patch Set 2: Verified+1
unrelated test failures
--
To view, visit http://gerrit.cloudera.org:8080/15182
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1e56b9dd988501af010bbd1a256671b0460f526
Gerrit-Change-Number: 15182
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Feb 2020 05:24:31 +0000
Gerrit-HasComments: No
[kudu-CR] [cmake modules] shared libs for yaml and gumbo on macOS
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15182 )
Change subject: [cmake_modules] shared libs for yaml and gumbo on macOS
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15182/2/cmake_modules/FindGumboParser.cmake
File cmake_modules/FindGumboParser.cmake:
http://gerrit.cloudera.org:8080/#/c/15182/2/cmake_modules/FindGumboParser.cmake@33
PS2, Line 33: find_library(GUMBO_PARSER_SHARED_LIB gumbo
What about replacing 'gumbo' with 'libgumbo${CMAKE_SHARED_LIBRARY_SUFFIX}'? Would that work without the set/restore stuff?
https://cmake.org/cmake/help/latest/variable/CMAKE_SHARED_LIBRARY_SUFFIX.html
BTW, I'd be fine with doing what every Find*.cmake does in this patch, then maybe doing this sort of robustness fix in a separate patch across the board.
--
To view, visit http://gerrit.cloudera.org:8080/15182
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1e56b9dd988501af010bbd1a256671b0460f526
Gerrit-Change-Number: 15182
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Feb 2020 05:53:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] [cmake modules] shared libs for yaml and gumbo on macOS
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15182 )
Change subject: [cmake_modules] shared libs for yaml and gumbo on macOS
......................................................................
[cmake_modules] shared libs for yaml and gumbo on macOS
I noticed that even in DEBUG build yaml and gumbo libraries were
linked in statically when building on macOS. It turned out the
system could not find shared libraries since their pattern contained
non-macOS '.so' suffix.
This patch address the issue. I verified that non-debug binaries
are linked dynamically with the libraries mentioned above.
I also added some extra code to protect against using static libraries
in place of the shared ones. It's possible to apply this approach
to other thirdparty libraries used by Kudu, but I think it's better
to do so in a separate changelist.
Change-Id: Ie1e56b9dd988501af010bbd1a256671b0460f526
Reviewed-on: http://gerrit.cloudera.org:8080/15182
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M cmake_modules/FindGumboParser.cmake
M cmake_modules/FindGumboQuery.cmake
M cmake_modules/FindYaml.cmake
3 files changed, 36 insertions(+), 6 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/15182
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie1e56b9dd988501af010bbd1a256671b0460f526
Gerrit-Change-Number: 15182
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] [cmake modules] shared libs for yaml and gumbo on macOS
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15182 )
Change subject: [cmake_modules] shared libs for yaml and gumbo on macOS
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15182/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15182/1//COMMIT_MSG@14
PS1, Line 14: with
Nit: too many with
http://gerrit.cloudera.org:8080/#/c/15182/1/cmake_modules/FindGumboParser.cmake
File cmake_modules/FindGumboParser.cmake:
http://gerrit.cloudera.org:8080/#/c/15182/1/cmake_modules/FindGumboParser.cmake@25
PS1, Line 25: gumbo
If libgumbo.so (or libgumbo.dylib on macOS) is missing, does this match against libgumbo.a?
--
To view, visit http://gerrit.cloudera.org:8080/15182
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1e56b9dd988501af010bbd1a256671b0460f526
Gerrit-Change-Number: 15182
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 Feb 2020 20:20:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] [cmake modules] shared libs for yaml and gumbo on macOS
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15182 )
Change subject: [cmake_modules] shared libs for yaml and gumbo on macOS
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15182
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1e56b9dd988501af010bbd1a256671b0460f526
Gerrit-Change-Number: 15182
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Feb 2020 18:00:02 +0000
Gerrit-HasComments: No
[kudu-CR] [cmake modules] shared libs for yaml and gumbo on macOS
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15182 )
Change subject: [cmake_modules] shared libs for yaml and gumbo on macOS
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15182/2/cmake_modules/FindGumboParser.cmake
File cmake_modules/FindGumboParser.cmake:
http://gerrit.cloudera.org:8080/#/c/15182/2/cmake_modules/FindGumboParser.cmake@33
PS2, Line 33: find_library(GUMBO_PARSER_SHARED_LIB gumbo
> What about replacing 'gumbo' with 'libgumbo${CMAKE_SHARED_LIBRARY_SUFFIX}'?
CMAKE_SHARED_LIBRARY_SUFFIX on macOS is ".tbd;.dylib;.so;.a", so nope, it would not work.
I also tried to set CMAKE_SHARED_LIBRARY_SUFFIX just for shared libs (i.e. '.so' on linux and '.dylib' on macOS) in the top-level CMakeLists.txt in $KUDU_ROOT, but it didn't work: if when using lib<name>.a for static library part, cmake could not find the static library (apparently, it needs '.a' to be in the suffix list even if lib<name>.a is given in find_library() function).
Yep, I can do this in every other file in this patch. And next patch we could introduce a function/macro around find_library(), something find_static_library() and find_shared_library().
--
To view, visit http://gerrit.cloudera.org:8080/15182
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1e56b9dd988501af010bbd1a256671b0460f526
Gerrit-Change-Number: 15182
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Feb 2020 06:00:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] [cmake modules] shared libs for yaml and gumbo on macOS
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: [cmake_modules] shared libs for yaml and gumbo on macOS
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/15182
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ie1e56b9dd988501af010bbd1a256671b0460f526
Gerrit-Change-Number: 15182
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] [cmake modules] shared libs for yaml and gumbo on macOS
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Volodymyr Verovkin,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15182
to look at the new patch set (#2).
Change subject: [cmake_modules] shared libs for yaml and gumbo on macOS
......................................................................
[cmake_modules] shared libs for yaml and gumbo on macOS
I noticed that even in DEBUG build yaml and gumbo libraries were
linked in statically when building on macOS. It turned out the
system could not find shared libraries since their pattern contained
non-macOS '.so' suffix.
This patch address the issue: I verified that non-debug binaries
are linked dynamically with the libraries mentioned above.
I also added some extra work to protect against using static gumbo
library in place of the shared one. The approach could be applied
for other libraries as well, but I'm not sure it's worth it.
Change-Id: Ie1e56b9dd988501af010bbd1a256671b0460f526
---
M cmake_modules/FindGumboParser.cmake
M cmake_modules/FindGumboQuery.cmake
M cmake_modules/FindYaml.cmake
3 files changed, 16 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/82/15182/2
--
To view, visit http://gerrit.cloudera.org:8080/15182
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie1e56b9dd988501af010bbd1a256671b0460f526
Gerrit-Change-Number: 15182
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>