Ryan Harper has posted comments on this change. Change subject: make compression type configurable, support gzip and xz ......................................................................
Patch Set 4: I would prefer that you didn't submit this (5 inline comments) Please review the comments in configure.ac; let's make it much simpler by not exposing a --with-xx option, but a much simpler check for xz, unxz, gzip, gunzip, and then based on whether xz is found, set the compression type. If we find xz, use it, if not, use gzip/gunzip. .................................................... File configure.ac Line 40: AM_INIT_AUTOMAKE([-Wno-portability]) Line 41: Line 42: # the make dist compress type Line 43: AC_PATH_PROG([XZ], [xz]) Line 44: AC_PATH_PROG([GZIP], [gzip]) These checks should be down in the sorted-list of AC_PATH_PROG checks (line 124). Line 45: if test "x$XZ" = "x"; then Line 46: if test "x$GZIP" = "x"; then Line 47: AC_MSG_ERROR([neither xz and gzip is found, please install one of them]) Line 48: else Line 46: if test "x$GZIP" = "x"; then Line 47: AC_MSG_ERROR([neither xz and gzip is found, please install one of them]) Line 48: else Line 49: AC_SUBST([DIST_TYPE_SUFFIX], [gz]) Line 50: AC_MSG_NOTICE([support gzip compress]) Configuration selection is usually emitted after all of the checks have been completed. I would move all of the logic around selecting the compression type after all of the PATH checking is complete. Line 51: fi Line 52: else Line 53: AC_SUBST([DIST_TYPE_SUFFIX], [xz]) Line 54: AC_MSG_NOTICE([support xz compress]) Line 67: [logrotate_compress=gzip], Line 68: [logrotate_compress=xz] Line 69: ) Line 70: ] Line 71: ) I don't think this needs to be surfaced as a configure switch. Rather we simply want to prefer xz over gzip if it's present. If we have xz, then we can set the compression type to that and use it. If we don't find xz and we don't find gzip, we can error out indicating we need some sort of compression program. Line 72: AC_MSG_NOTICE([configure logrotate compress by $logrotate_compress]) Line 73: Line 74: if test "x$logrotate_compress" = xgzip; then Line 75: AC_SUBST([COMPRESS_SUFFIX], [gz]) Line 74: if test "x$logrotate_compress" = xgzip; then Line 75: AC_SUBST([COMPRESS_SUFFIX], [gz]) Line 76: AC_SUBST([REQUIRE_COMPRESS], [gzip]) Line 77: AC_PATH_PROG([COMPRESSCMD], [gzip], [/usr/bin/gzip]) Line 78: AC_PATH_PROG([UNCOMPRESSCMD], [gunzip], [/usr/bin/gunzip]) Just add the uncompress commands to the list of external programs to check; we don't need to optionally check. Line 79: else Line 80: AC_SUBST([COMPRESS_SUFFIX], [xz]) Line 81: AC_SUBST([REQUIRE_COMPRESS], [xz]) Line 82: AC_PATH_PROG([COMPRESSCMD], [xz], [/usr/bin/xz]) Line 82: AC_PATH_PROG([COMPRESSCMD], [xz], [/usr/bin/xz]) Line 83: AC_PATH_PROG([UNCOMPRESSCMD], [unxz], [/usr/bin/unxz]) Line 84: fi Line 85: AC_CHECK_FILE(vdsm.spec, [rm -f vdsm.spec]) Line 86: AC_CHECK_FILE(vdsm/vdsm-logrotate.conf, [rm -f vdsm/vdsm-logrotate.conf]) Do not delete the generated files that you don't create; This is handled by make clean or distclean targets. Line 87: Line 88: # Checking for build tools Line 89: AC_PROG_CC Line 90: AC_PROG_LN_S -- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: ShaoHe Feng <[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: oVirt Jenkins CI Server _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
