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 <igoih...@redhat.com> Gerrit-Reviewer: Edward Haas <edwa...@redhat.com> Gerrit-Reviewer: Irit Goihman <igoih...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
