Spec URL: http://mkrizek.fedorapeople.org/yourls.spec SRPM URL: http://mkrizek.fedorapeople.org/yourls-1.5-1.fc15.src.rpm Description: Hi! This is a package for yourls (http://yourls.org) -- URL shortening service that you can run on your own server. Review would be much appreciated since this package will be used by AutoQA (http://fedoraproject.org/wiki/AutoQA). Also, because this is my first package I will need a sponsor. Thank you! rpmlint output: W: invalid-url Source0: http://yourls.googlecode.com/files/yourls-1.5.zip HTTP Error 404: Not Found I find this strange since wget http://yourls.googlecode.com/files/yourls-1.5.zip works fine. Am I missing something?
If you need a sponsor, the value "FE-NEEDSPONSOR" has to be set in the "Blocks" field. I've done this for you.
Sorry I can not review the SPEC but I will try and point out some things I see: in this line of sed: sed -e "s/dirname(dirname(__FILE__)).'\/user\/config.php'/'\%{_sysconfdir}\/%{name}\/config.php'/g" \ -e "s/config.php in \/user\//config.php in \%{_sysconfdir}\/%{name}\//g" \ -i ./includes/load-yourls.php you can use | instead of / so it will look cleaner and easier to read. License is wrong I couldn't find the exact version of the GPL the application is licensed so license field should be GPL+
Thank you very much for your comments, updated versions follows: Spec URL: http://mkrizek.fedorapeople.org/yourls.spec SRPM URL: http://mkrizek.fedorapeople.org/yourls-1.5-2.fc15.src.rpm
%defattr(-,root,root,-) %config(noreplace) %{_sysconfdir}/%{name}/config.php %config(noreplace) %{_sysconfdir}/httpd/conf.d/yourls.conf %doc changelog.txt readme.html sample-public-api.php.txt %doc sample-public-front-page.php.txt sample-remote-api-call.php.txt %dir %{_sysconfdir}/%{name}/ %{_datadir}/%{name}/ Instead you can do: config(noreplace) %{_sysconfdir}/%{name}/ config(noreplace) %{_sysconfdir}/httpd/conf.d/%{name}.conf %doc changelog.txt readme.html sample-public-api.php.txt %doc sample-public-front-page.php.txt sample-remote-api-call.php.txt %{_datadir}/%{name}/ If you are also going to build for RHEL then leave %defattr(-,root,root,-). Also the %clean section isn't needed unless for RHEL either BuildRoot: %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXX) Isn't needed unless for RHEL as well
> Instead you can do: > ... Unconvincing. Explicitly listing specific files in a %files section can be beneficial, not limited to config files. Such a files section makes a build fail whenever the specified file gets renamed or lost. That would not be the case when using directory tree inclusion as you suggested. The combination of %dir %{_sysconfdir}/%{name}/ %config(noreplace) %{_sysconfdir}/%{name}/config.php is fine.
(In reply to comment #5) > > Instead you can do: > > ... > > Unconvincing. > > Explicitly listing specific files in a %files section can be beneficial, not > limited to config files. Such a files section makes a build fail whenever the > specified file gets renamed or lost. That would not be the case when using > directory tree inclusion as you suggested. > > The combination of > > %dir %{_sysconfdir}/%{name}/ > %config(noreplace) %{_sysconfdir}/%{name}/config.php > > is fine. Well it seem almost every person that reviews my packages have different opinions. I can list every single file and they recommend to condense it a little and if I condense it too much, I need to list more.
@Martin: Sorry for the bit of disagreement. Some people that reviewed my packages recommend listing more files in some cases and others the opposite. Do as Michael has suggested, he has done this longer than I have.
Thanks Nathan and Michael, I will leave it as it is.
Hi Martin, I can sponsor you if you're willing to do a few informal reviews in order to show an understanding of the review guidelines. Please choose an uncommented review request not blocked by FE-NEEDSPONSOR, e.g. bug #729512 and/or bug #728649. An additional note on your spec: Please modify the sources with a patch rather than using sed. This ensures that rpmbuild fails if the modifications can't be applied for some reason. Thus you avoid building the package with silently failed sed substitutions leading to wrong file paths in the php code.
Thanks Martin, updated versions follow: Spec URL: http://mkrizek.fedorapeople.org/yourls.spec SRPM URL: http://mkrizek.fedorapeople.org/yourls-1.5-3.fc15.src.rpm
Here's the formal review. The package is almost ready. There are two things left that need to be fixed: - The package contains several files under different licenses, so we have a multiple licensing scenario here: * yourls files: GPL+ * GeoIP files: LGPLv2+ * JQuery files: MIT or GPLv2 => the License field should look like this: GPL+ and LGPLv2+ and (MIT or GPLv2) Also, add a comment about the multiple licensing scenario and the corresponding files above the License field. - Since the yourls archive doesn't contain a license file, ask upstream to add one to a future release. - Some of the doc files have DOS line endings (see rpmlint output). You can fix this by adding the following loop to the %prep section: for f in *.txt; do sed 's/\r//' $f > $f.new && touch -r $f $f.new && mv $f.new $f done $ rpmlint *.rpm yourls.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/yourls-1.5/sample-public-api.php.txt yourls.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/yourls-1.5/sample-public-front-page.php.txt yourls.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/yourls-1.5/changelog.txt yourls.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/yourls-1.5/sample-remote-api-call.php.txt yourls.src: W: invalid-url Source0: http://yourls.googlecode.com/files/yourls-1.5.zip HTTP Error 404: Not Found 2 packages and 0 specfiles checked; 0 errors, 5 warnings. --------------------------------- key: [+] OK [.] OK, not applicable [X] needs work --------------------------------- [+] MUST: The package must be named according to the Package Naming Guidelines. [+] MUST: The spec file name must match the base package %{name}. [+] MUST: The package must meet the Packaging Guidelines. [+] MUST: The package must be licensed with a Fedora approved license. - multiple licensing scenario: * GeoIP: LGPLv2+ * JQuery: MIT or GPLv2 [X] MUST: The License field in the package spec file must match the actual license. [.] MUST: The file containing the text of the license(s) for the package must be included in %doc. [+] MUST: The spec file must be written in American English. [+] MUST: The spec file for the package MUST be legible. [+] MUST: The sources used to build the package must match the upstream source. $ md5sum yourls-1.5.zip* fa32943f7d669640f08670ecdcde5ad1 yourls-1.5.zip fa32943f7d669640f08670ecdcde5ad1 yourls-1.5.zip.upstream [+] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture. [.] MUST: If the package does not successfully compile, build or work on an architecture. [.] MUST: All build dependencies must be listed in BuildRequires. [.] MUST: When compiling C, C++, or Fortran files, %{optflags} must be applied. [.] MUST: The spec file MUST handle locales properly. [.] MUST: Packages storing shared library files (not just symlinks) must call ldconfig in %post and %postun. [+] MUST: Packages must NOT bundle copies of system libraries. [.] MUST: If the package is designed to be relocatable, ... [+] MUST: A package must own all directories that it creates. [+] MUST: A Fedora package must not list a file more than once in %files. [+] MUST: Permissions on files must be set properly. [+] MUST: Each package must consistently use macros. [+] MUST: The package must contain code, or permissable content. [.] MUST: Large documentation files must go in a -doc subpackage. [+] MUST: Files in %doc must not affect the runtime of the application. [.] MUST: Header files must be in a -devel package. [.] MUST: Static libraries must be in a -static package. [.] MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package. [.] MUST: devel packages must require the base package using a fully versioned dependency. [.] MUST: Packages must NOT contain any .la libtool archives. [.] MUST: Packages containing GUI applications must include a %{name}.desktop file. [+] MUST: Packages must not own files or directories already owned by other packages. [+] MUST: All filenames in rpm packages must be valid UTF-8. EPEL <= 5 only: [+] MUST: The spec file must contain a valid BuildRoot field. [+] MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot}. [+] MUST: Each package must have a %clean section, which contains rm -rf %{buildroot}. [.] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' [X] SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [+] SHOULD: The reviewer should test that the package builds in mock. [+] SHOULD: The package should compile and build into binary rpms on all supported architectures. [+] SHOULD: The reviewer should test that the package functions as described. [.] SHOULD: If scriptlets are used, those scriptlets must be sane. [.] SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency. [.] SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg. [+] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself. [.] SHOULD: Your package should contain man pages for binaries/scripts.
(In reply to comment #11) > ... > - Since the yourls archive doesn't contain a license file, ask upstream to > add one to a future release. > http://code.google.com/p/yourls/issues/detail?id=911
(In reply to comment #11) > Here's the formal review. The package is almost ready. There are two things > left that need to be fixed: > > - The package contains several files under different licenses, so we have a > multiple licensing scenario here: > * yourls files: GPL+ > * GeoIP files: LGPLv2+ > * JQuery files: MIT or GPLv2 > > => the License field should look like this: > GPL+ and LGPLv2+ and (MIT or GPLv2) > Also, add a comment about the multiple licensing scenario and the > corresponding files above the License field. > > - Some of the doc files have DOS line endings (see rpmlint output). You can > fix this by adding the following loop to the %prep section: > > for f in *.txt; do > sed 's/\r//' $f > $f.new && > touch -r $f $f.new && > mv $f.new $f > done > Fixed: $ rpmlint rpmbuild/SPECS/yourls.spec rpmbuild/SRPMS/yourls-1.5-4.fc15.src.rpm rpmbuild/RPMS/noarch/yourls-1.5-4.fc15.noarch.rpm rpmbuild/SPECS/yourls.spec: W: invalid-url Source0: http://yourls.googlecode.com/files/yourls-1.5.zip HTTP Error 404: Not Found yourls.src: W: invalid-url Source0: http://yourls.googlecode.com/files/yourls-1.5.zip HTTP Error 404: Not Found 2 packages and 1 specfiles checked; 0 errors, 2 warnings. Spec URL: http://mkrizek.fedorapeople.org/yourls.spec SRPM URL: http://mkrizek.fedorapeople.org/yourls-1.5-4.fc15.src.rpm
OK, the package looks good now and is ready for check-in. The next step is to request a Git repo for the package: http://fedoraproject.org/wiki/Package_SCM_admin_requests Welcome to the packager group. Hope you'll have fun to provide and review packages. :) ---------------- Package APPROVED ----------------
Thank you very much for sponsoring me and reviewing the package, Martin! New Package SCM Request ======================= Package Name: yourls Short Description: Your Own URL Shortener Owners: mkrizek Branches: f14 f15 f16 el6 InitialCC:
Git done (by process-git-requests).