fedora-review denied: [Bug 226105] Merge Review: logwatch
by Red Hat Bugzilla
Bug 226105: Merge Review: logwatch
Product: Fedora Extras
Version: devel
Component: Package Review
Kevin Fenzi <kevin(a)tummy.com> has denied Ivana Varekova <varekova(a)redhat.com>'s
request for fedora-review:
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226105
------- Additional Comments from Kevin Fenzi <kevin(a)tummy.com>
1. Removing them I guess is ok.
Have you contacted upstream about distributing them at all?
2. good. ok.
3.1. %build added ok.
3.2. 644 on all the services looks good. ok.
3.3. The logwatch.pl links are still not relative...
Suggest:
Change:
ln -s %{_datadir}/logwatch/scripts/logwatch.pl
%{buildroot}%{_sysconfdir}/cron.daily/0logwatch
ln -s %{_datadir}/logwatch/scripts/logwatch.pl %{buildroot}%{_sbindir}/logwatch
to
ln -s ../../%{_datadir}logwatch/scripts/logwatch.pl
%{buildroot}%{_sysconfdir}/cron.daily/0logwatch
ln -s ../../%{_datadir}logwatch/scripts/logwatch.pl
%{buildroot}%{_sbindir}/logwatch
3.4. The "pix" file still has dos CR/FE endings...
Suggest:
%{__sed} -i 's/\r//' %{buildroot}%{_datadir}/logwatch/scripts/services/pix
3.5. ok.
So, 3.3 and 3.4 are the only outstanding items I see...
Once you have addressed these items (either by making the suggested changes, or
by explaining why they don't make sense), please reassign this review back
to me, and change the 'fedora-review' flag back to ? for me to take action.
17 years, 3 months
fedora-review denied: [Bug 226479] Merge Review: tcl
by Red Hat Bugzilla
Bug 226479: Merge Review: tcl
Product: Fedora Extras
Version: devel
Component: Package Review
Wart <wart(a)kobold.org> has denied Wart <wart(a)kobold.org>'s request for
fedora-review:
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226479
------- Additional Comments from Wart <wart(a)kobold.org>
Since Tcl is currently flip-flopping between 8.5a5 and 8.4, I'll start
with some of the low-hanging fruit in the spec file, and return to more
version-specific issues once it settles down.
* Source1 should no longer be necessary now that tk has been split out
into a separate package. This will greatly reduce the size of the
src rpm.
* Removing Source1 will let you collapse the build directory by one
level, since the tk sources no longer get unpacked alongside the tcl
sources. Make sure to adjust the setup and patch commands to reflect
this new structure.
* Source2 contains the html sources. Aren't these already built from the
Tcl sources, or do they have to come from a separate tarball? If
they must come from a separate tarball, it would be nice to include
a pointer to the original url, or commands used to generate the html
files.
* BuildRequires: sed not necessary. Likewise, I suspect that
BuildRequires: man isn't needed either.
* The Version: and URL: tags for the subpackages are redundant. They
are copied from the main package if not found.
* In %build, ls %{_tmppath} is pointless. Delete it.
* Move the 'make test' conditionals to a separate '%check' section instead of
putting it in %build.
* %install contains a 'make html' command that appears to be building the
html documentation. This should be in %build, unless it requires
Tcl to be installed before running. Even if that's the case, it
might be possible to run Tcl from the build directory in order to generate
the html docs. Either move 'make html' to %build, or document in the
spec file why it needs to be in %install.
* The backwards compatible symlink might be better addressed by creating
explicit directories. As it is now, it creates a link from /usr/lib to
/usr/share/tcl8.5 on 64-bit systems, and doesn't create and own
/usr/lib/tcl8.5 at all.
See bug #227200 for one possible workaround.
* The %post and %postun sections should be replaced by one-liners:
%post -p /sbin/ldconfig
%postun -p /sbin/ldconfig
17 years, 3 months
fedora-review denied: [Bug 225928] Merge Review: jakarta-commons-el
by Red Hat Bugzilla
Bug 225928: Merge Review: jakarta-commons-el
Product: Fedora Extras
Version: devel
Component: Package Review
Kevin Fenzi <kevin(a)tummy.com> has denied Kevin Fenzi <kevin(a)tummy.com>'s
request for fedora-review:
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225928
------- Additional Comments from Kevin Fenzi <kevin(a)tummy.com>
Hey Thomas.
The current way we are doing these reviews is described at:
http://fedoraproject.org/wiki/WarrenTogami/ReviewWithFlags
So, you should set the 'fedora-review' flag to - and reassign back to the
owner/submitter to fix items you see in your review. Then when they do so,
they should add a comment, change 'fedora-review' to ? and reassign back to you
to look over. Once you approve the package reassign the review back to the
submitter and set the 'fedora-review' flag to +
Blocker bugs aren't being used for these reviews.
Could you re-assign and set the flags?
17 years, 3 months
fedora-review requested: [Bug 226186] Merge Review: ncpfs
by Red Hat Bugzilla
Bug 226186: Merge Review: ncpfs
Product: Fedora Extras
Version: devel
Component: Package Review
Kevin Fenzi <kevin(a)tummy.com> has asked for fedora-review:
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226186
------- Additional Comments from Kevin Fenzi <kevin(a)tummy.com>
Hey mitr...
I think the procedure we are using currently is to reassign back to the
reviewer
and reset the fedora-review flag to ? when you want the reviewer to check back
on changes... I am doing so here so tibbs can continue.
17 years, 3 months
fedora-review denied: [Bug 227631] Review Request: autofs - A tool for automatically mounting and unmounting filesystems
by Red Hat Bugzilla
Bug 227631: Review Request: autofs - A tool for automatically mounting and
unmounting filesystems
Product: Fedora Extras
Version: devel
Component: Package Review
Orion Poplawski <orion(a)cora.nwra.com> has denied Orion Poplawski
<orion(a)cora.nwra.com>'s request for fedora-review:
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=227631
------- Additional Comments from Orion Poplawski <orion(a)cora.nwra.com>
Some of these are minor, but does bring about a nice consistency to the spec
files.
- rpmlint checks return:
W: autofs no-url-tag
Is there any?
W: autofs unversioned-explicit-obsoletes autofs-ldap
The specfile contains an unversioned Obsoletes: token, which will match all
older, equal and newer versions of the obsoleted thing. This may cause update
problems, restrict future package/provides naming, and may match something it
was originally not inteded to match -- make the Obsoletes versioned if
possible.
W: autofs macro-in-%changelog _xxx
Macros are expanded in %changelog too, which can in unfortunate cases lead
to the package not building at all, or other subtle unexpected conditions that
affect the build. Even when that doesn't happen, the expansion results in
possibly "rewriting history" on subsequent package revisions and generally
odd entries eg. in source rpms, which is rarely wanted. Avoid use of macros
in %changelog altogether, or use two '%'s to escape them, like '%%foo'.
W: autofs mixed-use-of-spaces-and-tabs (spaces: line 481, tab: line 102)
The specfile mixes use of spaces and tabs for indentation, which is a
cosmetic annoyance. Use either spaces or tabs for indentation, not both.
- Why define %version and %release? Aren't they are defined automatically?
- Consider Requires: chkconfig rather than Requires: /sbin/chkconfig, easier on
the depsolvers and unlikely to change. Similar for /bin/bash, others.
- %clean should just be:
rm -rf $RPM_BUILD_ROOT
- exit 0 in %preun seems unneeded.
- %defattr should be:
%defattr(-,root,root,-)
- There's an empty %doc line.
- Still need to ship %dir /misc? You don't with /net.
- This:
%dir %{_libdir}/autofs
%{_libdir}/autofs/*
could be:
%{_libdir}/autofs/
Good:
- package meets naming guidelines
- package meets packaging guidelines
- license ( ) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file
*
17 years, 3 months