Bug 480859 (diffuse)
Summary: | Review Request: diffuse - graphical diff tool | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Jon Levell <fedora> |
Component: | Package Review | Assignee: | Michael Schwendt <bugs.michael> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | low | ||
Version: | rawhide | CC: | bugs.michael, fedora-package-review, fkooman, i, itamar, mlichvar, notting, pahan, randyn3lrx |
Target Milestone: | --- | Flags: | bugs.michael:
fedora-review+
gwync: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | 0.2.15-4.fc10 | Doc Type: | Bug Fix |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2009-03-11 17:59:39 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: |
Description
Jon Levell
2009-01-20 22:49:38 UTC
(Just some remarks from quickly looking over the package) - For your Source line see: https://fedoraproject.org/wiki/PackagingDrafts/SourceUrl, maybe also make it Source0 instead of just Source. - See https://fedoraproject.org/wiki/Packaging:Python for specific Python packaging guidelines - See https://fedoraproject.org/wiki/PackageMaintainers/CreatingPackageHowTo for a template of a spec file (or use rpmdev-newspec), it might be nice to create your spec file based on this or modify yours to better match the template. rpmlint output: [fkooman@franek SPECS]$ rpmlint diffuse.spec 0 packages and 1 specfiles checked; 0 errors, 0 warnings. [fkooman@franek SPECS]$ [fkooman@franek SRPMS]$ rpmlint diffuse-0.2.15-1.fc10.src.rpm diffuse.src: W: summary-not-capitalized graphical tool for comparing and merging text files diffuse.src: W: non-standard-group Development/Tools/Version Control diffuse.src: W: no-url-tag 1 packages and 0 specfiles checked; 0 errors, 3 warnings. [fkooman@franek SRPMS]$ [fkooman@franek noarch]$ rpmlint diffuse-0.2.15-1.fc10.noarch.rpm diffuse.noarch: W: summary-not-capitalized graphical tool for comparing and merging text files diffuse.noarch: W: non-standard-group Development/Tools/Version Control diffuse.noarch: W: no-url-tag diffuse.noarch: W: conffile-without-noreplace-flag /etc/diffuserc diffuse.noarch: E: invalid-desktopfile /usr/share/applications/diffuse.desktop 1 packages and 0 specfiles checked; 1 errors, 4 warnings. [fkooman@franek noarch]$ please don't gzip man pages. gzip -9 %{buildroot}/usr/share/man/man1/diffuse.1 all files in /usr/share/man are magically gziped by rpm please macronify your %files section please replace all hardcoded paths below with macros. /usr/bin/diffuse /usr/share/diffuse/ /usr/share/applications/diffuse.desktop /usr/share/gnome/help/diffuse/C/diffuse.xml /usr/share/man/man1/diffuse.1.gz /usr/share/omf/diffuse/diffuse-C.omf /usr/share/pixmaps/diffuse.png for more info about macros please l@@k https://fedoraproject.org/wiki/Packaging/RPMMacros I hope I've addressed all the comments. Updated files are: Spec URL: http://coralbark.net/fedora/diffuse/diffuse.spec SRPM URL: http://coralbark.net/fedora/diffuse/diffuse-0.2.15-2.fc10.src.rpm RPM URL: http://coralbark.net/fedora/diffuse/diffuse-0.2.15-2.fc10.noarch.rpm you're missing somethint in changelog * Wed Jan 21 2009 Jon Levell <fedora> 0.2.15-2 should be something like this * Wed Jan 21 2009 Jon Levell <fedora> - 0.2.15-2 macronify more. from %{_bindir}/diffuse %{_datadir}/diffuse/ to %{_bindir}/%{name} %{_datadir}/%{name}/ change from %defattr(-,root,root) to %defattr(-,root,root,-) fix your buildroot according with guidelines https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag Another update: Spec URL: http://coralbark.net/fedora/diffuse/diffuse.spec SRPM URL: http://coralbark.net/fedora/diffuse/diffuse-0.2.15-3.fc10.src.rpm RPM URL: http://coralbark.net/fedora/diffuse/diffuse-0.2.15-3.fc10.noarch.rpm I've addressed all the comments except for #5. I considered what you suggest when making the first "macronified" version but it seems funny to change: %{_bindir}/diffuse to %{_bindir}/%{name} If I'm not changing: %{_sysconfdir}/diffuserc to %{_sysconfdir}/%{name}rc and I've not keen on making the second change - if we were changing the name of the program, there would be bigger issues than updating the files section. If people feel strongly that I should make the suggested change I will. If so, should I change the diffuserc as well as: %{_bindir}/diffuse %{_datadir}/diffuse/ > you're missing somethint in changelog No. The first %changelog entry quoted in comment 4 is fine. > macronify more. No. The request in comment 5 leads to macro-madness. Only in some cases it is convenient to replace the program name with %{name} -- e.g. if you want to reuse a spec file for other packages. It doesn't happen often that a program changes its name frequently, so that using %name everywhere (even in URL and %files lists) would be justified. * The guidelines want the .desktop file to be validated in the %install section: https://fedoraproject.org/wiki/Packaging/Guidelines#desktop-file-install_usage * The guidelines include specific shell code for scrollkeeper-update and desktop-data-base usage: https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Scrollkeeper https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#desktop-database * Several directories are not included in %files: %dir %{_datadir}/gnome/help/diffuse %dir %{_datadir}/gnome/help/diffuse/C %dir %{_datadir}/omf/diffuse https://fedoraproject.org/wiki/Packaging/UnownedDirectories * Rest is good. Another update: Spec URL: http://coralbark.net/fedora/diffuse/diffuse.spec SRPM URL: http://coralbark.net/fedora/diffuse/diffuse-0.2.15-4.fc10.src.rpm RPM URL: http://coralbark.net/fedora/diffuse/diffuse-0.2.15-4.fc10.noarch.rpm I've addressed the points from comment #9. I can see that I was very naive when I initially submitted the spec file on the grounds that it seemed to work fine on the systems I installed it on. Thanks Michael (and everyone else) for the helpful comments Yes, lots of package spec files "out there" seem to work, but there are many packaging pitfalls, and suddenly the packages break in unexpected ways. ;-) [...] The packaging is fine now. diffuse-0.2.15-4.fc10.src.rpm : APPROVED [...] I see you don't have any other review requests currently, and the NEEDSPONSOR keyword is set. What are your plans with regard to https://fedoraproject.org/wiki/PackageMaintainers/Join ? Re: Comment #11 I've been giving some thought to my sponsorship request. I would like to be sponsored but I don't (at least initially) want to maintain many packages. On the other hand, this package isn't very good evidence that I'm competent. Would a number of informal package reviews be enough? Yes, good idea. New Package CVS Request ======================= Package Name: diffuse Short Description: Graphical tool for comparing and merging text files Owners: jonquark Branches: F-9 F-10 EL-5 InitialCC: cvs done. diffuse-0.2.15-4.fc10 has been submitted as an update for Fedora 10. http://admin.fedoraproject.org/updates/diffuse-0.2.15-4.fc10 diffuse-0.2.15-4.fc10 has been pushed to the Fedora 10 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with su -c 'yum --enablerepo=updates-testing update diffuse'. You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-2310 diffuse-0.2.15-4.fc10 has been pushed to the Fedora 10 stable repository. If problems still persist, please make note of it in this bug report. Package Change Request ====================== Package Name: diffuse New Branches: epel7 Owners: cicku Git done (by process-git-requests). |