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 2016/09/22 03:07:45 UTC

[kudu-CR] thirdparty: split into dependency groups 

Hello Dan Burkert, Todd Lipcon,

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

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

to review the following change.

Change subject: thirdparty: split into dependency groups
......................................................................

thirdparty: split into dependency groups

The monolithic thirdparty build is now quite a bit larger than it used to be
on account of the extra LLVM build. Let's see if we can't speed it up. The
idea is simple: carve it up into disjoint sections so that individual
sections can be rebuilt as needed.

This patch separates the various portions of the thirdparty build into
"dependency groups". Conceptually, a dependency group is a set of
dependencies built a certain way, but the implementation is really just a
set of non-overlapping code fragments in build-thirdparty.sh.

The initial set of groups are:
- common: dependencies that are never instrumented.
- uninstrumented: dependencies that may be instrumented, but aren't in this
  build.
- instrumented_tsan: dependencies that may be instrumented, and are indeed
  in this build (with -fsanitize=thread).

These three generally map to the existing "installed", "installed-deps", and
"installed-deps-tsan" thirdparty subdirectories. There's an obvious pattern
here for future sanitizer builds (e.g. MSAN would provide an
"instrumented_msan" dependency group).

The new build-if-necessary.sh can accept an argument that maps to a set of
dependency groups representing a particular build. Every dependency group
has its own hash/stamp file so that it is only rebuilt when needed.

This also fixes a bug in the stamp file approach that prevented it from
actually rebuilding anything.

Change-Id: I549262858f98b5ce6c78e786e8c8d8134ba2ed36
---
M CMakeLists.txt
M build-support/jenkins/build-and-test.sh
M thirdparty/.gitignore
M thirdparty/build-if-necessary.sh
M thirdparty/build-thirdparty.sh
5 files changed, 256 insertions(+), 195 deletions(-)


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

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

[kudu-CR] thirdparty: split into dependency groups

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: thirdparty: split into dependency groups
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4513/2/thirdparty/build-thirdparty.sh
File thirdparty/build-thirdparty.sh:

Line 141:     echo TSAN does not work on OS X
> This triggers with a plain "./build-thirdparty.sh" invocation
Oops, will fix.


Line 395: echo "Thirdparty dependencies '$*' built and installed successfully"
> This isn't great with no arguments.
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I549262858f98b5ce6c78e786e8c8d8134ba2ed36
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] thirdparty: split into dependency groups

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

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

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

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

Change subject: thirdparty: split into dependency groups
......................................................................

thirdparty: split into dependency groups

The monolithic thirdparty build is now quite a bit larger than it used to be
on account of the extra LLVM build. Let's see if we can't speed it up. The
idea is simple: carve it up into disjoint sections so that individual
sections can be rebuilt as needed.

This patch separates the various portions of the thirdparty build into
"dependency groups". Conceptually, a dependency group is a set of
dependencies built a certain way, but the implementation is really just a
set of non-overlapping code fragments in build-thirdparty.sh.

The initial set of groups are:
- common: dependencies that are never instrumented.
- uninstrumented: dependencies that may be instrumented, but aren't in this
  build.
- tsan: dependencies that may be instrumented, and are indeed in this build
  (with -fsanitize=thread).

These three generally map to the existing "common", "uninstrumented", and
"tsan" thirdparty subdirectories. There's an obvious pattern here for future
sanitizer builds (e.g. MSAN would provide an "msan" dependency group).

The new build-if-necessary.sh can accept an argument that maps to a set of
dependency groups representing a particular build. Every dependency group
has its own hash/stamp file so that it is only rebuilt when needed.

This also fixes a bug in the stamp file approach that prevented it from
actually rebuilding anything.

Change-Id: I549262858f98b5ce6c78e786e8c8d8134ba2ed36
---
M CMakeLists.txt
M build-support/jenkins/build-and-test.sh
M thirdparty/.gitignore
M thirdparty/build-if-necessary.sh
M thirdparty/build-thirdparty.sh
5 files changed, 260 insertions(+), 202 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I549262858f98b5ce6c78e786e8c8d8134ba2ed36
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] thirdparty: split into dependency groups

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: thirdparty: split into dependency groups
......................................................................


