[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
..


Patch Set 8:

> (1 comment)

Sorry about that.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
..


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4411/8/bin/save-version.sh
File bin/save-version.sh:

Line 24: VERSION=2.8.0-cdh5-INTERNAL
> Was this the conflict? I think maybe it should be rolled back to 2.7.0.
I agree with Jim. Here's the change that fixes this: 
http://gerrit.cloudera.org:8080/4439


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
..


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4411/8/bin/save-version.sh
File bin/save-version.sh:

Line 24: VERSION=2.8.0-cdh5-INTERNAL
Was this the conflict? I think maybe it should be rolled back to 2.7.0.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
..


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

There was a path conflict during submission. I guess it didn't rebase properly. 
However, there are no build issues and it gave a verified +1. So carrying CR +2 
and Verified +1.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

[Sailesh Mukil (Code Review)]

Sailesh Mukil has submitted this change and it was merged.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
..


Make gen_build_version.py resilient to a failing git rev-parse

It was noticed that some build processes did not checkout Impala and
instead built it from a tarball. This would cause our gen_build_version
script to write a blank version info everytime to the version.info file.

This patch takes care of the case where if there is an already existing
version.info file and we cannot get the git rev-parse output, we use
the old file instead. Blank version info is written only when we don't
have an old version.info file and we cannot do a git rev-parse.

Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Reviewed-on: http://gerrit.cloudera.org:8080/4411
Reviewed-by: Sailesh Mukil 
Tested-by: Sailesh Mukil 
---
M bin/gen_build_version.py
M bin/save-version.sh
2 files changed, 35 insertions(+), 17 deletions(-)

Approvals:
  Sailesh Mukil: Looks good to me, approved; Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

[Sailesh Mukil (Code Review)]

Hello Michael Brown, Lars Volker, Internal Jenkins, Alex Behm,

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

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

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

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
..

Make gen_build_version.py resilient to a failing git rev-parse

It was noticed that some build processes did not checkout Impala and
instead built it from a tarball. This would cause our gen_build_version
script to write a blank version info everytime to the version.info file.

This patch takes care of the case where if there is an already existing
version.info file and we cannot get the git rev-parse output, we use
the old file instead. Blank version info is written only when we don't
have an old version.info file and we cannot do a git rev-parse.

Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
---
M bin/gen_build_version.py
M bin/save-version.sh
2 files changed, 35 insertions(+), 17 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/4411/7
-- 
To view, visit http://gerrit.cloudera.org:8080/4411
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

[Internal Jenkins (Code Review)]

Internal Jenkins has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
..


Patch Set 6: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
..


Patch Set 6: Code-Review+2

Thanks Alex.

Carry +2.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
..


Patch Set 5: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

[Michael Brown (Code Review)]

Michael Brown has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
..


Patch Set 5: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

[Michael Brown (Code Review)]

Michael Brown has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
..


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4411/4/bin/gen_build_version.py
File bin/gen_build_version.py:

PS4, Line 36: with open(os.devnull, 'w') as devnull:
Does this Python script need to support ancient Python 2.4? If yes, with won't 
be supported and this will fail.


http://gerrit.cloudera.org:8080/#/c/4411/4/bin/save-version.sh
File bin/save-version.sh:

PS4, Line 25: GIT_HASH=$(git rev-parse HEAD) 2> /dev/null
"2> /dev/null" should go inside the parens with the command.


PS4, Line 28:   GIT_HASH="Could not obtain git hash"
Has this path been tested? Does Impala work when the derived git hash doesn't 
match the typical pattern of a git hash? It might be safer to echo a warning 
here and make GIT_HASH empty. We know an empty GIT_HASH already won't cause 
Impala to crash or exit. If the point here is "best effort", this seems safer.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4411/3/bin/gen_build_version.py
File bin/gen_build_version.py:

Line 35: # Redirecting stdout and stderr to os.devnull
> nit: You could remove both parameters altogether, since None is the default
Didn't realize this earlier, but None actually doesn't redirect stdout or 
stderr. So it does end up getting printed on stdout/stderr. I've added a change 
to redirect to os.devnull.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4411/3/bin/gen_build_version.py
File bin/gen_build_version.py:

Line 35: can_obtain_git_hash = call(['git', 'rev-parse', 'HEAD'], stdout=None, 
stderr=None) == 0
nit: You could remove both parameters altogether, since None is the default for 
both.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4411/2/bin/gen_build_version.py
File bin/gen_build_version.py:

Line 35: can_obtain_git_hash = call(['git', 'rev-parse', 'HEAD'], stdout=PIPE, 
stderr=PIPE) == 0
Python docs discourage using PIPE here as it can block, depending on the output 
volume of the subprocess (https://docs.python.org/2/library/subprocess.html). 
However, in your case you're not reading the output, so you could just use None 
for both instead, which is also the default.


Line 43:   # SAVE_VERSION_SCRIPT will generate a dummy version.info file if we 
cannot obtain the
Maybe replace L54 with this?


http://gerrit.cloudera.org:8080/#/c/4411/2/bin/save-version.sh
File bin/save-version.sh:

Line 25: GIT_HASH=$(git rev-parse HEAD) > /dev/null
I think this should be 2> /dev/null, no?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4411/1/bin/gen_build_version.py
File bin/gen_build_version.py:

PS1, Line 34: can_obtain_git_hash = False if os.system('git rev-parse HEAD') 
else True
> This took me a second glance to understand, maybe replace with
Done


PS1, Line 39: if not version_file_exists or can_obtain_git_hash:
> what happens if we cannot obtain the git hash and don't have a version file
Done


http://gerrit.cloudera.org:8080/#/c/4411/1/bin/save-version.sh
File bin/save-version.sh:

Line 25: GIT_HASH=$(git rev-parse HEAD)
> Discard stderr?
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#2).

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
..

Make gen_build_version.py resilient to a failing git rev-parse

It was noticed that some build processes did not checkout Impala and
instead built it from a tarball. This would cause our gen_build_version
script to write a blank version info everytime to the version.info file.

This patch takes care of the case where if there is an already existing
version.info file and we cannot get the git rev-parse output, we use
the old file instead. Blank version info is written only when we don't
have an old version.info file and we cannot do a git rev-parse.

Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
---
M bin/gen_build_version.py
M bin/save-version.sh
2 files changed, 27 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/4411/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4411
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Lars Volker