[kudu-CR] [CMakeLists.txt] allow override for openssl on Linux

2017-10-27 Thread Alexey Serbin (Code Review) 
Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8407 )

Change subject: [CMakeLists.txt] allow override for openssl on Linux
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8407/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8407/1//COMMIT_MSG@9
PS1, Line 9: If specified, use the override for OPENSSL_ROOT_DIR for Linux 
builds
   : even if CENTOS_6_4_OPENSSL_DIR exists.
> I buy that, but the wording in your commit message (until the last line) su
I suspect that was unintended obfuscation :)  I need to teach myself some 
clarity discipline.

I'll clarify, sure.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe69b5d62ef4054e60138a154a85569d92166aff
Gerrit-Change-Number: 8407
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 27 Oct 2017 22:25:34 +
Gerrit-HasComments: Yes

[kudu-CR] [CMakeLists.txt] allow override for openssl on Linux

2017-10-27 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8407 )

Change subject: [CMakeLists.txt] allow override for openssl on Linux
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8407/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8407/1//COMMIT_MSG@9
PS1, Line 9: If specified, use the override for OPENSSL_ROOT_DIR for Linux 
builds
   : even if CENTOS_6_4_OPENSSL_DIR exists.
> Yes, it was, but it didn't work as expected.  At least it didn't work for m
I buy that, but the wording in your commit message (until the last line) 
suggests you're adding a new feature whereas you're just fixing a broken one. 
That's what I'd like to see clarified.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe69b5d62ef4054e60138a154a85569d92166aff
Gerrit-Change-Number: 8407
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 27 Oct 2017 22:16:04 +
Gerrit-HasComments: Yes

[kudu-CR] [CMakeLists.txt] allow override for openssl on Linux

2017-10-27 Thread Alexey Serbin (Code Review) 
Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8407 )

Change subject: [CMakeLists.txt] allow override for openssl on Linux
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8407/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8407/1//COMMIT_MSG@9
PS1, Line 9: If specified, use the override for OPENSSL_ROOT_DIR for Linux 
builds
   : even if CENTOS_6_4_OPENSSL_DIR exists.
> I don't really understand this; wasn't that the intent of the existing code
Yes, it was, but it didn't work as expected.  At least it didn't work for me at 
ve0518.halxg.cloudera.com.

FWIW, I also tried

if ((NOT "${OPENSSL_ROOT_DIR}") AND EXISTS "${CENTOS_6_4_OPENSSL_DIR}")
...
endif()


but it didn't work neither.


http://gerrit.cloudera.org:8080/#/c/8407/1//COMMIT_MSG@12
PS1, Line 12: The prior version didn't work at least with cmake 2.8.12.2.
> How are you using such an old version of cmake? The beginning of CMakeLists
woops, that's a mistake.  It was 3.9.0, of course.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe69b5d62ef4054e60138a154a85569d92166aff
Gerrit-Change-Number: 8407
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 27 Oct 2017 22:13:56 +
Gerrit-HasComments: Yes

[kudu-CR] [CMakeLists.txt] allow override for openssl on Linux

2017-10-27 Thread Alexey Serbin (Code Review) 
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8407

to look at the new patch set (#2).

Change subject: [CMakeLists.txt] allow override for openssl on Linux
..

[CMakeLists.txt] allow override for openssl on Linux

If specified, use the override for OPENSSL_ROOT_DIR for Linux builds
even if CENTOS_6_4_OPENSSL_DIR exists.

The prior version didn't work at least with cmake 3.9.0.

Change-Id: Ibe69b5d62ef4054e60138a154a85569d92166aff
---
M CMakeLists.txt
1 file changed, 1 insertion(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/8407/2
-- 
To view, visit http://gerrit.cloudera.org:8080/8407
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibe69b5d62ef4054e60138a154a85569d92166aff
Gerrit-Change-Number: 8407
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] [CMakeLists.txt] allow override for openssl on Linux

2017-10-27 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8407 )

Change subject: [CMakeLists.txt] allow override for openssl on Linux
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8407/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8407/1//COMMIT_MSG@9
PS1, Line 9: If specified, use the override for OPENSSL_ROOT_DIR for Linux 
builds
   : even if CENTOS_6_4_OPENSSL_DIR exists.
I don't really understand this; wasn't that the intent of the existing code too?


http://gerrit.cloudera.org:8080/#/c/8407/1//COMMIT_MSG@12
PS1, Line 12: The prior version didn't work at least with cmake 2.8.12.2.
How are you using such an old version of cmake? The beginning of CMakeLists.txt 
has:

  # Require cmake that can build LLVM [1].
  #
  # Note: cmake in thirdparty/ will always meet this minimum.
  #
  # 1. http://llvm.org/releases/3.9.0/docs/ReleaseNotes.html
  cmake_minimum_required(VERSION 3.4.3)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe69b5d62ef4054e60138a154a85569d92166aff
Gerrit-Change-Number: 8407
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 27 Oct 2017 22:09:13 +
Gerrit-HasComments: Yes

[kudu-CR] [CMakeLists.txt] allow override for openssl on Linux

2017-10-27 Thread Alexey Serbin (Code Review) 
Alexey Serbin has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8407


Change subject: [CMakeLists.txt] allow override for openssl on Linux
..

[CMakeLists.txt] allow override for openssl on Linux

If specified, use the override for OPENSSL_ROOT_DIR for Linux builds
even if CENTOS_6_4_OPENSSL_DIR exists.

The prior version didn't work at least with cmake 2.8.12.2.

Change-Id: Ibe69b5d62ef4054e60138a154a85569d92166aff
---
M CMakeLists.txt
1 file changed, 1 insertion(+), 1 deletion(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/8407/1
--
To view, visit http://gerrit.cloudera.org:8080/8407
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibe69b5d62ef4054e60138a154a85569d92166aff
Gerrit-Change-Number: 8407
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin