You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Grant Henke (Code Review)" <ge...@cloudera.org> on 2019/01/31 18:30:14 UTC
[kudu-CR] [build] Add a script to publish the kudu-binary jars
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12325
Change subject: [build] Add a script to publish the kudu-binary jars
......................................................................
[build] Add a script to publish the kudu-binary jars
Adds publish_mini_cluster_binaries.sh to publish the
kudu-binary jars. This script can install to the local
maven repository or deploy to a remote repsitory.
I also moved the files into a mini-cluster directory and
made sure the naming of mini-cluster was consistent.
Change-Id: I83fe6b81ef70ec75b10f66fa8582a861daf285e4
---
R build-support/mini-cluster/build_mini_cluster_binaries.sh
A build-support/mini-cluster/publish_mini_cluster_binaries.sh
R build-support/mini-cluster/relocate_binaries_for_mini_cluster.py
A build-support/mini-cluster/settings.xml
4 files changed, 239 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/12325/1
--
To view, visit http://gerrit.cloudera.org:8080/12325
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I83fe6b81ef70ec75b10f66fa8582a861daf285e4
Gerrit-Change-Number: 12325
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
[kudu-CR] [build] Add a script to publish the kudu-binary jars
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12325 )
Change subject: [build] Add a script to publish the kudu-binary jars
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12325/2/build-support/mini-cluster/publish_mini_cluster_binaries.sh
File build-support/mini-cluster/publish_mini_cluster_binaries.sh:
http://gerrit.cloudera.org:8080/#/c/12325/2/build-support/mini-cluster/publish_mini_cluster_binaries.sh@116
PS2, Line 116: PROP_FILE="META-INF/apache-kudu-test-binary.properties"
> May want to pipe stderr/stdout into /dev/null; we don't need to see the res
When I run this I don't see the result in stdout. I suppose it would print if `which` wasn't found. But I think that's worth printing as we expect every dev environment to have `which`.
http://gerrit.cloudera.org:8080/#/c/12325/2/build-support/mini-cluster/publish_mini_cluster_binaries.sh@202
PS2, Line 202:
:
:
:
> Maybe we should do this before publishing anything, so that if it fails, we
I don't that's worth the script complexity. On the very rare case this happens a partial upload is okay.
Given these jars need to be generated for multiple operating systems, we will likely be uploading the jars to the staging repository one at a time regardless.
In the future Docker may generate all the jars, but the OSX jar still needs to be created on a physical machine.
--
To view, visit http://gerrit.cloudera.org:8080/12325
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I83fe6b81ef70ec75b10f66fa8582a861daf285e4
Gerrit-Change-Number: 12325
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Brian McDevitt <br...@phdata.io>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 01 Feb 2019 17:06:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] [build] Add a script to publish the kudu-binary jars
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12325 )
Change subject: [build] Add a script to publish the kudu-binary jars
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh
File build-support/mini-cluster/publish_mini_cluster_binaries.sh:
http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@67
PS1, Line 67: # Parse the command line parameters.
> Enhanced getopt requires `brew install gnu-getopt` on Mac. Given this is a
macOS continues to disappoint. I agree with your conclusion.
http://gerrit.cloudera.org:8080/#/c/12325/2/build-support/mini-cluster/publish_mini_cluster_binaries.sh
File build-support/mini-cluster/publish_mini_cluster_binaries.sh:
http://gerrit.cloudera.org:8080/#/c/12325/2/build-support/mini-cluster/publish_mini_cluster_binaries.sh@116
PS2, Line 116: if [[ -z $(which mvn) ]]; then
May want to pipe stderr/stdout into /dev/null; we don't need to see the result in stdout.
http://gerrit.cloudera.org:8080/#/c/12325/2/build-support/mini-cluster/publish_mini_cluster_binaries.sh@202
PS2, Line 202: if [[ "$JAR_VERSION" != "$VERSION" ]]; then
: echo "The version ($VERSION) doesn't match the jar's artifact.version property ($JAR_VERSION)"
: exit 1
: fi
Maybe we should do this before publishing anything, so that if it fails, we won't have a partial publish?
--
To view, visit http://gerrit.cloudera.org:8080/12325
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I83fe6b81ef70ec75b10f66fa8582a861daf285e4
Gerrit-Change-Number: 12325
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Brian McDevitt <br...@phdata.io>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 31 Jan 2019 23:29:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] [build] Add a script to publish the kudu-binary jars
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12325 )
Change subject: [build] Add a script to publish the kudu-binary jars
......................................................................
[build] Add a script to publish the kudu-binary jars
Adds publish_mini_cluster_binaries.sh to publish the
kudu-binary jars. This script can install to the local
maven repository or deploy to a remote repsitory.
I also moved the files into a mini-cluster directory and
made sure the naming of mini-cluster was consistent.
Change-Id: I83fe6b81ef70ec75b10f66fa8582a861daf285e4
Reviewed-on: http://gerrit.cloudera.org:8080/12325
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
R build-support/mini-cluster/build_mini_cluster_binaries.sh
A build-support/mini-cluster/publish_mini_cluster_binaries.sh
R build-support/mini-cluster/relocate_binaries_for_mini_cluster.py
A build-support/mini-cluster/settings.xml
4 files changed, 254 insertions(+), 6 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/12325
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I83fe6b81ef70ec75b10f66fa8582a861daf285e4
Gerrit-Change-Number: 12325
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Brian McDevitt <br...@phdata.io>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [build] Add a script to publish the kudu-binary jars
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12325 )
Change subject: [build] Add a script to publish the kudu-binary jars
......................................................................
Patch Set 1:
I published a Mac and Linux jar to the SNAPSHOT repo here:
https://repository.apache.org/content/repositories/snapshots/org/apache/kudu/kudu-binary/1.9.0-SNAPSHOT/
Note: The jars themselves have some issues to workout yet.
--
To view, visit http://gerrit.cloudera.org:8080/12325
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I83fe6b81ef70ec75b10f66fa8582a861daf285e4
Gerrit-Change-Number: 12325
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Brian McDevitt <br...@phdata.io>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 31 Jan 2019 18:35:53 +0000
Gerrit-HasComments: No
[kudu-CR] [build] Add a script to publish the kudu-binary jars
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12325 )
Change subject: [build] Add a script to publish the kudu-binary jars
......................................................................
Patch Set 1:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh
File build-support/mini-cluster/publish_mini_cluster_binaries.sh:
http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@59
PS1, Line 59: VERSION=`cat ${SOURCE_ROOT}/version.txt`
> New bash scripts should prefer $(...) for command substitution over backtic
Done
http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@67
PS1, Line 67: # Parse the command line parameters.
> I'd recommend using getopt for this; it's the most idiomatic way to do comp
Enhanced getopt requires `brew install gnu-getopt` on Mac. Given this is a rarely run release only script I though this simplified parsing was appropriate.
http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@102
PS1, Line 102: exit 1
> Nit: indentation
Done
http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@105
PS1, Line 105: if [[ -z ${MVN_USERNAME} ]]; then
> Curious why you're using braces here but not above?
Done
http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@132
PS1, Line 132: local PROP=`unzip -q -c ${JAR} ${PROP_FILE} | grep "$KEY" | cut -d'=' -f2`
> Validate that the JAR exists first, and provide a nice error message if it
Done
http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@145
PS1, Line 145: mvn install:install-file ${@}
> Maybe validate that 'mvn' is on the path up front, and provide a nice error
Done
http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@160
PS1, Line 160: # Create a minimal POM file.
> Nit: indentation.
Done
http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@162
PS1, Line 162: rm -f ${POM_FILE}
> Not necessary given the non-append redirection below.
Done
http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@182
PS1, Line 182: MVN_ARGS="$MVN_ARGS -Dfile=$POM_FILE"
: MVN_ARGS="$MVN_ARGS -Dpackaging=pom"
> Won't this leak into the JAR publishing done below? Is that intentional?
Done
http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@191
PS1, Line 191: JAR_VERSION=`read_prop_or_die "$JAR" "artifact.version"`
> This isn't used.
Done
--
To view, visit http://gerrit.cloudera.org:8080/12325
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I83fe6b81ef70ec75b10f66fa8582a861daf285e4
Gerrit-Change-Number: 12325
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Brian McDevitt <br...@phdata.io>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 31 Jan 2019 20:04:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] [build] Add a script to publish the kudu-binary jars
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12325 )
Change subject: [build] Add a script to publish the kudu-binary jars
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12325/2/build-support/mini-cluster/publish_mini_cluster_binaries.sh
File build-support/mini-cluster/publish_mini_cluster_binaries.sh:
http://gerrit.cloudera.org:8080/#/c/12325/2/build-support/mini-cluster/publish_mini_cluster_binaries.sh@116
PS2, Line 116: if [[ -z $(which mvn) ]]; then
> When I run this I don't see the result in stdout. I suppose it would print
So on Ubuntu 18 here's what I see:
$ which doesnotexist
$ which ls
/bin/ls
$ which ls 2>/dev/null
/bin/ls
In short:
- Running `which` on a non-existent binary prints nothing but exits with a non-zero status.
- Running `which` on an existent binary prints the binary's path to stdout.
I'm not concerned about `which` itself not existing; it should be standard on just about every UNIX system. It's disappointing (again) that macOS and Linux have opposite behaviors with respect to printing.
--
To view, visit http://gerrit.cloudera.org:8080/12325
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I83fe6b81ef70ec75b10f66fa8582a861daf285e4
Gerrit-Change-Number: 12325
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Brian McDevitt <br...@phdata.io>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 01 Feb 2019 19:26:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] [build] Add a script to publish the kudu-binary jars
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Brian McDevitt, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12325
to look at the new patch set (#2).
Change subject: [build] Add a script to publish the kudu-binary jars
......................................................................
[build] Add a script to publish the kudu-binary jars
Adds publish_mini_cluster_binaries.sh to publish the
kudu-binary jars. This script can install to the local
maven repository or deploy to a remote repsitory.
I also moved the files into a mini-cluster directory and
made sure the naming of mini-cluster was consistent.
Change-Id: I83fe6b81ef70ec75b10f66fa8582a861daf285e4
---
R build-support/mini-cluster/build_mini_cluster_binaries.sh
A build-support/mini-cluster/publish_mini_cluster_binaries.sh
R build-support/mini-cluster/relocate_binaries_for_mini_cluster.py
A build-support/mini-cluster/settings.xml
4 files changed, 254 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/12325/2
--
To view, visit http://gerrit.cloudera.org:8080/12325
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I83fe6b81ef70ec75b10f66fa8582a861daf285e4
Gerrit-Change-Number: 12325
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Brian McDevitt <br...@phdata.io>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [build] Add a script to publish the kudu-binary jars
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12325 )
Change subject: [build] Add a script to publish the kudu-binary jars
......................................................................
Patch Set 1:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh
File build-support/mini-cluster/publish_mini_cluster_binaries.sh:
http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@59
PS1, Line 59: VERSION=`cat ${SOURCE_ROOT}/version.txt`
New bash scripts should prefer $(...) for command substitution over backticks.
Below too.
http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@67
PS1, Line 67: # Parse the command line parameters.
I'd recommend using getopt for this; it's the most idiomatic way to do complex parameter parsing in shell. I presume it's portable to macOS?
Here's a decent example of how it works: https://gist.github.com/madx/2767550
http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@102
PS1, Line 102: exit 1
Nit: indentation
http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@105
PS1, Line 105: if [[ -z ${MVN_USERNAME} ]]; then
Curious why you're using braces here but not above?
Also, do these expansions need to be surrounded by quotes? If not, what happens when they're not set? Doesn't bash wind up evaluating something looking like "if [[ -z ]]; then"?
http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@132
PS1, Line 132: local PROP=`unzip -q -c ${JAR} ${PROP_FILE} | grep "$KEY" | cut -d'=' -f2`
Validate that the JAR exists first, and provide a nice error message if it doesn't?
http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@145
PS1, Line 145: mvn install:install-file ${@}
Maybe validate that 'mvn' is on the path up front, and provide a nice error message if it's not?
http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@160
PS1, Line 160: # Create a minimal POM file.
Nit: indentation.
http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@162
PS1, Line 162: rm -f ${POM_FILE}
Not necessary given the non-append redirection below.
http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@182
PS1, Line 182: MVN_ARGS="$MVN_ARGS -Dfile=$POM_FILE"
: MVN_ARGS="$MVN_ARGS -Dpackaging=pom"
Won't this leak into the JAR publishing done below? Is that intentional?
http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@191
PS1, Line 191: JAR_VERSION=`read_prop_or_die "$JAR" "artifact.version"`
This isn't used.
--
To view, visit http://gerrit.cloudera.org:8080/12325
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I83fe6b81ef70ec75b10f66fa8582a861daf285e4
Gerrit-Change-Number: 12325
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Brian McDevitt <br...@phdata.io>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 31 Jan 2019 19:16:09 +0000
Gerrit-HasComments: Yes