You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2017/09/12 21:41:48 UTC
[Impala-ASF-CR] IMPALA-5905: add script for all-build-options job
Tim Armstrong has uploaded a new change for review.
http://gerrit.cloudera.org:8080/8043
Change subject: IMPALA-5905: add script for all-build-options job
......................................................................
IMPALA-5905: add script for all-build-options job
This checks in a modified version of the job script for
https://jenkins.impala.io/view/Experimental/job/all-build-options
which adds UBSAN and TSAN.
The script is also modified to not reference any jenkins environment
variables, e.g. WORKSPACE, since the Jenkins job script intermingled
references to those with the script logic.
Change-Id: I6e78f05c41e3ccd59af599b00e453e7f88b2bb34
---
A bin/all-build-options.sh
1 file changed, 57 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/8043/1
--
To view, visit http://gerrit.cloudera.org:8080/8043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6e78f05c41e3ccd59af599b00e453e7f88b2bb34
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5905: add script for all-build-options job
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-5905: add script for all-build-options job
......................................................................
Patch Set 2: Code-Review+2
(3 comments)
Can you add a note about ccache and ninja?
http://gerrit.cloudera.org:8080/#/c/8043/1/bin/all-build-options.sh
File bin/all-build-options.sh:
Line 1
> Yeah the jenkins/ subdirectory makes sense to me, there's a lot of junk in
I named the Jenkins job "all-build-options". I'm OK with a name change of any sort, or leaving it the same. Maybe "build-with-all-flag-combinations.sh"?
Line 21
Required the ninja build system and ccache to be installed, which are not strictly build requirements.
PS1, Line 35:
:
:
:
:
:
:
> I just preserved this logic from the original Jenkins job script. I didn't
One difference: if clean.sh fails in the call to buildall, that call fails but we continue to test more build options. This way, if we can't clean, we just dies since all the remaining calls to buildall.sh should also fail but will provide no additional information.
--
To view, visit http://gerrit.cloudera.org:8080/8043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6e78f05c41e3ccd59af599b00e453e7f88b2bb34
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5905: add script for all-build-options job
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5905: add script for all-build-options job
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8043/1/bin/all-build-options.sh
File bin/all-build-options.sh:
Line 1: #!/usr/bin/env bash
> I named the Jenkins job "all-build-options". I'm OK with a name change of a
Done
Line 21: # succeeds. Intended for use as a precommit test to make sure nothing got broken.
> Required the ninja build system and ccache to be installed, which are not s
Done
--
To view, visit http://gerrit.cloudera.org:8080/8043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6e78f05c41e3ccd59af599b00e453e7f88b2bb34
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5905: add script for all-build-options job
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has posted comments on this change.
Change subject: IMPALA-5905: add script for all-build-options job
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8043/1/bin/all-build-options.sh
File bin/all-build-options.sh:
Line 1: #!/usr/bin/env bash
Nit: I don't like the file name. all-build-options.sh sounds like it's something that spits out the build options, rather than something that actually then builds using all those options. "build-with-all-options.sh" is closer to what this does.
On the other hand, if we have a directory for Jenkins jobs, and the script names are one to one with job names, I have no problem with this name. As a new developer, though, figuring out what's what in bin/ is already a challenge.
PS1, Line 35: do
: OPTIONS[4]=$BUILD_SHARED_LIBS
: if ! ./bin/clean.sh
: then
: echo "Clean failed"
: exit 1
: fi
You could remove noclean in line 26 and remove this. It's obviously not exactly the same, but it's supposed to be!
--
To view, visit http://gerrit.cloudera.org:8080/8043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6e78f05c41e3ccd59af599b00e453e7f88b2bb34
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5905: add script for all-build-options job
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5905: add script for all-build-options job
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8043/1/bin/all-build-options.sh
File bin/all-build-options.sh:
Line 1: #!/usr/bin/env bash
> Nit: I don't like the file name. all-build-options.sh sounds like it's some
Yeah the jenkins/ subdirectory makes sense to me, there's a lot of junk in bin/
PS1, Line 35: do
: OPTIONS[4]=$BUILD_SHARED_LIBS
: if ! ./bin/clean.sh
: then
: echo "Clean failed"
: exit 1
: fi
> You could remove noclean in line 26 and remove this. It's obviously not exa
I just preserved this logic from the original Jenkins job script. I didn't add this so I don't know if there was some deeper motivation or it.
I could probably try to clean this and other things up but I'd prefer to check this in with the minimal changes required to make it work then worry about cleanup later.
--
To view, visit http://gerrit.cloudera.org:8080/8043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6e78f05c41e3ccd59af599b00e453e7f88b2bb34
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5905: add script for all-build-options job
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has submitted this change and it was merged.
Change subject: IMPALA-5905: add script for all-build-options job
......................................................................
IMPALA-5905: add script for all-build-options job
This checks in a modified version of the job script for
https://jenkins.impala.io/view/Experimental/job/all-build-options
which adds UBSAN and TSAN.
The script is also modified to not reference any jenkins environment
variables, e.g. WORKSPACE, since the Jenkins job script intermingled
references to those with the script logic.
Change-Id: I6e78f05c41e3ccd59af599b00e453e7f88b2bb34
Reviewed-on: http://gerrit.cloudera.org:8080/8043
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Tim Armstrong <ta...@cloudera.com>
---
A bin/jenkins/build-all-flag-combinations.sh
1 file changed, 59 insertions(+), 0 deletions(-)
Approvals:
Tim Armstrong: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/8043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I6e78f05c41e3ccd59af599b00e453e7f88b2bb34
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5905: add script for all-build-options job
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has uploaded a new patch set (#2).
Change subject: IMPALA-5905: add script for all-build-options job
......................................................................
IMPALA-5905: add script for all-build-options job
This checks in a modified version of the job script for
https://jenkins.impala.io/view/Experimental/job/all-build-options
which adds UBSAN and TSAN.
The script is also modified to not reference any jenkins environment
variables, e.g. WORKSPACE, since the Jenkins job script intermingled
references to those with the script logic.
Change-Id: I6e78f05c41e3ccd59af599b00e453e7f88b2bb34
---
A bin/jenkins/all-build-options.sh
1 file changed, 57 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/8043/2
--
To view, visit http://gerrit.cloudera.org:8080/8043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6e78f05c41e3ccd59af599b00e453e7f88b2bb34
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5905: add script for all-build-options job
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Jim Apple,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8043
to look at the new patch set (#3).
Change subject: IMPALA-5905: add script for all-build-options job
......................................................................
IMPALA-5905: add script for all-build-options job
This checks in a modified version of the job script for
https://jenkins.impala.io/view/Experimental/job/all-build-options
which adds UBSAN and TSAN.
The script is also modified to not reference any jenkins environment
variables, e.g. WORKSPACE, since the Jenkins job script intermingled
references to those with the script logic.
Change-Id: I6e78f05c41e3ccd59af599b00e453e7f88b2bb34
---
A bin/jenkins/build-all-flag-combinations.sh
1 file changed, 59 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/8043/3
--
To view, visit http://gerrit.cloudera.org:8080/8043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6e78f05c41e3ccd59af599b00e453e7f88b2bb34
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5905: add script for all-build-options job
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5905: add script for all-build-options job
......................................................................
Patch Set 3: Code-Review+2 Verified+1
Carry +2. Verified manually by running locally (no point in running the merge job since it doesn't execute this script yet).
--
To view, visit http://gerrit.cloudera.org:8080/8043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6e78f05c41e3ccd59af599b00e453e7f88b2bb34
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No