SPEC URL: http://monnerat.fedorapeople.org/mate-applet-softupd.spec SRPM URL: http://monnerat.fedorapeople.org/mate-applet-softupd-0.2.5-1.fc18.src.rpm Description: Software updates notification applet for the MATE desktop environment. Uses PackageKit for notification and yumex for updates. rpmlint output: spec: none srpm: none rpm: none debuginfo: none
Hi, i catch this review because i build this package also for my external additional mate-repo and i like it to be in fedora. see http://forums.mate-desktop.org/viewtopic.php?f=8&t=1478 I will give you tomorrow some hints.
Why did you patch the upstream code? The first part (configure.ac) is only cosmetics in my eyes. And patching mate-applet-softupd.spec.in is useless because you use your own spec for building the rpm. mate-applet-softupd.spec.in is for building a rpm directly from the tar ball. The package builds fine without patching it. If you don't have another serious reason for using the patch, please remove them. @ your spec file 1. remove %global pixmapdir %{_datadir}/pixmaps %global iconsdir %{_datadir}/icons %global localedir %{_datadir}/locale configure choose those paths withhout setting global defines by hand. 2. Why you use 'rm -rf m4' and create them new? And if you want to do this you can simply use autoreconf -i -f to create the *.m4 files new. 3. Using BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) is obsolete, you don't need it anymore. https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag 4. remove rm -rf "${RPM_BUILD_ROOT}" from install section, this obsolete for the same reason. 5. change make %{?_smp_mflags} CFLAGS="${CFLAGS}" to make %{?_smp_mflags} https://fedoraproject.org/wiki/Packaging:Guidelines#Parallel_make In result you have to change make install DESTDIR="${RPM_BUILD_ROOT}" to make install DESTDIR=$RPM_BUILD_ROOT and rm -rf "${RPM_BUILD_ROOT}%{_docdir}" to rm -rf $RPM_BUILD_ROOT%{_docdir} 6. pls use macros for find langugage. %find_lang %{name} and %files -f %{name}.lang https://fedoraproject.org/wiki/Packaging:Guidelines#Handling_Locale_Files 7. pls use valid rpm scriptlets for the icon cache https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache %post /bin/touch --no-create %{_datadir}/icons/hicolor &>/dev/null || : %postun if [ $1 -eq 0 ] ; then /bin/touch --no-create %{_datadir}/icons/hicolor &>/dev/null /usr/bin/gtk-update-icon-cache %{_datadir}/icons/hicolor &>/dev/null || : fi %posttrans /usr/bin/gtk-update-icon-cache %{_datadir}/icons/hicolor &>/dev/null || : 8. using a clean section is obsolete, pls remove them. https://fedoraproject.org/wiki/Packaging:Guidelines#.25clean 9. using %defattr(-, root, root, -) is obsolete if you don't want to build for EPEL5, pls remove. https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions 10. pls remove all those lines in the spec file. #------------------------------------------------------------------------------- I know your spec file is based from Assen's one, but this is a personal format style and shoudn't use for fedora. https://fedoraproject.org/wiki/Packaging:Guidelines#Writing_a_package_from_scratch Ok, that's all for the moment :) , pls upload a new spec file and SRPM version. Than i can use the review-tool and rpmlint.
> The first part (configure.ac) is only cosmetics in my eyes. No it isn't. It fixes an error that occurs only if no backend is found. However I agree this is most unlikely to occur in our case. > If you don't have another serious reason for using the patch, please remove them. Yes, it also adds a missing french translation. The patch is a post-0.2.5 adjustment that has been transmitted upstream and accepted as a whole. That's why it includes fixes to the upstream spec file. I agree we don't need the whole patch: if it really disturbs you, I can make a patch that only deals with configure.ac and the missing french string. > 1. remove Done > 2. Why you use 'rm -rf m4' and create them new? Since I have to autoconf (because of configure.ac change), I prefer to use system macros that are in phase with the autoconf version. And yes, I have replaced the whole auto-things by autoreconf as you suggest. > 3. Using BuildRoot ... is obsolete Removed I hope it will also be obsolete in EPEL whenever this package (and Mate) will hit it: although I do not plan to deal with EPEL myself, I welcome comaintainers for that platform and I like to ease their work and unify the spec file when possible. > 4. remove rm -rf "${RPM_BUILD_ROOT}" from install section Done Same EPEL remark > 5. change make %{?_smp_mflags} CFLAGS="${CFLAGS}" ... Done: this was remaining from the time this package did not use automake. > 6. pls use macros for find langugage. Done > 7. pls use valid rpm scriptlets for the icon cache Done, although they were perfectly valid, even if not matching guideline characters one to one. About quoting: you made me remove quoting everywhere. I did it but I do not agree: this weakens the spec file in case the substituted macro/shellvar value ever contains a shell special character. > 8. using a clean section is obsolete Done. See EPEL remark. > 9. using %defattr(-, root, root, -) is obsolete Removed > 10. pls remove all those lines in the spec file. Done, although I find it much less readable. > pls upload a new spec file and SRPM version. SPEC URL: http://monnerat.fedorapeople.org/mate-applet-softupd.spec SRPM URL: http://monnerat.fedorapeople.org/mate-applet-softupd-0.2.5-2.fc18.src.rpm Thanks for your work!
(In reply to comment #3) > > The first part (configure.ac) is only cosmetics in my eyes. > No it isn't. It fixes an error that occurs only if no backend is found. > However I agree this is most unlikely to occur in our case. agree > > If you don't have another serious reason for using the patch, please remove them. > Yes, it also adds a missing french translation. > The patch is a post-0.2.5 adjustment that has been transmitted upstream and > accepted as a whole. That's why it includes fixes to the upstream spec file. > I agree we don't need the whole patch: if it really disturbs you, I can make > a patch that only deals with configure.ac and the missing french string. You have to update you tarball, as Assen told me this morning the french translation is include in 0.2.5. (org.mate.applets.SoftupdApplet.mate-panel-applet.in) I'm still wondering why he doesn't fix the typo in configure.ac, maybe he needs a reminder. > > 1. remove > Done > > > 2. Why you use 'rm -rf m4' and create them new? > Since I have to autoconf (because of configure.ac change), I prefer to use > system macros that are in phase with the autoconf version. agree in this case I forgot to say you don't need BR automake, it will be already called by autoconf. But you can change this later. Review results comming soon. PS: my irc nickname is raveit65 on fedora and mate channels
review, rpmlint and licencecheck results are OK. But i really don't see the need of patching mate-applet-softupd.spec.in You don't need this for building and believe me Assen would never change his spec file, because he use it for a different situation. Pls remove this part and the already in upstream applied french translations part, and i will approved your request.
> You have to update you tarball, as Assen told me this morning the french translation is include in 0.2.5. (org.mate.applets.SoftupdApplet.mate-panel-applet.in) > I'm still wondering why he doesn't fix the typo in configure.ac, maybe he needs a reminder. The tarball I use is OK: it's the upstream 0.2.5. I sent the patch on friday 8/3 to Assen, telling him there's no hurry to release a 0.2.6 with it. In his reply, he told me all changes are accepted and applied to his personal SCM. The french translation is included in 0.2.5 tarball, but a single string was left out.
Ok, you're right, the french description is missing. But pls remove the part of mate-applet-softupd.spec.in from the patch. I see no argument to patch this file. And remove BR automake. And i need a new upload of SPEC and SRPM with this changes for the final review.
Ok , wait i will talk to Assen if he really want to change his spec file.
> I forgot to say you don't need BR automake, it will be already called by autoconf. Not true. However it's brought in by gtk2 (at least). In a mock build, BR autoconf only adds packages m4 and perl-Data-Dumper. Do you really want to rely on gtk2 BR?
Wolfgang, Patrick, Thanks for your help on this package. I did not expect it to cause such a large discussion. Let me explain briefly: 1. The spec file in my source tarball: in all my sources I always add a spec file. It is designed to be as generic as possible and will likely fail any check for compliance with strict distribution rules - like Fedora's. It is mostly for my usage, because I try to provide generic RPMs whenever feasible (my software usually has only few dependencies, not hard to track). To be able to quickly build RPMs for different architectures (and use cases), I prefer to build the RPM file in-tree using the Makefile with 'make rpm'; therefore, my spec files usually have "%setup" and "./configure" commented out - my usual workflow is to run ./configure wth proper parameters (even if it is cross-compile), then "make" and "make rpm". I know that there are (and I have used) dedicetdd tols like mock, but still I find it easier and quicker to buil RPMs in-tree. I can't tell whether Patrick should patch my RPM file or keep an entirely new one; to me, Fedora seems too cutting-edge in terms of spec files (e.g., I don't want to lose, in general, the ability to build on/for RHEL - or cut off things which will work on other RPM-based distros), so I guess it is better for the package maintainer to decide. 2. The patch: when I released 0.2.5, one of the descriptions in French, sent me by Patrick, was missing. This is fixed in my trunk along with the configure.ac. I can tag and send you a 0.2.6 tarball right away, if this will make it easier for you. Otherwise, youcan apply the patch as it is written by Patrick. WWell,
> Wolfgang, Patrick, > > Thanks for your help on this package. I did not expect it to cause such a > large discussion. > > Let me explain briefly: > > 1. The spec file in my source tarball: in all my sources I always add a spec > file. It is designed to be as generic as possible and will likely fail any > check for compliance with strict distribution rules - like Fedora's. It is > mostly for my usage, because I try to provide generic RPMs whenever feasible > (my software usually has only few dependencies, not hard to track). > > To be able to quickly build RPMs for different architectures (and use > cases), I prefer to build the RPM file in-tree using the Makefile with 'make > rpm'; therefore, my spec files usually have "%setup" and "./configure" > commented out - my usual workflow is to run ./configure wth proper > parameters (even if it is cross-compile), then "make" and "make rpm". I know > that there are (and I have used) dedicetdd tols like mock, but still I find > it easier and quicker to buil RPMs in-tree. This is exactly what i thought. So it makes no sense to patch mate-applet-softupd.spec.in if upstream don't want to follow them. And as i said before, for building the package for fedora this file is useless. > > I can't tell whether Patrick should patch my RPM file or keep an entirely > new one; to me, Fedora seems too cutting-edge in terms of spec files (e.g., > I don't want to lose, in general, the ability to build on/for RHEL - or cut > off things which will work on other RPM-based distros), so I guess it is > better for the package maintainer to decide. > > 2. The patch: when I released 0.2.5, one of the descriptions in French, sent > me by Patrick, was missing. This is fixed in my trunk along with the > configure.ac. I can tag and send you a 0.2.6 tarball right away, if this > will make it easier for you. Otherwise, youcan apply the patch as it is > written by Patrick. > > WWell, Thanks for your comment. For me you shouldn't release a new release now, because it is valid to use the patch without mate-applet-softupd.spec.in, for correct the french translation which is in upstream. @ Patrick You're right with BR automake, don't change this. So pls, update SPEC and SRPM for the final review.
> I can't tell whether Patrick should patch my RPM file or keep an entirely new one; to me, Fedora seems too cutting-edge in terms of spec files (e.g., I don't want to lose, in general, the ability to build on/for RHEL - or cut off things which will work on other RPM-based distros), so I guess it is better for the package maintainer to decide. That's my idea too, and that's why the "misc" patch only change things in the included spec file that are applicable to generic rpmbuilds. I maintain a Fedora spec file separately and do not force it into the project. The original "misc" patch was primarily made for upstream and it was (originally) quicker and simpler to include it "as is" in the Fedora build since it is a superset of the needed fixes. I'm perfectly aware that changing the tarball spec file has no impact on the Fedora package. I've now implemented the needed changes into 2 patches: "badvarset" to fix configure.ac. "morefrench" to add the translated string. SPEC URL: http://monnerat.fedorapeople.org/mate-applet-softupd.spec SRPM URL: http://monnerat.fedorapeople.org/mate-applet-softupd-0.2.5-3.fc18.src.rpm @Assen: many thanks for your participation :)
APPROVED ! Package Review ============== Key: [x] = Pass [!] = Fail [-] = Not applicable [?] = Not evaluated [ ] = Manual review needed ===== MUST items ===== C/C++: [ ]: Package does not contain kernel modules. [ ]: Package contains no static executables. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs. Generic: [ ]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [ ]: %build honors applicable compiler flags or justifies otherwise. [ ]: Package contains no bundled libraries without FPC exception. [ ]: Changelog in prescribed format. [ ]: Sources contain only permissible code or content. [ ]: Package contains desktop file if it is a GUI application. [ ]: Development files must be in a -devel package [ ]: Package requires other packages for directories it uses. [ ]: Package uses nothing in %doc for runtime. [ ]: Package is not known to require ExcludeArch. [ ]: Package complies to the Packaging Guidelines [ ]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "*No copyright* GPL (v2 or later)". 1 files have unknown license. Detailed output of licensecheck in /home/rave/919469-mate-applet- softupd/licensecheck.txt [ ]: The spec file handles locales properly. [ ]: Package consistently uses macro is (instead of hard-coded directory names). [ ]: Package is named according to the Package Naming Guidelines. [ ]: Package does not generate any conflict. [ ]: Package obeys FHS, except libexecdir and /usr/target. [ ]: If the package is a rename of another package, proper Obsoletes and Provides are present. [ ]: Package must own all directories that it creates. [ ]: Package does not own files or directories owned by other packages. [ ]: Requires correct, justified where necessary. [ ]: Spec file is legible and written in American English. [ ]: Package contains systemd file(s) if in need. [ ]: gtk-update-icon-cache is invoked when required Note: icons in mate-applet-softupd [ ]: Useful -debuginfo package or justification otherwise. [ ]: Large documentation must go in a -doc subpackage. Note: Documentation size is 30720 bytes in 6 files. [x]: All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Each %files section contains %defattr if rpm < 4.4 [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Fully versioned dependency in subpackages, if present. [x]: Spec file lacks Packager, Vendor, PreReq tags. [x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %doc. [x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package do not use a name that already exist [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Packages must not store files under /srv, /opt or /usr/local [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: No rpmlint messages. ===== SHOULD items ===== Generic: [ ]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [ ]: Final provides and requires are sane (see attachments). [ ]: Package functions as described. [ ]: Latest version is packaged. [ ]: Package does not include license text files separate from upstream. [ ]: Patches link to upstream bugs/comments/lists or are otherwise justified. [ ]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [ ]: Package should compile and build into binary rpms on all supported architectures. [ ]: %check is present and all tests pass. [ ]: Packages should try to preserve timestamps of original installed files. [x]: Sources can be downloaded from URI in Source: tag [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: Dist tag is present. [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Uses parallel make. [x]: SourceX tarball generation or download is documented. [x]: SourceX is a working URL. [x]: Spec use %global instead of %define. ===== EXTRA items ===== Generic: [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Rpmlint is run on all installed packages. Note: No rpmlint messages. [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: mate-applet-softupd-0.2.5-3.fc19.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. Rpmlint (installed packages) ---------------------------- # rpmlint mate-applet-softupd 1 packages and 0 specfiles checked; 0 errors, 0 warnings. # echo 'rpmlint-done:' Requires -------- mate-applet-softupd (rpmlib, GLIBC filtered): /bin/sh PackageKit libatk-1.0.so.0()(64bit) libc.so.6()(64bit) libcairo.so.2()(64bit) libdbus-1.so.3()(64bit) libdbus-glib-1.so.2()(64bit) libfontconfig.so.1()(64bit) libfreetype.so.6()(64bit) libgdk-x11-2.0.so.0()(64bit) libgdk_pixbuf-2.0.so.0()(64bit) libgio-2.0.so.0()(64bit) libglib-2.0.so.0()(64bit) libgobject-2.0.so.0()(64bit) libgtk-x11-2.0.so.0()(64bit) libmate-panel-applet-4.so.1()(64bit) libmatenotify.so.1()(64bit) libpackagekit-glib2.so.16()(64bit) libpango-1.0.so.0()(64bit) libpangocairo-1.0.so.0()(64bit) libpangoft2-1.0.so.0()(64bit) libsqlite3.so.0()(64bit) rtld(GNU_HASH) yumex Provides -------- mate-applet-softupd: mate-applet-softupd mate-applet-softupd(x86-64) MD5-sum check ------------- http://www.zavedil.com/wp-content/uploads/2013/03/mate-applet-softupd-0.2.5.tar.gz : CHECKSUM(SHA256) this package : 9cd01e8d7e8c138367e26ec839f52b219bfa63f3a746eefc187d32c8a20f5526 CHECKSUM(SHA256) upstream package : 9cd01e8d7e8c138367e26ec839f52b219bfa63f3a746eefc187d32c8a20f5526 Generated by fedora-review 0.4.0 (660ce56) last change: 2013-01-29 Buildroot used: fedora-rawhide-x86_64 Command line :/usr/bin/fedora-review -v -b 919469 -m fedora-rawhide-x86_64
Many thanks for you work on this, Wolfgang.
New Package SCM Request ======================= Package Name: mate-applet-softupd Short Description: MATE Software Update Applet Owners: monnerat Branches: f18 f19 InitialCC:
Git done (by process-git-requests).
mate-applet-softupd-0.2.5-3.fc18 has been submitted as an update for Fedora 18. https://admin.fedoraproject.org/updates/mate-applet-softupd-0.2.5-3.fc18
Wolfgang, you should fill the empty checkbox in fedora-review, not paste the output without checking. Anyway, I have a few questions : - why does it requires both yumex and packagekit ? To me, that's redundant, since the configure will use the first one that will be seen, so forcing one or the other kinda defeat the point. And it will fallback on yum in all case it seems. - the patches should be commented in the spec https://fedoraproject.org/wiki/Packaging:Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment - I am not sure also that the directory ownership is clean, but that's pretty hard to see as mate is already installing lots of stuff without clear package ownership, and I am still looking on how to fill useful bug report for now based on my automated check :/
> - why does it requires both yumex and packagekit ? To me, that's redundant, since the configure will use the first one that will be seen, so forcing one or the other kinda defeat the point. And it will fallback on yum in all case it seems. This applet uses an external "backend" to check for updates, and an external "updater" to apply updates. These external applications have to be determined at CONFIGURE time, not dynamically at execution time. Thus they should be chosen at rpmbuild time. For an explanation about the choice used here, see NOTES 1A and 2A of http://www.zavedil.com/mate-software-updates-applet/. Note 1B explains yum-updatesd is not shipped by default and yum backend is ugly. These are the reasons that guided my (may be somewhat arbitrary) choice. > - the patches should be commented in the spec Patches have been sent upstream, but there's not target there to link to. > - I am not sure also that the directory ownership is clean Directory Package Comment %{_libexecdir} filesystem Core member: always here %{_datadir}/pixmaps filesystem " %{_datadir}/icons/icolor/*/apps hicolor-icon-theme Yes, may require it %{_datadir}/mate-panel/applets mate-panel Brough in by mate-panel-libs Require %{_datadir}/dbus-1/services dbus Yes, may require it
(In reply to comment #18) > Wolfgang, you should fill the empty checkbox in fedora-review, not paste the > output without checking. Michael, i did check the package for the other points but i forget to fil out them in the review text, respectively i didn't know that i have to fil out the empty checkbox. Thanks for your hint, i will do this in next reviews. > > Anyway, I have a few questions : > > - why does it requires both yumex and packagekit ? To me, that's redundant, > since the configure will use the first one that will be seen, so forcing one > or the other kinda defeat the point. And it will fallback on yum in all case > it seems. In my opinion using packagekit as backend is the best solution. I provide this package with yum-updatesd as backend for a half a year in my repo, which works well too. But the user have to enabled the daemon with systemd. Enable the daemon in the rpm isn't allowed according Package Guidelines as far as i know. > > > - the patches should be commented in the spec > https://fedoraproject.org/wiki/Packaging: > Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment > @ Patrick simply add comment to the patch, ie. adding french translation which is send to upstream. Or something else. > > - I am not sure also that the directory ownership is clean, but that's > pretty hard to see as mate is already installing lots of stuff without clear > package ownership, and I am still looking on how to fill useful bug report > for now based on my automated check :/ For me this looks OK. For example %{_datadir}/mate-panel/applets shouldn't definitely owned by mate-panel, because mate-panel comes with the main applets. For your information, i steped back to fedora mate team, checking directories ownership is on my list. If you have a tool for this, this would be helpful. @ Patrick Why you don't want to build for f17? The package works well on fc17 since half a year and there are mate user who use f17. ;)
> Why you don't want to build for f17? Because I don't have an installed F17 and I'll never have. I have a real lack of resources for it, both as hardware/space and my own time. If someone fills a BZ for F17 I would'nt be able to help. If you want to take it as a comaintainer, you're welcome. I do not have a tool for Requires/Provides closure and shortest path between to packages dependencies, through I'd love to have one. Of course such a tool could handle directories ownership. I think of it for a long time (some years ago, I even started to write something in C, but in the meantime, the rpm API (no yum at this time) has changed :-/) The problem is, once again, my own time resources.
... and forgive me, but I don't know why this f... BZ has stopped cutting long lines cleverly !
(In reply to comment #21) > > Why you don't want to build for f17? > Because I don't have an installed F17 and I'll never have. I have a real > lack of resources for it, both as hardware/space and my own time. If someone > fills a BZ for F17 I would'nt be able to help. > If you want to take it as a comaintainer, you're welcome. > done! Can you pls do a change Package SCM Request for 17? Than i will build the package for f17.
Package Change Request ====================== Package Name: mate-applet-softupd New Branches: f17 Owners: monnerat raveit65 InitialCC: raveit65 wants to maintain an F17 branch.
mate-applet-softupd-0.2.5-3.fc17 has been submitted as an update for Fedora 17. https://admin.fedoraproject.org/updates/mate-applet-softupd-0.2.5-3.fc17
mate-applet-softupd-0.2.5-3.fc18 has been pushed to the Fedora 18 testing repository.
mate-applet-softupd-0.2.5-3.fc18 has been pushed to the Fedora 18 stable repository.
mate-applet-softupd-0.2.5-3.fc17 has been pushed to the Fedora 17 stable repository.