Alon Bar-Lev has posted comments on this change. Change subject: build: Revamp mom build system ......................................................................
Patch Set 1: (21 comments) sorry about so many comments... another suggestion is to have config.py.in to inject autoconf variables into python domain, such as PACKAGE_NAME, PACKAGE_VERSION, to be provided in --version/logs etc. http://gerrit.ovirt.org/#/c/23400/1/.gitignore File .gitignore: Line 18: INSTALL Line 19: missing Line 20: Line 21: tmp.repos/ Line 22: rpmbuild still required these two above? http://gerrit.ovirt.org/#/c/23400/1/Makefile.am File Makefile.am: Line 25 Line 26 Line 27 Line 28 Line 29 spec is managed by autoconf it is should be removed at correct phase. Line 15: Line 16: SUBDIRS = contrib doc mom tests Line 17: Line 18: AUTOMAKE_OPTIONS = foreign 1.9 Line 19: ACLOCAL_AMFLAGS = --install -I m4 not sure why --install is required Line 20: Line 21: # This is an *exception*, we ship also mom.spec so it's possible to build Line 22: # the rpm from the tarball. Line 23: EXTRA_DIST = \ Line 35: $(NULL) Line 36: Line 37: CLEANFILES = \ Line 38: mom.spec \ Line 39: $(DIST_ARCHIVES) \ please do not remove dist archive it should be left even in clean so that it won't get lost, especially if someone md5sum it before release and accidentally make clean install to check something. Line 40: $(NULL) Line 41: Line 38: mom.spec \ Line 39: $(DIST_ARCHIVES) \ Line 40: $(NULL) Line 41: Line 42: TMPREPOS = tmp.repos why is this needed? http://gerrit.ovirt.org/#/c/23400/1/autogen.sh File autogen.sh: Line 1: #!/bin/bash no reason for bash Line 2: Line 3: rm -rf autom4te.cache Line 4: autoreconf -ivf Line 5: Line 1: #!/bin/bash Line 2: Line 3: rm -rf autom4te.cache this is not required in recent autotools (at least 6 years). Line 4: autoreconf -ivf Line 5: Line 6: if [ ! -f "configure" ]; then Line 7: echo "Failed to generate configure script. Check to make sure autoconf, " Line 2: Line 3: rm -rf autom4te.cache Line 4: autoreconf -ivf Line 5: Line 6: if [ ! -f "configure" ]; then usually if ! [ xxx ]; then is better as the shell inverts the result, but it is really not required as autoreconf result should be checked. Line 7: echo "Failed to generate configure script. Check to make sure autoconf, " Line 8: echo "automake, and other build dependencies are properly installed." Line 9: exit 1 Line 10: fi Line 8: echo "automake, and other build dependencies are properly installed." Line 9: exit 1 Line 10: fi Line 11: Line 12: if [ "x$1" == "x--system" ]; then == -> = Line 13: ./configure --prefix=/usr --sysconfdir=/etc --localstatedir=/var Line 14: else Line 15: if [ $# -gt 0 ]; then Line 16: ./configure $@ Line 12: if [ "x$1" == "x--system" ]; then Line 13: ./configure --prefix=/usr --sysconfdir=/etc --localstatedir=/var Line 14: else Line 15: if [ $# -gt 0 ]; then Line 16: ./configure $@ "$@" I think Line 17: else Line 18: ./configure --prefix=/usr/local Line 19: fi Line 16: ./configure $@ Line 17: else Line 18: ./configure --prefix=/usr/local Line 19: fi Line 20: fi autogen should be just: autoreconf -ivf nothing more... anyway... prefix is /usr/local by default :) all the other are stuff that should be specified by whoever configure the package. because of that I do not provide any script in packages... not that it is super important, but it provides people with the means to handle packages without these helper without feeling helpless. http://gerrit.ovirt.org/#/c/23400/1/configure.ac File configure.ac: Line 19 Line 20 Line 21 Line 22 Line 23 AM_PATH_PYTHON([2.6],, [AC_MSG_ERROR([Cannot find python])]) Line 22: AM_INIT_AUTOMAKE Line 23: Line 24: # Checking for build tools Line 25: AM_PATH_PYTHON([2.6]) Line 26: AC_PATH_PROG([PEP8], [pep8], [/usr/bin/pep8]) please copy as much as possible from otopi AC_ARG_VAR([PEP8], [path to pep8 utility]) AC_CHECK_PROGS([PEP8], [pep8]) Line 27: AC_PYTHON_MODULE([unittest]) Line 28: AC_SUBST([HAVE_PYMOD_UNITTEST]) Line 29: Line 30: # Checking for nosetests http://gerrit.ovirt.org/#/c/23400/1/contrib/Makefile.am File contrib/Makefile.am: Line 13: # License along with this program; if not, write to the Free Software Line 14: # Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA Line 15: Line 16: EXTRA_DIST = \ Line 17: momd.init \ dist_noinst_SCRIPTS for scripts? Line 18: momd.service \ Line 19: mom-rpcclient.py \ http://gerrit.ovirt.org/#/c/23400/1/mom.spec.in File mom.spec.in: Line 52: %setup -q -n %{package_name}-%{package_version} Line 53: Line 54: %build Line 55: %configure Line 56: make add %{?_smp_mflags}? Line 57: Line 58: %install Line 59: rm -rf "%{buildroot}" Line 60: make DESTDIR=%{buildroot} install Line 70: mv "%{buildroot}"/_tmp "%{buildroot}"/%{_pkgdocdir} Line 71: cp -p COPYING README "%{buildroot}"/%{_pkgdocdir} Line 72: Line 73: %check Line 74: make check add %{?_smp_mflags}? Line 75: Line 76: %clean Line 77: rm -rf "%{buildroot}" Line 78: Line 88: Line 89: Line 90: %postun Line 91: if [ "$1" -ge "1" ] ; then Line 92: /sbin/service momd condrestart >/dev/null 2>&1 || /bin/true I do not like the use of full paths, usually best to leave the system to find utilities. Line 93: fi Line 94: Line 95: Line 96: %files Line 102: Line 103: # The use of '_pkgdocdir' conflicts with '%%doc' in some distro versions. Line 104: # Therefore, '%%doc' MUST NOT be used to include additional documentation Line 105: # files so long as this is in use. Line 106: %{_pkgdocdir}/ but why not use %doc for these files that are not installed at docdir? Line 107: Line 108: %changelog Line 109: * Fri Oct 05 2012 Adam Litke <[email protected]> - 0.3.0-1 Line 110: - Upgrade to version 0.3.0 http://gerrit.ovirt.org/#/c/23400/1/mom/Collectors/Makefile.am File mom/Collectors/Makefile.am: Line 23: GuestQemuProc.py \ Line 24: HostKSM.py \ Line 25: HostMemory.py \ Line 26: __init__.py \ Line 27: QemuGuestAgentClient.py \ try to sort these blocks :) http://gerrit.ovirt.org/#/c/23400/1/tests/Makefile.am File tests/Makefile.am: Line 12: # You should have received a copy of the GNU General Public Line 13: # License along with this program; if not, write to the Free Software Line 14: # Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA Line 15: Line 16: EXTRA_DIST = \ these are dist_noins_PYTHON/SCRIPTS no? Line 17: GeneralTests.py \ Line 18: ParserTests.py \ Line 19: run_tests_local.sh \ Line 20: testrunner.py \ Line 20: testrunner.py \ Line 21: $(NULL) Line 22: Line 23: check-local: Line 24: ./run_tests_local.sh *.py 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/23400 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I311dcad505ba338b092c4857a4b6dc1c6180bcd9 Gerrit-PatchSet: 1 Gerrit-Project: mom Gerrit-Branch: master Gerrit-Owner: Adam Litke <[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
