Adam Litke has posted comments on this change. Change subject: Change versioning schema and fix tarball ......................................................................
Patch Set 11: Verified+1 Code-Review-1 (11 comments) Looks pretty good. Please address review comments. http://gerrit.ovirt.org/#/c/22874/11/.gitignore File .gitignore: Line 17: INSTALL Line 18: missing Line 19: Line 20: tmp.repos/ Line 21: rpmbuild/ These bottom two aren't required, right? nothing in the in-tree build process will create these files. If some other process (jenkins) creates them, then it is the responsibility of that script to remove the files if they get in the way. http://gerrit.ovirt.org/#/c/22874/11//COMMIT_MSG Commit Message: Line 8: Line 9: * Added autogen.sh Line 10: * Changed the versions style to be same Line 11: as other oVirt packages Line 12: * Removed unused scipts You missed a fairly big change here: * Switch from distutils to autotools. Line 13: Line 14: Change-Id: I4337150f58cd7d9b3a04a12a3afa9acd8a826a22 http://gerrit.ovirt.org/#/c/22874/11/Makefile.am File Makefile.am: Line 21 Line 22 Line 23 Line 24 Line 25 Don't forget to also 'git rm setup.py.in' Line 38: Line 39: .PHONY: srpm rpm Line 40: Line 41: TMPREPOS = tmp.repos Line 42: This isn't needed. http://gerrit.ovirt.org/#/c/22874/11/configure.ac File configure.ac: Line 19 Line 20 Line 21 Line 22 Line 23 From Alon: AM_PATH_PYTHON([2.6],, [AC_MSG_ERROR([Cannot find python])]) Line 9: define([VERSION_SUFFIX], [_master]) Line 10: Line 11: AC_INIT([mom], VERSION_NUMBER[]VERSION_SUFFIX, [[email protected]]) Line 12: PACKAGE_RPM_VERSION="VERSION_NUMBER" Line 13: PACKAGE_RPM_RELEASE="4.$(echo VERSION_SUFFIX | sed 's/^_//')" Reset 4 to 0. Also, I prefer: PACKAGE_RPM_RELEASE="0$(echo VERSION_SUFFIX | sed 's/^_/./')" Note I removed the '.' between 0 and $ and moved it to the sed replace. This produces a valid PACKAGE_RPM_RELEASE even when VERSION_SUFFIX is empty (as would be the case for a release rpm). As you have it here, an empty version suffix would cause a '..' to be in the Release: field which is invalid. Line 14: PACKAGE_NAME="mom" Line 15: AC_SUBST([PACKAGE_NAME]) Line 16: AC_SUBST([PACKAGE_RPM_VERSION]) Line 17: AC_SUBST([PACKAGE_RPM_RELEASE]) http://gerrit.ovirt.org/#/c/22874/11/contrib/Makefile.am File contrib/Makefile.am: Line 15: Line 16: EXTRA_DIST = \ Line 17: momd.init \ Line 18: momd.service \ Line 19: mom-rpcclient.py \ From Alon: dist_noinst_SCRIPTS for scripts? http://gerrit.ovirt.org/#/c/22874/11/mom.spec.in File mom.spec.in: Line 82: Line 83: Line 84: %preun Line 85: if [ $1 = 0 ] ; then Line 86: /sbin/service momd stop >/dev/null 2>&1 || /bin/true From Alon: I do not like the use of full paths, usually best to leave the system to find utilities. Line 87: /sbin/chkconfig --del momd Line 88: fi Line 89: Line 90: Line 96: Line 97: %files Line 98: %defattr(-,root,root,-) Line 99: %doc COPYING Line 100: %doc README You don't include the examples directory? In http://gerrit.ovirt.org/#/c/23400/1/mom.spec.in,cm see how I included all documentation files. Line 101: %{_sbindir}/momd Line 102: %{_initrddir}/momd Line 103: %{python_sitelib}/* Line 104: %config(noreplace) %{_sysconfdir}/momd.conf Line 105: Line 106: %changelog Line 107: * Sun Jan 19 2014 Kiril Nesenko <[email protected]> - 0.3.2 Line 108: - Change versioning style Line 109: I think it's also worth mentioning that we switched to an autotools based build here. Line 110: * Fri Oct 05 2012 Adam Litke <[email protected]> - 0.3.0-1 Line 111: - Upgrade to version 0.3.0 Line 112: - Upstream fixes CVE-2012-4480 Line 113: http://gerrit.ovirt.org/#/c/22874/11/tests/Makefile.am File tests/Makefile.am: Line 20: testrunner.py \ Line 21: $(NULL) Line 22: Line 23: check-local: Line 24: ./run_tests_local.sh *.py Transferring comment from Alon on my original series: this won't run in separate builddir, I think it should be: $(srcdir)/run_tests_local.sh $(srcdir)/*.py -- To view, visit http://gerrit.ovirt.org/22874 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4337150f58cd7d9b3a04a12a3afa9acd8a826a22 Gerrit-PatchSet: 11 Gerrit-Project: mom Gerrit-Branch: master Gerrit-Owner: Kiril Nesenko <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Doron Fediuck <[email protected]> Gerrit-Reviewer: Eyal Edri <[email protected]> Gerrit-Reviewer: Kiril Nesenko <[email protected]> Gerrit-Reviewer: Martin Sivák <[email protected]> Gerrit-Reviewer: Sandro Bonazzola <[email protected]> Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
