Spec URL: http://katiska.org/classified_ads/srpm/classified_ads.spec SRPM URL: http://katiska.org/classified_ads/srpm/classified_ads-0.04-1.fc21.src.rpm Description: Classified ads is an attempt to re-produce parts of the functionality that went away when Usenet news ceased to exist. This attempt tries to fix the problem of disappearing news-servers so that there is no servers required ; data storage is implemented inside client applications that you and me are running. Fedora Account System Username: costello This is my very first attempt to contribute to content of Fedora linux distribution. This is my own attempt to handle human-to-human communications inside internet, from end-users perspective. As a long-time internet-user this is to-day more or less the way I see how messaging should happen. Features of the program include, but are not limited to: * Sending and retrieving messages public and private, between humans or inside groups * No need for server-side support of any kind * Minimal hassle for the end-user * No need for contracts with any service-operators, not counting your ISP * Identification of message senders while allowing some withdrawal of personal details * Text-based search of public posting * Unfortunately no text-based or mobile UI yet, only Qt. * Early stage of development ; while basic functions seem to be all right there is surely bugs, "features" and 2 million fatal errors. As developer and user of the program I naturally will maintain it for fedora too if this piece of sw is deemed suitable for distribution. As this is my very first submission, my understanding is that I need a sponsor to review my package first.
_ → - in name QMAKE_ARGS+="INCLUDEPATH+=${LOCALBASE}/include/miniupnpc/ LIBS+=-L${LOCALBASE}/lib" Since you don't export those variables this has no effect. The paths look wrong too — they are not multiarch at least. Maybe somebody who knows how to package qmake-qt4-based stuff will chime in. Remove %clean, BuildRoot, unless you'll be building this also for EPEL5. Why fontconfig in Requires? Most likely various things in Requires are unnecessary. Dependencies on libraries are added automatically. Try removing all of them, building the package, looking at autogenerated requires, and maybe adding something back. No license file? Since this seems to be a graphical application, you need an appdata file too. Use %{version} in URL. Use the URL you have for Source0, and use a link to a human-readable web-page in URL.
(In reply to Zbigniew Jędrzejewski-Szmek from comment #1) > _ → - in name > > QMAKE_ARGS+="INCLUDEPATH+=${LOCALBASE}/include/miniupnpc/ > LIBS+=-L${LOCALBASE}/lib" .. Thank you for your comment Zbigniew ; if I manage to address these issues somehow, how do I proceed then? Just replace the faulty spec file at my URL above and make a notice in thread of this ticket that there has been an attempt to fix problems ; or file a new ticket or what? -- Antti, being clueless of the process
Make the changes in the specfile, including bumping the Release field and adding a line in %changelog. Then recreate the srpm and a comment with new Spec URL: and SRPM URL: fields. Use the same URL for the spec file. This way it is still possible to extract the old spec file from the old SRPM, but the spec file matches the name of the package.
> _ → - in name Please do point at the packaging guidelines more often than not: https://fedoraproject.org/wiki/Packaging:Guidelines#Naming -> https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Common_Character_Set_for_Package_Naming -> https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Separators | | packages where the upstream name naturally contains an underscore are | excluded from this. [...] Consider pointing the fedora-review tool at review tickets like this. It evaluates the "Spec URL:" and "SRPM URL:" lines, downloads the latest files, builds the packages on your machine, and then performs many helpful checks on the results.
Just a quick question about this name change .. while I'm the upstream, it is technically possible to rename the package. Now it seems to me that rpmbuild wants to find sources inside tarball in directory named <packagename>-<version> and if I change package name, the directory name inside tarball needs to change too, right? If I want to use the tarball downloadable from github as source, I need to re-name the sw (maybe project) in github also. Other linux distributions make similar assumptions about naming of the directories and if I now change the name, I very easily have a situation that I need to re-name the package in other linux distributions too and now, this is not that easy thing to do. So, one option is to put releases for fedora in other location than github release-tag, and have it packaged into tarball differently (different directory name only I think) - or is there better suggestions, anyone? -- Antti
Since the underscore is in the upstream name, and the package is already used, then keep it the way it is. It's nicer to have dashes then underscores in a name, but consistency is the most important. (BTW: you *can* override of the directory with -n new-dir-name. But I'm not suggesting that you do that.)
Spec URL: http://katiska.org/classified_ads/srpm/classified-ads.spec SRPM URL: http://katiska.org/classified_ads/srpm/classified-ads-0.04-2.fc21.src.rpm Dear Sirs, Changes 0.04-1 -> 0.04-2 * _ → - in name in spec file. * %build stage is now much simpler. * no %clean or %BuildRoot. * Dependencies are figured out automatically, seems all right to me. * Added license file to spec, was already in sources. * Added appdata. * Both URL: and Source0: now point to relevant URLs. * Huge original high-resolution bitmaps removed from source as they were not needed in build-process. * Manpage partly re-written in less personal tone. Zbigniew above suggested keeping the Spec URL same but as I managed to change name of the package, the spec name also now contains hyphen instead of underscore in URL also. Some changes (appdata etc.) required changes to source, changes have been pushed upstream but release not tagged yet in github where Source0: of .spec is now pointing to. For this reason rpmlint gives me a file-size mismatch warning as it seems to check size of tarball of actual rel 0.04 while this is 0.04+additions.
Yep, spec file looks good. But you still need a sponsor. I'd suggest doing a few reviews of other packages (without actually approving them or setting the fedora-review flag, since you haven't been sponsored yet). This always helps.
Ok, great, after un-necessarily twiddling with the fedora-review -flag I don't know if this bugreport is still having correct status to go forward? Regardless of the flag value, I'd still be in need of a sponsor for my package so please correct the flag if it makes any difference..
The flag is OK.
> now it seems to me that rpmbuild wants to find sources inside tarball in > directory named <packagename>-<version> and if I change package name, the > directory name inside tarball needs to change too, right? Just for the record, the %setup macro is flexible enough due to its various options. It could handle a different top-level directory. It just defaults to -n %{name}-%{version}. Consider pointing the fedora-review tool at this ticket: fedora-review -b 1202063 It evaluates the "Spec URL:" and "SRPM URL:" lines, downloads the latest files, performs local test-builds and runs various checks on the build results. Please take that as a little exercise to begin with.
Spec URL: http://katiska.org/classified_ads/srpm/classified-ads.spec SRPM URL: http://katiska.org/classified_ads/srpm/classified-ads-0.05-1.fc21.src.rpm Ok, fedora-review was very helpful, issues raised and fixed are these: - Sources used to build the package match the upstream source, as provided in the spec URL. Note: Upstream MD5sum check error, diff is in /tmp/review-1202063/1202063 -classified-ads/diff.txt See: http://fedoraproject.org/wiki/Packaging/SourceURL -> actual release (0.05) now built - All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. Note: These BR are not needed: gcc-c++ See: http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2 -> gcc-c++ dependency gone - Package uses either %{buildroot} or $RPM_BUILD_ROOT Note: Using both %{buildroot} and $RPM_BUILD_ROOT See: http://fedoraproject.org/wiki/Packaging/Guidelines#macros -> %{buildroot} selected - 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. Note: Cannot find LICENSE in rpm(s) See: http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text -> The binary rpm does install /usr/share/licenses/classified-ads/LICENSE and in spec that is done using %license keyword - is the tool failing to detect that or what? No change done due to this reported issue. - Package installs a %{name}.desktop using desktop-file-install or desktop- file-validate if there is such a file. -> validation added [!]: Uses parallel make %{?_smp_mflags} macro. -> used %{?_smp_mflags} in make Note: Directories without known owners: /usr/share/app-install, /usr/share/app-install/icons, /usr/share/classified-ads -> now included directories in spec
(In reply to Antti Järvinen from comment #12) > - 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. > Note: Cannot find LICENSE in rpm(s) > See: > http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text > -> > The binary rpm does install /usr/share/licenses/classified-ads/LICENSE > and in spec that is done using %license keyword - is the tool failing > to detect that or what? No change done due to this reported issue. fedora-review hasn't been adapted yet to the relatively new %license macro. I think a patch is in review to fix that.
And according to http://koji.fedoraproject.org/koji/taskinfo?taskID=9345035 it seems to compile also on ARM..
Spec URL: http://katiska.org/classified_ads/srpm/classified-ads.spec SRPM URL: http://katiska.org/classified_ads/srpm/classified-ads-0.07-1.fc21.src.rpm Updated to latest upstream release, packaging is the same. Changes are mostly about how build-system handles intermediate bitmaps, resulting functionality has not changed.
Spec URL: http://katiska.org/classified_ads/srpm/classified-ads.spec SRPM URL: http://katiska.org/classified_ads/srpm/classified-ads-0.09-1.fc24.src.rpm Updated to latest upstream release (mostly bugfixes, minor feature additions), packaging is almost the same: spec file is not the same as in 0.09 github tag, it has %lang(..) additions to make rpmlint keep quiet, otherwise as in 0.09 tag. Fedora-review tool is happy, except for the upstream .tar.gz checksum as is expected. Still looking for a sponsor for this package.
FESCo voted to allow anybody to do initial reviews, seperately from the sponsorship process [https://fedorahosted.org/fesco/ticket/1499]. I'll take this review. - suggestion: put BuildRequires each on a separate line, this makes it easier to spot mistakes. - suggestion: add empty lines before %description, before %changelog, before %files. - suggestion: use "%make_install INSTALL_ROOT=%{buildroot}" for the make line (shorter is better). - suggestion: use '.*' instead of '.gz' for the man pages. This will avoid issues if the compression ever changes. - You should use %find_lang macro instead of explicitly listing files. See https://fedoraproject.org/wiki/PackagingDrafts/find_lang. - DISPLAY= classified-ads dumps core :( (Not a packaging issue, just pointing it out.) classified-ads.src: W: file-size-mismatch classified-ads-0.09.tar.gz = 2287384, https://github.com/operatornormal/classified-ads/archive/0.09.tar.gz#/classified-ads-0.09.tar.gz = 2288561 3 packages and 0 specfiles checked; 0 errors, 1 warnings. So... no major issues. Please update the tarball and maybe fix the other things.
?
Ok, great. There is a new upstream release coming in a few weeks, release testing is ongoing. My suggestion is that as new version is packaged, the problems listed by Zbigniew are fixed in the process. -- Antti
If you prefer, you can wait... but those (small) packaging issues are mostly independent of upstream version. I don't see why this review should not be finished without waiting a few weeks for upstream release.
Spec URL: http://katiska.org/classified_ads/srpm/classified-ads.spec SRPM URL: http://katiska.org/classified_ads/srpm/classified-ads-0.10-1.fc25.src.rpm Ok Sirs, here is what is intended for 0.10 release. There is a new upstream release with additional features and also attempt to address problems related to packaging. Details below: > - suggestion: put BuildRequires each on a separate line, this makes it easier to spot mistakes. Now done. > - suggestion: add empty lines before %description, before %changelog, before %files. Now done. - suggestion: use "%make_install INSTALL_ROOT=%{buildroot}" for the make line (shorter is better). Not done, $DESTDIR is still required by makefile or translation files will end up in wrong location. ..partially due to stupidity in makefile generation but un-trivial to fix in qt environment. - suggestion: use '.*' instead of '.gz' for the man pages. This will avoid issues if the compression ever changes. Now done. - You should use %find_lang macro instead of explicitly listing files. See https://fedoraproject.org/wiki/PackagingDrafts/find_lang. Now done. - DISPLAY= classified-ads dumps core :( (Not a packaging issue, just pointing it out.) Now done. Reason for this not being done before was issues with wayland but there may be a workaround. The workaround is not tested with wayland so please report any issues. Rpmlint seems quiet.
%make_install includes $DESTDIR: $ rpmbuild --eval %make_install /usr/bin/make install DESTDIR=/home/zbyszek/rpmbuild/BUILDROOT/%{name}-%{version}-%{release}.x86_64 Similarly %make_build is preferred to make %_smp_mflags. You must add install scriptlets for the desktop file: https://fedoraproject.org/wiki/Packaging:Scriptlets#desktop-database, https://fedoraproject.org/wiki/Packaging:Scriptlets#Icon_Cache. This is going away soon, but unfortunately it's still required. There should be blank lines between %changelog entries (before any line with "*"). (For example, during mass rebuilds a script will add a changelog entry with a blank line automatically, and it'll look super ugly to have different formatting.) You shouldn't use %define: https://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define. And that definition should be moved somewhere to the top of the file (e.g. before %description). I checked the license headers, and the files specify headers of LGPLv2 or later, except for textedit/textedit.*. Those files specify LGPLv2 or LGPLv3+. You could license the result as LGPLv2 or LGPLv3+, but just LGPLv2 is also acceptable. No issue here, just making a note. In the manpage: "users ability" → "user's ability" or "users' ability", and similarly for "users encryption keys". + latest version + license is acceptable (LGPLv2) + license tag matches sources + license file is present, %license is used + builds and installs OK + rpmlint is quiet
All Right, spec+rpm have been updated in previously announced location. Changes this time include: - Install happens with %make_install. INSTALL_ROOT is still required in addition to DESTDIR so length did not change much. - Build happens with %make_build - Scriptlets added for proper handling of desktop files. Proper usage to be verified by someone more knowledgeable. - %define gone. Reason for %define was getting rid of broken debuginfo rpm. Debuginfo rpm is fixed and %define thus gone. - Manpage typos fixed. -- Antti Järvinen
OK, great. So we're done with the package. I can sponsor you into the packagers group. Please do two or three reviews of packages from http://fedoraproject.org/PackageReviewStatus/NEW.html, and paste the links here. It's best to start with the output of fedora-review and going over the checklist it produces, and also rpmlint (although it has many false positives, so don't take it too seriously).
All right, I have done two more initial reviews for programs implemented in technologies that I know at least something about, here: https://bugzilla.redhat.com/show_bug.cgi?id=1323334 is Qt gui for "pass" password manager, and https://bugzilla.redhat.com/show_bug.cgi?id=1313828 is a upnp-implementation for mono, both look like reasonable candidates to be included. -- Antti
Very good reviews. I made some comments on the first one. Please consider assigning them to yourself. You are now added to the packagers group. Have fun, and feel free to ping me if you have any questions (zbyszek in #fedora-devel, zbyszek at in.waw.pl).
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/classified-ads
Thanks Zbigniew&Jon :)