You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2018/05/16 21:21:31 UTC

[kudu-CR] KUDU-2427: adjust gold linker detection 

Hello Todd Lipcon,

I'd like you to do a code review. Please visit

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

to review the following change.


Change subject: KUDU-2427: adjust gold linker detection
......................................................................

KUDU-2427: adjust gold linker detection

This patch makes two adjustments to the existing gold linker detection:
1. Ubuntu 18.04's gcc 7 no longer respects a /usr/bin/ld symlink; it is
   hard-wired to use ld.bfd. Instead, we need to pass -fuse-ld=gold to
   choose the gold linker.
2. The old bug that led to tcmalloc being dropped from Kudu's dependency
   list has been fixed, so let's condition the workaround on the appropriate
   version of the gold linker.

Change-Id: Ib1fae9893aaaf4916205d4c5ae6bb5c93e0505d0
---
M CMakeLists.txt
M src/kudu/codegen/CMakeLists.txt
2 files changed, 78 insertions(+), 21 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib1fae9893aaaf4916205d4c5ae6bb5c93e0505d0
Gerrit-Change-Number: 10428
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2427: adjust gold linker detection

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10428 )

Change subject: KUDU-2427: adjust gold linker detection
......................................................................


Patch Set 3: Code-Review+1

(3 comments)

http://gerrit.cloudera.org:8080/#/c/10428/3/CMakeLists.txt
File CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/10428/3/CMakeLists.txt@442
PS3, Line 442: extract
extracts


http://gerrit.cloudera.org:8080/#/c/10428/3/CMakeLists.txt@442
PS3, Line 442: to 
drop


http://gerrit.cloudera.org:8080/#/c/10428/3/CMakeLists.txt@510
PS3, Line 510: Error: 
Nit: does it add more clarity for the message?  I saw another error message above, and it does not contain extra suffix like this.  Maybe, for more consistency, add this suffix for the message at like 463 or drop it here.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib1fae9893aaaf4916205d4c5ae6bb5c93e0505d0
Gerrit-Change-Number: 10428
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 17 May 2018 17:32:11 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2427: adjust gold linker detection

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Todd Lipcon, 

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

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

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

Change subject: KUDU-2427: adjust gold linker detection
......................................................................

KUDU-2427: adjust gold linker detection

This patch makes two adjustments to the existing gold linker detection:
1. Ubuntu 18.04's gcc 7 no longer respects a /usr/bin/ld symlink; it is
   hard-wired to use ld.bfd. Instead, we need to pass -fuse-ld=gold to
   choose the gold linker.
2. The old bug that led to tcmalloc being dropped from Kudu's dependency
   list has been fixed, so let's condition the workaround on the appropriate
   version of the gold linker.

Change-Id: Ib1fae9893aaaf4916205d4c5ae6bb5c93e0505d0
---
M CMakeLists.txt
M src/kudu/codegen/CMakeLists.txt
2 files changed, 86 insertions(+), 21 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib1fae9893aaaf4916205d4c5ae6bb5c93e0505d0
Gerrit-Change-Number: 10428
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2427: adjust gold linker detection

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10428 )

Change subject: KUDU-2427: adjust gold linker detection
......................................................................


Patch Set 4: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib1fae9893aaaf4916205d4c5ae6bb5c93e0505d0
Gerrit-Change-Number: 10428
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 17 May 2018 23:44:55 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2427: adjust gold linker detection

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10428 )

Change subject: KUDU-2427: adjust gold linker detection
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10428/3/CMakeLists.txt
File CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/10428/3/CMakeLists.txt@484
PS3, Line 484: devtoolset-3
is this dts-3 specific? or would also be true of devtoolset-7 for example?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib1fae9893aaaf4916205d4c5ae6bb5c93e0505d0
Gerrit-Change-Number: 10428
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 17 May 2018 17:37:09 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2427: adjust gold linker detection

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Alexey Serbin, Kudu Jenkins, Todd Lipcon, 

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

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

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

Change subject: KUDU-2427: adjust gold linker detection
......................................................................

KUDU-2427: adjust gold linker detection

This patch makes two adjustments to the existing gold linker detection:
1. Ubuntu 18.04's gcc 7 no longer respects a /usr/bin/ld symlink; it is
   hard-wired to use ld.bfd. Instead, we need to pass -fuse-ld=gold to
   choose the gold linker.
2. The old bug that led to tcmalloc being dropped from Kudu's dependency
   list has been fixed, so let's condition the workaround on the appropriate
   version of the gold linker.

