Change in vdsm[master]: make compression type configurable, support gzip and xz

Alon Bar-Lev alonbl at redhat.com
Mon Nov 5 09:03:12 UTC 2012


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 <shaohef at linux.vnet.ibm.com>
Gerrit-Reviewer: Alon Bar-Lev <alonbl at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: Ryan Harper <ryanh at us.ibm.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: ShaoHe Feng <shaohef at linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming at linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj at linux.vnet.ibm.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list