Patch Set 1:

(3 comments)

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

Line 29: "instrumented_msan" dependency group).
Will this actually work for msan, though?  My understanding is that msan requires _all_ dependencies to be instrumented, which would result in a "dependency group" all on it's own, with no dependency on the common group.  Maybe that's fine?


http://gerrit.cloudera.org:8080/#/c/4513/1/thirdparty/build-if-necessary.sh
File thirdparty/build-if-necessary.sh:

PS1, Line 86: #
leftover


http://gerrit.cloudera.org:8080/#/c/4513/1/thirdparty/build-thirdparty.sh
File thirdparty/build-thirdparty.sh:

Line 363:   build_protobuf
hmm.  I think what this is saying, is that protobuf (and the others below) get built in tsan mode, whenever you call build-thirdparty.sh protobuf.  Thats great in theory (I think you asked me the other day and I said I supported that), but it seems to me that if you start off with a clean thirdparty and call 'build-thirdparty.sh protobuf' it's going to try and build an instrumented protobuf without having built an instrumented libc++ to link it against.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I549262858f98b5ce6c78e786e8c8d8134ba2ed36
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] thirdparty: split into dependency groups

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: thirdparty: split into dependency groups
......................................................................


Patch Set 1:

(4 comments)

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

Line 29: "instrumented_msan" dependency group).
> Will this actually work for msan, though?  My understanding is that msan re
You're probably right. I'm working on a follow-up patch that rebuilds the C dependencies for TSAN too.


http://gerrit.cloudera.org:8080/#/c/4513/1/thirdparty/build-if-necessary.sh
File thirdparty/build-if-necessary.sh:

PS1, Line 86: #
> leftover
Done


Line 91:       CHANGED_FILE_COUNT=$(find . -cnewer $STAMP_FILE | wc -l)
> Consider adding '-type f' into the find options.  If directory (especially 
I don't think it matters much either way. This case is so very rare (basically for developers who are annoyed at thirdparty rebuilds when testing out of a tarball install) that I don't think we need to optimize for a tarball whose contents actually changes.


http://gerrit.cloudera.org:8080/#/c/4513/1/thirdparty/build-thirdparty.sh
File thirdparty/build-thirdparty.sh:

Line 363:   build_protobuf
> hmm.  I think what this is saying, is that protobuf (and the others below) 
Yes, that's true, and it is something of a functional regression from before. But, I also think it's such a rarely used feature that it's not a huge deal either way. I only use "build-thirdparty.sh <lib>" on a fully-formed thirdparty tree when I want to rebuild a single library.

We can certainly revisit this if there are complaints; it's relatively easy to adjust.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I549262858f98b5ce6c78e786e8c8d8134ba2ed36
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] thirdparty: split into dependency groups

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: thirdparty: split into dependency groups
......................................................................


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I549262858f98b5ce6c78e786e8c8d8134ba2ed36
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] thirdparty: split into dependency groups

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: thirdparty: split into dependency groups
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4513/2/thirdparty/build-thirdparty.sh
File thirdparty/build-thirdparty.sh:

Line 141:     echo TSAN does not work on OS X
This triggers with a plain "./build-thirdparty.sh" invocation


Line 395: echo "Thirdparty dependencies '$*' built and installed successfully"
This isn't great with no arguments.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I549262858f98b5ce6c78e786e8c8d8134ba2ed36
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] thirdparty: split into dependency groups

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: thirdparty: split into dependency groups
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4513/1/thirdparty/build-if-necessary.sh
File thirdparty/build-if-necessary.sh:

Line 91:       CHANGED_FILE_COUNT=$(find . -cnewer $STAMP_FILE | wc -l)
Consider adding '-type f' into the find options.  If directory (especially which is not under the version control) appeared or get some unrelated files, we don't want to rebuild, right?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I549262858f98b5ce6c78e786e8c8d8134ba2ed36
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] thirdparty: split into dependency groups

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

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

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

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

