Dan Kenigsberg has posted comments on this change. Change subject: packaging: add required packages... ......................................................................
Patch Set 2: (4 comments) http://gerrit.ovirt.org/#/c/26330/2/ovirt-el-deps.repo File ovirt-el-deps.repo: Line 1: [ovirt-epel] I do not understand the asymmetry here. Why do we call this repo "ovirt-epel" and do not have the "ovirt-" prefix on other repositories? Why here we allow only epel-release (and inotify) to be loaded, and accept any package elsewhere? Line 2: name=Extra Packages for Enterprise Linux 6 - $basearch Line 3: #baseurl=http://download.fedoraproject.org/pub/epel/6/$basearch Line 4: mirrorlist=https://mirrors.fedoraproject.org/metalink?repo=epel-6&arch=$basearch Line 5: failovermethod=priority Line 3: #baseurl=http://download.fedoraproject.org/pub/epel/6/$basearch Line 4: mirrorlist=https://mirrors.fedoraproject.org/metalink?repo=epel-6&arch=$basearch Line 5: failovermethod=priority Line 6: enabled=1 Line 7: includepkgs=epel-release,python-inotify in epel-release is installed, we do not need python-inotify explicitly mentioned here, right? If not, shouldn't we mention python-daemon as well? Line 8: gpgcheck=1 Line 9: Line 10: [jpackage-6.0-generic] Line 11: name=JPackage 6.0, for generic http://gerrit.ovirt.org/#/c/26330/2/ovirt-release.spec.in File ovirt-release.spec.in: Line 85: if [ "$DIST" == "EL" ] ; then Line 86: install -m 644 "%{_datadir}/%{package_name}/ovirt-el-deps.repo" "%{_sysconfdir}/yum.repos.d/ovirt-el-deps.repo" Line 87: fi Line 88: Line 89: if rpm --eval "%%dist" | grep -qFi 'fc19'; then wouldn't it be cleaner to have a build per platform, with more standard %if 0%{?fedora} in the %install section? It's a bit odd to have an rpm modifying its shipped files upon installation. Or does it add needless burden to release engineering? (Obviously, this question goes beyond the scope of this specific patch.) Line 90: install -m 644 "%{_datadir}/%{package_name}/ovirt-f19-deps.repo" "%{_sysconfdir}/yum.repos.d/ovirt-f19-deps.repo" Line 91: fi Line 92: Line 93: Line 96: %defattr(-,root,root,-) Line 97: %doc COPYING Line 98: %{_datadir}/%{package_name}/ Line 99: %ghost %config(noreplace) %{_sysconfdir}/yum.repos.d/ovirt.repo Line 100: %ghost %config(noreplace,missingok) %{_sysconfdir}/yum.repos.d/glusterfs-epel.repo Do you keep glusterfs-epel here intentionally? Why? Line 101: %ghost %config(noreplace,missingok) %{_sysconfdir}/yum.repos.d/ovirt-f19-deps.repo Line 102: %ghost %config(noreplace,missingok) %{_sysconfdir}/yum.repos.d/ovirt-el-deps.repo Line 103: Line 104: %changelog -- To view, visit http://gerrit.ovirt.org/26330 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib357d7066970496e94f45e19b931fe7b381ef1bc Gerrit-PatchSet: 2 Gerrit-Project: ovirt-release Gerrit-Branch: master Gerrit-Owner: Sandro Bonazzola <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: David Caro <[email protected]> Gerrit-Reviewer: Douglas Schilling Landgraf <[email protected]> Gerrit-Reviewer: Kiril Nesenko <[email protected]> Gerrit-Reviewer: Lev Veyde <[email protected]> Gerrit-Reviewer: Sandro Bonazzola <[email protected]> Gerrit-Reviewer: Simone Tiraboschi <[email protected]> Gerrit-Reviewer: Yedidyah Bar David <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
