Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
gerrit-hooks has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. .. Patch Set 22: * Update tracker: IGNORE, no Bug-Url found * Set MODIFIED::IGNORE, no Bug-Url found. -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 22 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Dan Kenigsberg has submitted this change and it was merged. Change subject: tox: fail make process if required tox version isn't installed. .. tox: fail make process if required tox version isn't installed. Over RHEL 7.2 yum contains only tox-1.4.2 which doesn't fit to vdsm requirements. We require to install tox with pip currently. This patch validates tox and its version and fails the make process if tox is older than 2.1.1 tox check was removed from configure.ac and now it's existence will be checked only during the build process. Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Signed-off-by: Irit Goihman Reviewed-on: https://gerrit.ovirt.org/59306 Continuous-Integration: Jenkins CI Reviewed-by: Nir Soffer Reviewed-by: Yaniv Bronhaim Reviewed-by: Dan Kenigsberg --- M Makefile.am A build-aux/vercmp M configure.ac M tox.sh 4 files changed, 92 insertions(+), 18 deletions(-) Approvals: Nir Soffer: Looks good to me, but someone else must approve Yaniv Bronhaim: Looks good to me, but someone else must approve Jenkins CI: Passed CI tests Irit Goihman: Verified Dan Kenigsberg: Looks good to me, approved -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: merged Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 22 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Dan Kenigsberg has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. .. Patch Set 21: Code-Review+2 raising -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 21 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Yaniv Bronhaim has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. .. Patch Set 21: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 21 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Nir Soffer has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. .. Patch Set 21: Code-Review+1 Piotr, can you ack this? -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 21 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Irit Goihman has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. .. Patch Set 21: > Irit, did you address the comments in version 7? version 7 is very old, are you sure you meant that version? -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 21 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Irit Goihman has posted comments on this change.
Change subject: tox: fail make process if required tox version isn't installed.
..
Patch Set 21: Verified+1
(7 comments)
https://gerrit.ovirt.org/#/c/59306/20/Makefile.am
File Makefile.am:
Line 93
Line 94
Line 95
Line 96
Line 97
> I think we can simplify and make it more clear like this:
Done
https://gerrit.ovirt.org/#/c/59306/17/build-aux/vercmp
File build-aux/vercmp:
Line 26:
Line 27:
Line 28: def main(args):
Line 29: args = parse_args()
Line 30: if args.verbose:
> You want to setup logging always. --verbose should effect only the log leve
Done
Line 31: level = logging.DEBUG
Line 32: else:
Line 33: level = logging.INFO
Line 34:
Line 28: def main(args):
Line 29: args = parse_args()
Line 30: if args.verbose:
Line 31: level = logging.DEBUG
Line 32: else:
> Better exit in __main__, and return a value here, so we can test this funct
Done
Line 33: level = logging.INFO
Line 34:
Line 35: logging.basicConfig(level=level, format="%(name)s: %(message)s")
Line 36: return compare_versions(args.actual_version, args.required_version)
Line 40: actual_version = [int(n) for n in actual_version.split('.')]
Line 41: required_version = [int(n) for n in required_version.split('.')]
Line 42:
Line 43: padding = len(actual_version) - len(required_version)
Line 44: if padding > 0:
> This will use the root logger, so we will see this output:
Done
Line 45: required_version += [0] * padding
Line 46:
Line 47: if actual_version < required_version:
Line 48: log.debug("%s < %s", actual_version, required_version)
https://gerrit.ovirt.org/#/c/59306/19/build-aux/vercmp
File build-aux/vercmp:
Line 44: if padding > 0:
Line 45: required_version += [0] * padding
Line 46:
Line 47: if actual_version < required_version:
Line 48: log.debug("%s < %s", actual_version, required_version)
> You are not using the logger, so the output is:
Done
Line 49: return 0
Line 50: elif actual_version == required_version:
Line 51: log.debug("%s == %s", actual_version, required_version)
Line 52: return 1
https://gerrit.ovirt.org/#/c/59306/20/build-aux/vercmp
File build-aux/vercmp:
Line 39: def compare_versions(actual_version, required_version):
Line 40: actual_version = [int(n) for n in actual_version.split('.')]
Line 41: required_version = [int(n) for n in required_version.split('.')]
Line 42:
Line 43: padding = len(actual_version) - len(required_version)
> This should be:
Done
Line 44: if padding > 0:
Line 45: required_version += [0] * padding
Line 46:
Line 47: if actual_version < required_version:
https://gerrit.ovirt.org/#/c/59306/17/tox.sh
File tox.sh:
Line 1: #!/bin/sh -e
Line 2:
Line 3: WHITELIST=(build-aux/vercmp \
Line 4:contrib/logdb \
> We use only spaces in scripts, this is not a makefile.
Done
Line 5:contrib/logstat \
Line 6:contrib/profile-stats \
Line 7:init/daemonAdapter \
Line 8:vdsm/get-conf-item \
--
To view, visit https://gerrit.ovirt.org/59306
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c
Gerrit-PatchSet: 21
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Irit Goihman
Gerrit-Reviewer: Edward Haas
Gerrit-Reviewer: Irit Goihman
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Nir Soffer has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. .. Patch Set 21: Irit, did you address the comments in version 7? -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 21 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
gerrit-hooks has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. .. Patch Set 21: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0']) -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 21 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Nir Soffer has posted comments on this change.
Change subject: tox: fail make process if required tox version isn't installed.
..
Patch Set 20:
(7 comments)
https://gerrit.ovirt.org/#/c/59306/20/Makefile.am
File Makefile.am:
Line 93
Line 94
Line 95
Line 96
Line 97
I think we can simplify and make it more clear like this:
out=`tox --version`; \
if [ $$? -ne 0 ]; then \
echo "Error: cannot run tox, please install tox $(TOX_MIN_VERSION) or
later"; \
exit 1; \
fi; \
version=`echo $$out | cut -d' ' -f1`; \
if build-aux/vercmp $$version $(TOX_MIN_VERSION); then \
echo "Error: tox is too old, please install tox $(TOX_MIN_VERSION) or
later"; \
exit 1; \
fi
Line 91:tox -- pep8
Line 92:
Line 93: .PHONY: tox
Line 94: tox:
Line 95:if [ `command -v tox` >/dev/null 2>&1 ]; then \
This will run tox on the machine, running all the tests defined in tox.ini -
very bad idea for checking if tox is installed.
Line 96:if build-aux/vercmp `tox --version | cut -d' ' -f1` \
Line 97:"$(TOX_MIN_VERSION)"; \
Line 98:then \
Line 99:echo "Error: tox is too old please install tox \
Line 93: .PHONY: tox
Line 94: tox:
Line 95:if [ `command -v tox` >/dev/null 2>&1 ]; then \
Line 96:if build-aux/vercmp `tox --version | cut -d' ' -f1` \
Line 97:"$(TOX_MIN_VERSION)"; \
it is very hard to read such code, specially inside a makefile.
Line 98:then \
Line 99:echo "Error: tox is too old please install tox \
Line 100: $(TOX_MIN_VERSION) or later"; \
Line 101: exit 1; \
Line 95:if [ `command -v tox` >/dev/null 2>&1 ]; then \
Line 96:if build-aux/vercmp `tox --version | cut -d' ' -f1` \
Line 97:"$(TOX_MIN_VERSION)"; \
Line 98:then \
Line 99:echo "Error: tox is too old please install tox \
We need some punctuation here
Line 100: $(TOX_MIN_VERSION) or later"; \
Line 101: exit 1; \
Line 102: fi \
Line 103: else \
Line 100: $(TOX_MIN_VERSION) or later"; \
Line 101: exit 1; \
Line 102: fi \
Line 103: else \
Line 104: echo "Error: please install tox $(TOX_MIN_VERSION) or
later.";\
The trailing period is not needed.
Line 105: exit 1; \
Line 106: fi
Line 107:
Line 108:
Line 104: echo "Error: please install tox $(TOX_MIN_VERSION) or
later.";\
Line 105: exit 1; \
Line 106: fi
Line 107:
Line 108:
Unneeded
Line 109: .PHONY: python3
Line 110: python3: all
Line 111: if [ "$(PYTHON3_SUPPORT)" == "1" ]; then \
Line 112: PYTHONDONTWRITEBYTECODE=1 $(PYTHON3) -m compileall \
https://gerrit.ovirt.org/#/c/59306/20/build-aux/vercmp
File build-aux/vercmp:
Line 39: def compare_versions(actual_version, required_version):
Line 40: actual_version = [int(n) for n in actual_version.split('.')]
Line 41: required_version = [int(n) for n in required_version.split('.')]
Line 42:
Line 43: padding = len(required_version) < len(actual_version)
This should be:
padding = len(actual_version) - len(required_version)
Current code never adds only one 0:
# build-aux/vercmp -v 0.1.0.0 0.1
vercmp: [0, 1, 0, 0] > [0, 1, 0]
This should be:
vercmp: [0, 1, 0, 0] == [0, 1, 0, 0]
Line 44: if padding > 0:
Line 45: required_version += [0] * padding
Line 46:
Line 47: if actual_version < required_version:
--
To view, visit https://gerrit.ovirt.org/59306
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c
Gerrit-PatchSet: 20
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Irit Goihman
Gerrit-Reviewer: Edward Haas
Gerrit-Reviewer: Irit Goihman
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
gerrit-hooks has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. .. Patch Set 20: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0']) -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 20 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Nir Soffer has posted comments on this change.
Change subject: tox: fail make process if required tox version isn't installed.
..
Patch Set 19:
(1 comment)
https://gerrit.ovirt.org/#/c/59306/19/build-aux/vercmp
File build-aux/vercmp:
Line 44: if padding > 0:
Line 45: required_version += [0] * padding
Line 46:
Line 47: if actual_version < required_version:
Line 48: logging.debug("%s < %s", actual_version, required_version)
You are not using the logger, so the output is:
root: [0, 1, 10] < [0, 2, 0]
Instead of:
vercmp: [0, 1, 10] < [0, 2, 0]
Did you test this with -v?
All logging calls should use the logger:
log.debug(...)
Line 49: return 0
Line 50: elif actual_version == required_version:
Line 51: logging.debug("%s == %s", actual_version, required_version)
Line 52: return 1
--
To view, visit https://gerrit.ovirt.org/59306
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c
Gerrit-PatchSet: 19
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Irit Goihman
Gerrit-Reviewer: Edward Haas
Gerrit-Reviewer: Irit Goihman
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
gerrit-hooks has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. .. Patch Set 19: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0']) -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 19 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
gerrit-hooks has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. .. Patch Set 18: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0']) -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 18 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Nir Soffer has posted comments on this change.
Change subject: tox: fail make process if required tox version isn't installed.
..
Patch Set 17:
(5 comments)
https://gerrit.ovirt.org/#/c/59306/17/build-aux/vercmp
File build-aux/vercmp:
Line 26: def main():
Line 27: args = parse_args()
Line 28: if args.verbose:
Line 29: logging.basicConfig(
Line 30: level=logging.DEBUG, format="%(name)s: %(message)s")
You want to setup logging always. --verbose should effect only the log level:
level = logging.DEBUG if args.verbose else logging.INFO
logging.basicConfig(level=level, format=...)
Line 31: retval = compare_versions(args.actual_version,
args.required_version)
Line 32: sys.exit(retval)
Line 33:
Line 34:
Line 28: if args.verbose:
Line 29: logging.basicConfig(
Line 30: level=logging.DEBUG, format="%(name)s: %(message)s")
Line 31: retval = compare_versions(args.actual_version,
args.required_version)
Line 32: sys.exit(retval)
Better exit in __main__, and return a value here, so we can test this function.
Line 33:
Line 34:
Line 35: def compare_versions(actual_version, required_version):
Line 36: actual_version = [int(n) for n in actual_version.split('.')]
Line 40: if padding > 0:
Line 41: required_version += [0] * padding
Line 42:
Line 43: if actual_version < required_version:
Line 44: logging.debug("%s < %s", actual_version, required_version)
This will use the root logger, so we will see this output:
root: 0.2.3 < 0.2.4
While we actualy want to see:
vercmp: 0.2.3 < 0.2.4
To make this work, we want to create a global logger:
log = logging.getLogger("vercmp")
And use it later to log messages.
Look for example in vdsm/storage/fc-scan
Line 45: return 0
Line 46: elif actual_version == required_version:
Line 47: logging.debug("%s == %s", actual_version, required_version)
Line 48: return 1
Line 62: action='store_true', default=False)
Line 63: return parser.parse_args()
Line 64:
Line 65: if __name__ == '__main__':
Line 66: main()
If we do:
sys.exit(main(sys.argv[1:]))
We can test main by calling it with arguments. For this tool we probably will
not use this capability, but it is good idea to use this pattern on any tool we
write.
Look for example in vdsm/storage/fc-scan
https://gerrit.ovirt.org/#/c/59306/17/tox.sh
File tox.sh:
Line 1: #!/bin/sh -e
Line 2:
Line 3: WHITELIST=(build-aux/vercmp \
Line 4:contrib/logdb \
We use only spaces in scripts, this is not a makefile.
Line 5:contrib/logstat \
Line 6:contrib/profile-stats \
Line 7:init/daemonAdapter \
Line 8:vdsm/get-conf-item \
--
To view, visit https://gerrit.ovirt.org/59306
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Irit Goihman
Gerrit-Reviewer: Edward Haas
Gerrit-Reviewer: Irit Goihman
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Irit Goihman has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. .. Patch Set 17: Verified+1 -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 17 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
gerrit-hooks has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. .. Patch Set 17: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0']) -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 17 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Irit Goihman has posted comments on this change.
Change subject: tox: fail make process if required tox version isn't installed.
..
Patch Set 16:
(1 comment)
https://gerrit.ovirt.org/#/c/59306/16/build-aux/vercmp
File build-aux/vercmp:
Line 28: if args.verbose:
Line 29: logging.basicConfig(level=logging.DEBUG, format="%(name)s:
%(message)s")
Line 30: retval = compare_versions(args.actual_version,
args.required_version)
Line 31: sys.exit(retval)
Line 32:
> 2 empty lines need here to make pep8 tool happy. You also need to add this
Done
Line 33: def compare_versions(actual_version, required_version):
Line 34: actual_version = [int(n) for n in actual_version.split('.')]
Line 35: required_version = [int(n) for n in required_version.split('.')]
Line 36:
--
To view, visit https://gerrit.ovirt.org/59306
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Irit Goihman
Gerrit-Reviewer: Edward Haas
Gerrit-Reviewer: Irit Goihman
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Nir Soffer has posted comments on this change.
Change subject: tox: fail make process if required tox version isn't installed.
..
Patch Set 16:
(1 comment)
https://gerrit.ovirt.org/#/c/59306/16/build-aux/vercmp
File build-aux/vercmp:
Line 28: if args.verbose:
Line 29: logging.basicConfig(level=logging.DEBUG, format="%(name)s:
%(message)s")
Line 30: retval = compare_versions(args.actual_version,
args.required_version)
Line 31: sys.exit(retval)
Line 32:
2 empty lines need here to make pep8 tool happy. You also need to add this to
the pep8 whitelist in tox.sh.
Line 33: def compare_versions(actual_version, required_version):
Line 34: actual_version = [int(n) for n in actual_version.split('.')]
Line 35: required_version = [int(n) for n in required_version.split('.')]
Line 36:
--
To view, visit https://gerrit.ovirt.org/59306
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Irit Goihman
Gerrit-Reviewer: Edward Haas
Gerrit-Reviewer: Irit Goihman
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Irit Goihman has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. .. Patch Set 15: (2 comments) https://gerrit.ovirt.org/#/c/59306/15/Makefile.am File Makefile.am: Line 83:done; Line 84: Line 85: .PHONY: pyflakes Line 86: pyflakes: tox Line 87:tox -- pyflakes ; > ; not needed now Done Line 88: Line 89: .PHONY: pep8 Line 90: pep8: tox Line 91:tox -- pep8 ; Line 87:tox -- pyflakes ; Line 88: Line 89: .PHONY: pep8 Line 90: pep8: tox Line 91:tox -- pep8 ; > ; not needed now Done Line 92: Line 93: .PHONY: tox Line 94: tox: Line 95:if [ `command -v tox` >/dev/null 2>&1 ]; then \ -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Nir Soffer has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. .. Patch Set 15: (1 comment) https://gerrit.ovirt.org/#/c/59306/15/Makefile.am File Makefile.am: Line 97:"$(TOX_MIN_VERSION)"; \ Line 98:then \ Line 99:echo "Error: tox is too old please install tox \ Line 100: $(TOX_MIN_VERSION) or later"; \ Line 101: exit 1; \ > it looks like it can't run combined with shell if condition... it gets invo Right, we cannot use it. Line 102: fi \ Line 103: else \ Line 104: echo "Error: please install tox $(TOX_MIN_VERSION) or later.";\ Line 105: exit 1; \ -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Irit Goihman has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. .. Patch Set 15: (1 comment) https://gerrit.ovirt.org/#/c/59306/15/Makefile.am File Makefile.am: Line 97:"$(TOX_MIN_VERSION)"; \ Line 98:then \ Line 99:echo "Error: tox is too old please install tox \ Line 100: $(TOX_MIN_VERSION) or later"; \ Line 101: exit 1; \ > Why not use $(error description)? it looks like it can't run combined with shell if condition... it gets invoked without checking the condition Line 102: fi \ Line 103: else \ Line 104: echo "Error: please install tox $(TOX_MIN_VERSION) or later.";\ Line 105: exit 1; \ -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Nir Soffer has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. .. Patch Set 15: (4 comments) https://gerrit.ovirt.org/#/c/59306/15/Makefile.am File Makefile.am: Line 83:done; Line 84: Line 85: .PHONY: pyflakes Line 86: pyflakes: tox Line 87:tox -- pyflakes ; ; not needed now Line 88: Line 89: .PHONY: pep8 Line 90: pep8: tox Line 91:tox -- pep8 ; Line 87:tox -- pyflakes ; Line 88: Line 89: .PHONY: pep8 Line 90: pep8: tox Line 91:tox -- pep8 ; ; not needed now Line 92: Line 93: .PHONY: tox Line 94: tox: Line 95:if [ `command -v tox` >/dev/null 2>&1 ]; then \ Line 97:"$(TOX_MIN_VERSION)"; \ Line 98:then \ Line 99:echo "Error: tox is too old please install tox \ Line 100: $(TOX_MIN_VERSION) or later"; \ Line 101: exit 1; \ Why not use $(error description)? Line 102: fi \ Line 103: else \ Line 104: echo "Error: please install tox $(TOX_MIN_VERSION) or later.";\ Line 105: exit 1; \ Line 101: exit 1; \ Line 102: fi \ Line 103: else \ Line 104: echo "Error: please install tox $(TOX_MIN_VERSION) or later.";\ Line 105: exit 1; \ Why not use $(error description)? Line 106: fi Line 107: Line 108: Line 109: .PHONY: python3 -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
gerrit-hooks has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. .. Patch Set 15: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0']) -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Nir Soffer has posted comments on this change.
Change subject: tox: fail make process if required tox version isn't installed.
..
Patch Set 13:
(1 comment)
https://gerrit.ovirt.org/#/c/59306/13/build-aux/vercmp
File build-aux/vercmp:
Line 38: logging.debug("{} == {}".format(actual_version,
required_version))
Line 39: sys.exit(1)
Line 40: else:
Line 41: logging.debug("{} > {}".format(actual_version,
required_version))
Line 42: sys.exit(2)
> you're right but I need to return the value to shell script and the two pos
We can return a value here, and exit with this value in main.
Line 43:
Line 44:
Line 45: def parse_args():
Line 46: parser = argparse.ArgumentParser(description='compare actual
version to '
--
To view, visit https://gerrit.ovirt.org/59306
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Irit Goihman
Gerrit-Reviewer: Edward Haas
Gerrit-Reviewer: Irit Goihman
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Irit Goihman has posted comments on this change.
Change subject: tox: fail make process if required tox version isn't installed.
..
Patch Set 14:
(4 comments)
https://gerrit.ovirt.org/#/c/59306/13/build-aux/vercmp
File build-aux/vercmp:
Line 31:
Line 32: def compare_versions(actual_version, required_version):
Line 33: actual_version = [int(n) for n in actual_version.split('.')]
Line 34: required_version = [int(n) for n in required_version.split('.')]
Line 35:
> When using logging we don't format the string before logging. Use the stand
Done
Line 36: padding = len(required_version) < len(actual_version)
Line 37: if padding > 0:
Line 38: required_version += [0] * padding
Line 39:
Line 38: required_version += [0] * padding
Line 39:
Line 40: if actual_version < required_version:
Line 41: logging.debug("%s < %s", actual_version, required_version)
Line 42: sys.exit(0)
> It would be nicer to return a value instead of exiting here. This make it p
you're right but I need to return the value to shell script and the two
possible ways are with sys.exit(value) or print value.
will this utility be tested? it was written only for the purpose of comparing
tox version.
Line 43: elif actual_version == required_version:
Line 44: logging.debug("%s == %s", actual_version, required_version)
Line 45: sys.exit(1)
Line 46: else:
Line 51: def parse_args():
Line 52: parser = argparse.ArgumentParser(description='compare actual
version to '
Line 53: 'required version.\n This utility
supports '
Line 54: 'only version numbers separated
by dots.')
Line 55: parser.add_argument('actual_version')
> Can you move main to the start of the module? It is much nicer to read a mo
Done
Line 56: parser.add_argument('required_version')
Line 57: parser.add_argument('-v', '--verbose', help='increase output
verbosity',
Line 58: action='store_true', default=False)
Line 59: return parser.parse_args()
Line 54: 'only version numbers separated
by dots.')
Line 55: parser.add_argument('actual_version')
Line 56: parser.add_argument('required_version')
Line 57: parser.add_argument('-v', '--verbose', help='increase output
verbosity',
Line 58: action='store_true', default=False)
> Nice!
Done
Line 59: return parser.parse_args()
Line 60:
Line 61: if __name__ == '__main__':
--
To view, visit https://gerrit.ovirt.org/59306
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Irit Goihman
Gerrit-Reviewer: Edward Haas
Gerrit-Reviewer: Irit Goihman
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
gerrit-hooks has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. .. Patch Set 14: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0']) -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Nir Soffer has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. .. Patch Set 12: (1 comment) https://gerrit.ovirt.org/#/c/59306/12/Makefile.am File Makefile.am: Line 111: exit 1; \ Line 112: fi \ Line 113: else \ Line 114: echo "Error: please install tox $(TOX_MIN_VERSION) or later.";\ Line 115: exit 1; \ > Done - but actually why not adding a new target named 'style check' and hav Because we want to make it easy to run pep8 or pyflakes. To run both of them you can simply run: make pep8 pyflakes I think it is easy enough so we don't need style-check target. Line 116: fi Line 117: Line 118: .PHONY: python3 Line 119: python3: all -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Nir Soffer has posted comments on this change.
Change subject: tox: fail make process if required tox version isn't installed.
..
Patch Set 13:
(7 comments)
https://gerrit.ovirt.org/#/c/59306/13/build-aux/vercmp
File build-aux/vercmp:
Line 31: if padding > 0:
Line 32: required_version += [0] * padding
Line 33:
Line 34: if actual_version < required_version:
Line 35: logging.debug("{} < {}".format(actual_version,
required_version))
When using logging we don't format the string before logging. Use the standard
way:
logging.debug("%s < %s", actual_version, required_version)
Line 36: sys.exit(0)
Line 37: elif actual_version == required_version:
Line 38: logging.debug("{} == {}".format(actual_version,
required_version))
Line 39: sys.exit(1)
Line 34: if actual_version < required_version:
Line 35: logging.debug("{} < {}".format(actual_version,
required_version))
Line 36: sys.exit(0)
Line 37: elif actual_version == required_version:
Line 38: logging.debug("{} == {}".format(actual_version,
required_version))
Same
Line 39: sys.exit(1)
Line 40: else:
Line 41: logging.debug("{} > {}".format(actual_version,
required_version))
Line 42: sys.exit(2)
Line 37: elif actual_version == required_version:
Line 38: logging.debug("{} == {}".format(actual_version,
required_version))
Line 39: sys.exit(1)
Line 40: else:
Line 41: logging.debug("{} > {}".format(actual_version,
required_version))
Same
Line 42: sys.exit(2)
Line 43:
Line 44:
Line 45: def parse_args():
Line 38: logging.debug("{} == {}".format(actual_version,
required_version))
Line 39: sys.exit(1)
Line 40: else:
Line 41: logging.debug("{} > {}".format(actual_version,
required_version))
Line 42: sys.exit(2)
It would be nicer to return a value instead of exiting here. This make it
possible to test this function.
Line 43:
Line 44:
Line 45: def parse_args():
Line 46: parser = argparse.ArgumentParser(description='compare actual
version to '
Line 51: parser.add_argument('-v', '--verbose', help='increase output
verbosity',
Line 52: action='store_true', default=False)
Line 53: return parser.parse_args()
Line 54:
Line 55: def main():
Can you move main to the start of the module? It is much nicer to read a module
from top to bottom instead of from bottom to the top.
Line 56: args = parse_args()
Line 57: if args.verbose:
Line 58: logging.basicConfig(level=logging.DEBUG)
Line 59: compare_versions(args.actual_version, args.required_version)
Line 54:
Line 55: def main():
Line 56: args = parse_args()
Line 57: if args.verbose:
Line 58: logging.basicConfig(level=logging.DEBUG)
Nice!
It would be also useful to set a format, otherwise we will get unhelpful log
format. See for example how this is done in vdsm/storage/fc-scan.
Line 59: compare_versions(args.actual_version, args.required_version)
Line 60:
Line 61: if __name__ == '__main__':
Line 55: def main():
Line 56: args = parse_args()
Line 57: if args.verbose:
Line 58: logging.basicConfig(level=logging.DEBUG)
Line 59: compare_versions(args.actual_version, args.required_version)
We should exit here with the value returned from compare_versions.
Line 60:
Line 61: if __name__ == '__main__':
--
To view, visit https://gerrit.ovirt.org/59306
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Irit Goihman
Gerrit-Reviewer: Edward Haas
Gerrit-Reviewer: Irit Goihman
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Irit Goihman has posted comments on this change.
Change subject: tox: fail make process if required tox version isn't installed.
..
Patch Set 13:
(14 comments)
https://gerrit.ovirt.org/#/c/59306/12/Makefile.am
File Makefile.am:
Line 111: PYTHONDONTWRITEBYTECODE=1 $(PYTHON3) -m compileall \
Line 112: `find -not -path './.tox/*' -name '*.py'` ; \
Line 113: else \
Line 114: echo "Warning: skipping python3 syntax check"; \
Line 115: fi
> This block is duplicated. We can eliminate the duplication by moving it to
Done - but actually why not adding a new target named 'style check' and having
two runs for pyflakes and pep8?
Line 116:
Line 117: # Note: dependencies ordered by time needed to run them
Line 118: check-recursive: gitignore abs_imports python3 pyflakes pep8
Line 119:
https://gerrit.ovirt.org/#/c/59306/12/build-aux/vercmp
File build-aux/vercmp:
Line 20: #
Line 21: import argparse
Line 22: import logging
Line 23: import sys
Line 24:
> we can remove this import, see below.
Done
Line 25:
Line 26: def compare_versions(actual_version, required_version):
Line 27: actual_version = [int(n) for n in actual_version.split('.')]
Line 28: required_version = [int(n) for n in required_version.split('.')]
Line 22: import logging
Line 23: import sys
Line 24:
Line 25:
Line 26: def compare_versions(actual_version, required_version):
> This is not clear:
Done
Line 27: actual_version = [int(n) for n in actual_version.split('.')]
Line 28: required_version = [int(n) for n in required_version.split('.')]
Line 29:
Line 30: padding = len(required_version) < len(actual_version)
Line 22: import logging
Line 23: import sys
Line 24:
Line 25:
Line 26: def compare_versions(actual_version, required_version):
> This is not clear:
Done
Line 27: actual_version = [int(n) for n in actual_version.split('.')]
Line 28: required_version = [int(n) for n in required_version.split('.')]
Line 29:
Line 30: padding = len(required_version) < len(actual_version)
Line 24:
Line 25:
Line 26: def compare_versions(actual_version, required_version):
Line 27: actual_version = [int(n) for n in actual_version.split('.')]
Line 28: required_version = [int(n) for n in required_version.split('.')]
> We can simplify this:
Done
Line 29:
Line 30: padding = len(required_version) < len(actual_version)
Line 31: if padding > 0:
Line 32: required_version += [0] * padding
Line 26: def compare_versions(actual_version, required_version):
Line 27: actual_version = [int(n) for n in actual_version.split('.')]
Line 28: required_version = [int(n) for n in required_version.split('.')]
Line 29:
Line 30: padding = len(required_version) < len(actual_version)
> Comment text must be separated from the # with one space, and treated as se
Done
Line 31: if padding > 0:
Line 32: required_version += [0] * padding
Line 33:
Line 34: if actual_version < required_version:
Line 27: actual_version = [int(n) for n in actual_version.split('.')]
Line 28: required_version = [int(n) for n in required_version.split('.')]
Line 29:
Line 30: padding = len(required_version) < len(actual_version)
Line 31: if padding > 0:
> This may work but very unclear - we can simplify.
Done
Line 32: required_version += [0] * padding
Line 33:
Line 34: if actual_version < required_version:
Line 35: logging.debug("{} < {}".format(actual_version,
required_version))
Line 33:
Line 34: if actual_version < required_version:
Line 35: logging.debug("{} < {}".format(actual_version,
required_version))
Line 36: sys.exit(0)
Line 37: elif actual_version == required_version:
> We should support <, <= and ==.
Done - but changed it according to my needs
Line 38: logging.debug("{} == {}".format(actual_version,
required_version))
Line 39: sys.exit(1)
Line 40: else:
Line 41: logging.debug("{} > {}".format(actual_version,
required_version))
Line 36: sys.exit(0)
Line 37: elif actual_version == required_version:
Line 38: logging.debug("{} == {}".format(actual_version,
required_version))
Line 39: sys.exit(1)
Line 40: else:
> description=compare required_version to actual_version
Done
Line 41: logging.debug("{} > {}".format(actual_version,
required_version))
Line 42: sys.exit(2)
Line 43:
Line 44:
Line 38: logging.debug("{} == {}".format(actual_version,
required_version))
Line 39: sys.exit(1)
Line 40: else:
Line 41: logging.debug("{} > {}".format(actual_version,
required_version))
Line 42: sys.exit(2)
> Use required_version
Done
Line 43:
Line 44:
Line 45: def parse_args():
Line 46: parser = argparse.ArgumentParser(description='compare actual
version to '
Line 39: sys.exit(1)
Line 40: else:
Line 41: loggi
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
gerrit-hooks has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. .. Patch Set 13: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0']) -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Nir Soffer has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. .. Patch Set 12: (2 comments) https://gerrit.ovirt.org/#/c/59306/12/Makefile.am File Makefile.am: Line 94:exit 1; \ Line 95:fi \ Line 96:else \ Line 97:echo "Error: please install tox $(TOX_MIN_VERSION) or later.";\ Line 98:exit 1; \ We can use make $(error message...) instead of echo and exit. Line 99:fi Line 100: Line 101: .PHONY: pep8 Line 102: pep8: Line 111: exit 1; \ Line 112: fi \ Line 113: else \ Line 114: echo "Error: please install tox $(TOX_MIN_VERSION) or later.";\ Line 115: exit 1; \ This block is duplicated. We can eliminate the duplication by moving it to tox target that will fail with "tox is not installed" or "tox is too old" message. pe8 and pyflakes can depend on the tox target - something like this: pep8: tox tox -- pep8 pyflakes: tox tox -- pyflakes .PHONY: tox tox: fail if tox is not installed... fail if tox is too old... Line 116: fi Line 117: Line 118: .PHONY: python3 Line 119: python3: all -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Nir Soffer has posted comments on this change.
Change subject: tox: fail make process if required tox version isn't installed.
..
Patch Set 12:
(12 comments)
https://gerrit.ovirt.org/#/c/59306/12/build-aux/vercmp
File build-aux/vercmp:
Line 20: #
Line 21: import argparse
Line 22: import sys
Line 23:
Line 24: from six.moves import zip, zip_longest
we can remove this import, see below.
Line 25:
Line 26: def vercmp(ver1, ver2, verbose):
Line 27: tuple1 = tuple(map(int, ver1.split('.')))
Line 28: tuple2 = tuple(map(int, ver2.split('.')))
Line 22: import sys
Line 23:
Line 24: from six.moves import zip, zip_longest
Line 25:
Line 26: def vercmp(ver1, ver2, verbose):
This is not clear:
- vercmp is not clear, does not explain what this function does. How about
compare_versions?
- ver1 and ver2 are meaningless, how about required_version and actual_version?
Line 27: tuple1 = tuple(map(int, ver1.split('.')))
Line 28: tuple2 = tuple(map(int, ver2.split('.')))
Line 29:
Line 30: #pad shorter tuple with zeros
Line 24: from six.moves import zip, zip_longest
Line 25:
Line 26: def vercmp(ver1, ver2, verbose):
Line 27: tuple1 = tuple(map(int, ver1.split('.')))
Line 28: tuple2 = tuple(map(int, ver2.split('.')))
We can simplify this:
- We don't need tuples, we can compare lists in the same way.
- Using lists makes padding easier, see bellow
- map is works, but modern code should use list comprehension
- the names tuple1 and tuple2 are meaningless.
Line 29:
Line 30: #pad shorter tuple with zeros
Line 31: tuple1, tuple2 = zip(*zip_longest(tuple1, tuple2, fillvalue=0))
Line 32: if(verbose):
Line 26: def vercmp(ver1, ver2, verbose):
Line 27: tuple1 = tuple(map(int, ver1.split('.')))
Line 28: tuple2 = tuple(map(int, ver2.split('.')))
Line 29:
Line 30: #pad shorter tuple with zeros
Comment text must be separated from the # with one space, and treated as
sentence, the first letter must be capitalized.
Line 31: tuple1, tuple2 = zip(*zip_longest(tuple1, tuple2, fillvalue=0))
Line 32: if(verbose):
Line 33: print("{} < {} = {}".format(ver1, ver2, ver1 len(actual_version)
if padding:
required_version += [0] * padding
It is 3 lines but you don't have to visit the documentation to understand this
code, and you don't need to a comment to explain it.
Line 32: if(verbose):
Line 33: print("{} < {} = {}".format(ver1, ver2, ver1", actual_version)
sys.exit(2)
debug() can use verbose flag to print only if --verbose was used.
Line 38:
Line 39: def parse_args():
Line 40: parser = argparse.ArgumentParser(description='checks if version2
is newer '
Line 41: 'than version1')
Line 36: else:
Line 37: sys.exit(1)
Line 38:
Line 39: def parse_args():
Line 40: parser = argparse.ArgumentParser(description='checks if version2
is newer '
description=compare required_version to actual_version
Line 41: 'than version1')
Line 42: parser.add_argument('version1')
Line 43: parser.add_argument('version2')
Line 44: parser.add_argument('-v', '--verbose', help='increase output
verbosity',
Line 38:
Line 39: def parse_args():
Line 40: parser = argparse.ArgumentParser(description='checks if version2
is newer '
Line 41: 'than version1')
Line 42: parser.add_argument('version1')
Use required_version
Line 43: parser.add_argument('version2')
Line 44: parser.add_argument('-v', '--verbose', help='increase output
verbosity',
Line 45: action='store_true')
Line 46: return parser.parse_args()
Line 39: def parse_args():
Line 40: parser = argparse.ArgumentParser(description='checks if version2
is newer '
Line 41: 'than version1')
Line 42: parser.add_argument('version1')
Line 43: parser.add_argument('version2')
Use actual_version
We want all code to call this helper in the same way, makes it easier to use
and review.
Line 44: parser.add_argument('-v', '--verbose', help='increase output
verbosity',
Line 45: action='store_true')
Line 46: return parser.parse_args()
Line 47:
Line 41: 'than version1')
Line 42: parser.add_argument('version1')
Line 43: parser.add_argument('version2')
Line 44: parser.add_argument('-v', '--verbose', help='increase output
verbosity',
Line 45: action='store_true')
Needs default value.
Line 46: return parser.parse_args()
Line 47:
Line 48: if __name__ == '__main__':
Line 49: args = vars(parse_args())
Line 44: parser.add_argument('-v', '--verbose', help='increase output
verbosity',
Line 45: action='store_true')
Line 46: return parser.parse_args()
Line 47:
Line 48: if __name__ == '_
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
gerrit-hooks has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. .. Patch Set 12: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0']) -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Irit Goihman has posted comments on this change.
Change subject: tox: fail make process if required tox version isn't installed.
..
Patch Set 11:
(2 comments)
https://gerrit.ovirt.org/#/c/59306/11/build-aux/vercmp
File build-aux/vercmp:
Line 21: import argparse
Line 22: from six.moves import zip, zip_longest
Line 23: import sys
Line 24:
Line 25: def vercmp(ver1, ver2, verbose):
> it should explain that we check that ver1 is older than ver2 .. I would thi
Done
Line 26: tuple1 = tuple(map(int, ver1.split('.')))
Line 27: tuple2 = tuple(map(int, ver2.split('.')))
Line 28:
Line 29: #pad shorter tuple with zeros
Line 29: #pad shorter tuple with zeros
Line 30: tuple1, tuple2 = zip(*zip_longest(tuple1, tuple2, fillvalue=0))
Line 31: if(verbose):
Line 32: print("{} < {} = {}".format(ver1, ver2, ver1 exit with 0, and with 1 if the check returns False. then you can just run t
Done
Line 34:
Line 35: def parse_args():
Line 36: parser = argparse.ArgumentParser(description='compares two version
numbers')
Line 37: parser.add_argument('version1')
--
To view, visit https://gerrit.ovirt.org/59306
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Irit Goihman
Gerrit-Reviewer: Edward Haas
Gerrit-Reviewer: Irit Goihman
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Irit Goihman has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. .. Patch Set 11: (2 comments) https://gerrit.ovirt.org/#/c/59306/10/Makefile.am File Makefile.am: Line 82:done; Line 83: Line 84: .PHONY: pyflakes Line 85: pyflakes: Line 86:if ! build-aux/vercmp `tox --version | cut -d' ' -f1` \ > :) I tried to say that we should remove it from configure and check it only sorry, it was late :) I think that if we remove the check from configure we should exit if tox doesn't exist Line 87:"$(TOX_MIN_VERSION)" | grep "True" > /dev/null; \ Line 88:then \ Line 89:tox -- pyflakes ; \ Line 90:else \ https://gerrit.ovirt.org/#/c/59306/11/Makefile.am File Makefile.am: Line 82:done; Line 83: Line 84: .PHONY: pyflakes Line 85: pyflakes: Line 86:if ! build-aux/vercmp `tox --version | cut -d' ' -f1` \ > I think that you don't need to check for TOX, because this won't return "Tr you're right but maybe it'll be more informative to check and give error message Line 87:"$(TOX_MIN_VERSION)" | grep "True" > /dev/null; \ Line 88:then \ Line 89:tox -- pyflakes ; \ Line 90:else \ -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Yaniv Bronhaim has posted comments on this change.
Change subject: tox: fail make process if required tox version isn't installed.
..
Patch Set 11: Code-Review-1
(3 comments)
https://gerrit.ovirt.org/#/c/59306/11/Makefile.am
File Makefile.am:
Line 82:done;
Line 83:
Line 84: .PHONY: pyflakes
Line 85: pyflakes:
Line 86:if ! build-aux/vercmp `tox --version | cut -d' ' -f1` \
I think that you don't need to check for TOX, because this won't return "True"
in can tox does not exist.
Line 87:"$(TOX_MIN_VERSION)" | grep "True" > /dev/null; \
Line 88:then \
Line 89:tox -- pyflakes ; \
Line 90:else \
https://gerrit.ovirt.org/#/c/59306/11/build-aux/vercmp
File build-aux/vercmp:
Line 21: import argparse
Line 22: from six.moves import zip, zip_longest
Line 23: import sys
Line 24:
Line 25: def vercmp(ver1, ver2, verbose):
it should explain that we check that ver1 is older than ver2 .. I would think
that vercmp will just check that ver1 is equal to ver2. also the description
doesn't say what exactly we check
Line 26: tuple1 = tuple(map(int, ver1.split('.')))
Line 27: tuple2 = tuple(map(int, ver2.split('.')))
Line 28:
Line 29: #pad shorter tuple with zeros
Line 29: #pad shorter tuple with zeros
Line 30: tuple1, tuple2 = zip(*zip_longest(tuple1, tuple2, fillvalue=0))
Line 31: if(verbose):
Line 32: print("{} < {} = {}".format(ver1, ver2, ver1https://gerrit.ovirt.org/59306
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Irit Goihman
Gerrit-Reviewer: Edward Haas
Gerrit-Reviewer: Irit Goihman
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
Yaniv Bronhaim has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. .. Patch Set 10: (1 comment) https://gerrit.ovirt.org/#/c/59306/10/Makefile.am File Makefile.am: Line 82:done; Line 83: Line 84: .PHONY: pyflakes Line 85: pyflakes: Line 86:if [ -x "$(TOX)" ]; then \ > Done - removed tox check since it's already being checked in cofigure.ac :) I tried to say that we should remove it from configure and check it only here Line 87:if ! build-aux/vercmp `tox --version | cut -d' ' -f1` "$(TOX_MIN_VERSION)" | grep "True" > /dev/null; \ Line 88:then \ Line 89:tox -- pyflakes ; \ Line 90:else \ -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tox: fail make process if required tox version isn't installed.
gerrit-hooks has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. .. Patch Set 11: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0']) -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Irit Goihman Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
