Alon Bar-Lev has posted comments on this change. Change subject: make compression type configurable, support gzip and xz ......................................................................
Patch Set 5: I would prefer that you didn't submit this (7 inline comments) Hello, The question can be why not use gzip and only gzip. The SOURCES of vdsm is not something that gain much from either compression method. On BUILD machine it is better to have minimum requirement. You are confusing between the RUNTIME machine and the BUILD machine if you consider the source tarball compression and the logrotate usage. Hello, The question can be why not use gzip and only gzip. The SOURCES of vdsm is not something that gain much from either compression method. On BUILD machine it is better to have minimum requirement. You are confusing between the RUNTIME machine and the BUILD machine if you consider the source tarball compression and the logrotate usage. Anyway, to distribute source in multiple formats you only need to use the following options in Makefile.am: --- AUTOMAKE_OPTIONS = dist-xz --- And not use complex patch as you proposed. This will produce both gz and xz tarballs, for developers to chose what to use. It save 300K!!! does it worth it? I think no. I added some comments in-line, but anyway, I don't understand the need and the added complexity, not the add of other dependencies. Regards, Alon. --- Dan, Please notice that this is one of these rare cases I put -1, it means that "Had I had been the maintainer I would not submitted this patch". Alon Anyway, to distribute source in multiple formats you only need to use the following options in Makefile.am: --- AUTOMAKE_OPTIONS = dist-xz --- And not use complex patch as you proposed. This will produce both gz and xz tarballs, for developers to chose what to use. It save 300K!!! does it worth it? I think no. I added some comments in-line, but anyway, I don't understand the need and the added complexity, not the add of other dependencies. Regards, Alon. .................................................... File configure.ac Line 122: fi Line 123: Line 124: Line 125: # Checking for xz compress Line 126: AC_PATH_PROG([XZ], [xz]) Automake does this automatically, no need for autoconf code. And... usage of optional component should be explicit using --with-compress= parameter to autoconf with default which is optionally auto-detect. Build system is not mind reader to know what to support. So if you thought of using it in different place, this should be completely altered. Line 127: if test "x$XZ" = "x"; then Line 128: AC_PATH_PROG([GZIP], [gzip], [/usr/bin/gzip]) Line 129: AC_SUBST([DIST_TYPE_SUFFIX], [gz]) Line 130: AC_MSG_NOTICE([not find xz, fallback on gzip compress]) Line 196: AC_PATH_PROG([IPROUTE_PATH], [ip], [/sbin/ip]) Line 197: AC_PATH_PROG([WGET_PATH], [wget], [/usr/bin/wget]) Line 198: AC_PATH_PROG([YUM_PATH], [yum], [/usr/bin/yum]) Line 199: Line 200: AC_CHECK_FILE(vdsm.spec, [rm -f vdsm.spec]) This does not belongs to this patch. This should not be used also in other patches without a GOOD reason. Line 201: Line 202: # Keep sorted Line 203: AC_OUTPUT([ Line 204: Makefile .................................................... File Makefile.am Line 18: # Refer to the README and COPYING files for full details of the license Line 19: # Line 20: Line 21: SUBDIRS = vdsm vdsm_cli vds_bootstrap vdsm_reg vdsm_hooks tests vdsm-tool Line 22: make_dist_type = @DIST_TYPE_SUFFIX@ This is not required, as you can use $(DIST_TYPE_SUFFIX) Line 23: Line 24: include $(top_srcdir)/build-aux/Makefile.subs Line 25: Line 26: # This is an *exception*, we ship also vdsm.spec so it's possible to build the Line 29 Line 30 Line 31 Line 32 Line 33 This can be both. Line 130 Line 131 Line 132 Line 133 Line 134 This can be either as we create the two. Line 138: Line 139: rpm: dist Line 140: rpmbuild -ta $(if $(BUILDID),--define="extra_release .$(BUILDID)") \ Line 141: $(WITH_HOOKS) $(DIST_ARCHIVES) Line 142: dist dist-all: This should be replaced with: AUTOMAKE_OPTIONS = dist-xz It will produce both formats. Line 143: test $(make_dist_type) = "xz" \ Line 144: && $(MAKE) $(AM_MAKEFLAGS) dist-xz \ Line 145: || (test $(make_dist_type) = "gz" \ Line 146: && $(MAKE) $(AM_MAKEFLAGS) dist-gzip) .................................................... File vdsm.spec.in Line 32: Line 33: Group: Applications/System Line 34: License: GPLv2+ Line 35: Url: http://www.ovirt.org/wiki/Vdsm Line 36: Source0: %{vdsm_name}-%{version}.tar.%{compress_suffix} There can be only one here. You can allow override but there can be only one. Line 37: BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) Line 38: Line 39: BuildRequires: python Line 40: BuildRequires: python-devel -- To view, visit http://gerrit.ovirt.org/4174 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4d88e9d40aefc87564c9c7e23f0d28aaad867aca Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: ShaoHe Feng <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Mark Wu <[email protected]> Gerrit-Reviewer: Ryan Harper <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: ShaoHe Feng <[email protected]> Gerrit-Reviewer: Shu Ming <[email protected]> Gerrit-Reviewer: Xu He Jie <[email protected]> Gerrit-Reviewer: Zhou Zheng Sheng <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