Change subject: thirdparty: split into dependency groups
......................................................................

thirdparty: split into dependency groups

The monolithic thirdparty build is now quite a bit larger than it used to be
on account of the extra LLVM build. Let's see if we can't speed it up. The
idea is simple: carve it up into disjoint sections so that individual
sections can be rebuilt as needed.

This patch separates the various portions of the thirdparty build into
"dependency groups". Conceptually, a dependency group is a set of
dependencies built a certain way, but the implementation is really just a
set of non-overlapping code fragments in build-thirdparty.sh.

The initial set of groups are:
- common: dependencies that are never instrumented.
- uninstrumented: dependencies that may be instrumented, but aren't in this
  build.
- tsan: dependencies that may be instrumented, and are indeed in this build
  (with -fsanitize=thread).

These three generally map to the existing "common", "uninstrumented", and
"tsan" thirdparty subdirectories. There's an obvious pattern here for future
sanitizer builds (e.g. MSAN would provide an "msan" dependency group).

The new build-if-necessary.sh can accept an argument that maps to a set of
dependency groups representing a particular build. Every dependency group
has its own hash/stamp file so that it is only rebuilt when needed.

This also fixes a bug in the stamp file approach that prevented it from
actually rebuilding anything.

Change-Id: I549262858f98b5ce6c78e786e8c8d8134ba2ed36
---
M CMakeLists.txt
M build-support/jenkins/build-and-test.sh
M thirdparty/.gitignore
M thirdparty/build-if-necessary.sh
M thirdparty/build-thirdparty.sh
5 files changed, 271 insertions(+), 202 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/4513/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4513
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I549262858f98b5ce6c78e786e8c8d8134ba2ed36
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] thirdparty: split into dependency groups

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

Change subject: thirdparty: split into dependency groups
......................................................................


thirdparty: split into dependency groups

The monolithic thirdparty build is now quite a bit larger than it used to be
on account of the extra LLVM build. Let's see if we can't speed it up. The
idea is simple: carve it up into disjoint sections so that individual
sections can be rebuilt as needed.

This patch separates the various portions of the thirdparty build into
"dependency groups". Conceptually, a dependency group is a set of
dependencies built a certain way, but the implementation is really just a
set of non-overlapping code fragments in build-thirdparty.sh.

The initial set of groups are:
- common: dependencies that are never instrumented.
- uninstrumented: dependencies that may be instrumented, but aren't in this
  build.
- tsan: dependencies that may be instrumented, and are indeed in this build
  (with -fsanitize=thread).

These three generally map to the existing "common", "uninstrumented", and
"tsan" thirdparty subdirectories. There's an obvious pattern here for future
sanitizer builds (e.g. MSAN would provide an "msan" dependency group).

The new build-if-necessary.sh can accept an argument that maps to a set of
dependency groups representing a particular build. Every dependency group
has its own hash/stamp file so that it is only rebuilt when needed.

This also fixes a bug in the stamp file approach that prevented it from
actually rebuilding anything.

Change-Id: I549262858f98b5ce6c78e786e8c8d8134ba2ed36
Reviewed-on: http://gerrit.cloudera.org:8080/4513
Reviewed-by: Dan Burkert <da...@cloudera.com>
Tested-by: Dan Burkert <da...@cloudera.com>
---
M CMakeLists.txt
M build-support/jenkins/build-and-test.sh
M thirdparty/.gitignore
M thirdparty/build-if-necessary.sh
M thirdparty/build-thirdparty.sh
5 files changed, 271 insertions(+), 202 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved; Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I549262858f98b5ce6c78e786e8c8d8134ba2ed36
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] thirdparty: split into dependency groups

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: thirdparty: split into dependency groups
......................................................................


Patch Set 4: Code-Review+2 Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I549262858f98b5ce6c78e786e8c8d8134ba2ed36
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No