Change-Id: Ib1fae9893aaaf4916205d4c5ae6bb5c93e0505d0
---
M CMakeLists.txt
M src/kudu/codegen/CMakeLists.txt
2 files changed, 87 insertions(+), 21 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/10428/4
-- 
To view, visit http://gerrit.cloudera.org:8080/10428
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib1fae9893aaaf4916205d4c5ae6bb5c93e0505d0
Gerrit-Change-Number: 10428
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2427: adjust gold linker detection

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10428 )

Change subject: KUDU-2427: adjust gold linker detection
......................................................................


Patch Set 3: Code-Review+1

(2 comments)

looks good although I'm not sure skipping gold on el6 is necessary

http://gerrit.cloudera.org:8080/#/c/10428/3/CMakeLists.txt
File CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/10428/3/CMakeLists.txt@479
PS3, Line 479: typically
really? I always use enable_devtoolset.sh on RHEL6 via some custom aliases, but I thought that was required, i.e. enable_devtoolset.sh ninja


http://gerrit.cloudera.org:8080/#/c/10428/3/CMakeLists.txt@484
PS3, Line 484: devtoolset-3
> It would be true for all of them. Should I make this a prefix match?
if we must do this, maybe use string MATCHES regex



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib1fae9893aaaf4916205d4c5ae6bb5c93e0505d0
Gerrit-Change-Number: 10428
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 17 May 2018 20:58:39 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2427: adjust gold linker detection

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10428 )

Change subject: KUDU-2427: adjust gold linker detection
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10428/3/CMakeLists.txt
File CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/10428/3/CMakeLists.txt@479
PS3, Line 479: typically
> really? I always use enable_devtoolset.sh on RHEL6 via some custom aliases,
Nope. Technically you only need to run cmake within devtoolset-3; make and ninja needn't use it. See https://kudu.apache.org/docs/installation.html#rhel_from_source for more details.

If make/ninja were also invoked inside devtoolset, this wouldn't be necessary. But I didn't feel comfortable requiring that workflow change, hence the workaround.


http://gerrit.cloudera.org:8080/#/c/10428/3/CMakeLists.txt@484
PS3, Line 484: devtoolset-3
> if we must do this, maybe use string MATCHES regex
Yeah I'll follow Todd's suggestion and use a prefix match via if(MATCHES).



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib1fae9893aaaf4916205d4c5ae6bb5c93e0505d0
Gerrit-Change-Number: 10428
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 17 May 2018 21:59:06 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2427: adjust gold linker detection

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10428 )

Change subject: KUDU-2427: adjust gold linker detection
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10428/3/CMakeLists.txt
File CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/10428/3/CMakeLists.txt@484
PS3, Line 484: devtoolset-3
> is this dts-3 specific? or would also be true of devtoolset-7 for example?
It would be true for all of them. Should I make this a prefix match?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib1fae9893aaaf4916205d4c5ae6bb5c93e0505d0
Gerrit-Change-Number: 10428
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 17 May 2018 17:38:43 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2427: adjust gold linker detection

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10428 )

Change subject: KUDU-2427: adjust gold linker detection
......................................................................


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/10428/3/CMakeLists.txt
File CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/10428/3/CMakeLists.txt@442
PS3, Line 442: to 
> drop
Done


http://gerrit.cloudera.org:8080/#/c/10428/3/CMakeLists.txt@442
PS3, Line 442: extract
> extracts
Done


http://gerrit.cloudera.org:8080/#/c/10428/3/CMakeLists.txt@510
PS3, Line 510: Error: 
> Nit: does it add more clarity for the message?  I saw another error message
Sure, will drop it here.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib1fae9893aaaf4916205d4c5ae6bb5c93e0505d0
Gerrit-Change-Number: 10428
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 17 May 2018 17:41:06 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2427: adjust gold linker detection

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10428 )

Change subject: KUDU-2427: adjust gold linker detection
......................................................................

KUDU-2427: adjust gold linker detection

This patch makes two adjustments to the existing gold linker detection:
1. Ubuntu 18.04's gcc 7 no longer respects a /usr/bin/ld symlink; it is
   hard-wired to use ld.bfd. Instead, we need to pass -fuse-ld=gold to
   choose the gold linker.
2. The old bug that led to tcmalloc being dropped from Kudu's dependency
   list has been fixed, so let's condition the workaround on the appropriate
   version of the gold linker.

Change-Id: Ib1fae9893aaaf4916205d4c5ae6bb5c93e0505d0
Reviewed-on: http://gerrit.cloudera.org:8080/10428
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mp...@apache.org>
---
M CMakeLists.txt
M src/kudu/codegen/CMakeLists.txt
2 files changed, 87 insertions(+), 21 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Mike Percy: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib1fae9893aaaf4916205d4c5ae6bb5c93e0505d0
Gerrit-Change-Number: 10428
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>