Description: Polipo is a lightweight caching Web proxy that was designed as a personal cache. It is able to cache incomplete objects and will complete them using range requests. It will use HTTP/1.1 pipelining if supported by the remote server. SPEC: http://dl.dropbox.com/u/1338197/1/polipo.spec SRPM: http://dl.dropbox.com/u/1338197/1/polipo-1.0.4.1-1.fc12.src.rpm
Tor Browser Bundle contains Tor + Vidalia + Torbutton +Polipo + Firefox. All of them now are in the repo of fedora except polipo. See http://www.torproject.org/download.html.en
Since I'm not approved, I'm able to give you an unofficial review rpmlint polipo-1.0.4.1-1.fc12.src.rpm polipo.spec polipo.src: W: spelling-error %description -l en_US pipelining -> pipe lining, pipe-lining, pipeline polipo.src: W: invalid-url URL www.pps.jussieu.fr/~jch/software/polipo/ polipo.src: W: strange-permission polipo.init 0755 polipo.src:65: W: mixed-use-of-spaces-and-tabs (spaces: line 65, tab: line 1) polipo.spec:65: W: mixed-use-of-spaces-and-tabs (spaces: line 65, tab: line 1) 1 packages and 1 specfiles checked; 0 errors, 5 warnings. Looking deeper to the spec-file: - URL-Entry should start with http:// - yum provides /sbin/install-info lists info as package providing /sbin/install-info. If you definitely need it for pre and post-section, take info. https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRequires Lists info as package, that doesn't need to be included. - $RPM_BUILD_ROOT and %{buildroot}-Macros should not be mixed. (https://fedoraproject.org/wiki/Packaging:Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS) Mock builds fine. Ok, looking deeper: +:ok, =:needs attention, -:needs fixing [+] MUST: rpmlint must be run on every package. (see above) [+] 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. [FIXME?: covers this list and more] [+] MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines. [+] MUST: The License field in the package spec file must match the actual license. [+] MUST: 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 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, as provided in the spec URL. <<md5sum checksum>> [+] MUST: The package must successfully compile and build into binary rpms on at least one supported architecture. [+] MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. [-] MUST: All build dependencies must be listed in BuildRequires [=] MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. [*] MUST: Every binary RPM package which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. [*] MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review [-] MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. [+] MUST: A package must not contain any duplicate files in the %files listing. [+] MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line. [+] MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). [-] MUST: Each package must consistently use macros, as described in the macros section of Packaging Guidelines. [+] MUST: The package must contain code, or permissible content. This is described in detail in the code vs. content section of Packaging Guidelines. [+] MUST: Large documentation files should go in a doc subpackage. [+] MUST: If a package includes something as %doc, it 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: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability). [+] 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: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} [+] MUST: Packages must NOT contain any .la libtool archives, these should be removed in the spec. [+] MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. [+] MUST: Packages must not own files or directories already owned by other packages. [+] MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT). [+] MUST: All filenames in rpm packages must be valid UTF-8.
> Since I'm not approved, I'm able to give you an unofficial review > > rpmlint polipo-1.0.4.1-1.fc12.src.rpm polipo.spec > polipo.src: W: spelling-error %description -l en_US pipelining -> pipe lining, > pipe-lining, pipeline Not spelling error indeed,See http://en.wikipedia.org/wiki/HTTP_pipelining > polipo.src: W: invalid-url URL www.pps.jussieu.fr/~jch/software/polipo/ Fixed > polipo.src: W: strange-permission polipo.init 0755 most files in the /etc/rc.d/init.d should be 0755. > polipo.src:65: W: mixed-use-of-spaces-and-tabs (spaces: line 65, tab: line 1) Fixed > polipo.spec:65: W: mixed-use-of-spaces-and-tabs (spaces: line 65, tab: line 1) > 1 packages and 1 specfiles checked; 0 errors, 5 warnings. > Fixed > Looking deeper to the spec-file: > > - URL-Entry should start with http:// Fixed > - yum provides /sbin/install-info lists info as package providing > /sbin/install-info. If you definitely need it for pre and post-section, take > info. https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRequires Lists > info as package, that doesn't need to be included. Modified based on http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Texinfo > - $RPM_BUILD_ROOT and %{buildroot}-Macros should not be mixed. > (https://fedoraproject.org/wiki/Packaging:Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS) Fixed > > Mock builds fine. > Ok, looking deeper: > +:ok, =:needs attention, -:needs fixing > > [+] MUST: rpmlint must be run on every package. > (see above) > [+] 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. [FIXME?: covers this > list and more] > [+] MUST: The package must be licensed with a Fedora approved license and meet > the Licensing Guidelines. > [+] MUST: The License field in the package spec file must match the actual > license. > [+] MUST: 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 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, > as provided in the spec URL. > <<md5sum checksum>> > [+] MUST: The package must successfully compile and build into binary rpms on > at least one supported architecture. > [+] MUST: If the package does not successfully compile, build or work on an > architecture, then those architectures should be listed in the spec in > ExcludeArch. > [-] MUST: All build dependencies must be listed in BuildRequires Which dependencies are missing in BuildRequires? > [=] MUST: The spec file MUST handle locales properly. This is done by using the > %find_lang macro. > [*] MUST: Every binary RPM package which stores shared library files (not just > symlinks) in any of the dynamic linker's default paths, must call ldconfig in > %post and %postun. > [*] MUST: If the package is designed to be relocatable, the packager must state > this fact in the request for review > [-] MUST: A package must own all directories that it creates. If it does not > create a directory that it uses, then it should require a package which does > create that directory. How to fix it? I think I already own all directories. > [+] MUST: A package must not contain any duplicate files in the %files listing. > [+] MUST: Permissions on files must be set properly. Executables should be set > with executable permissions, for example. Every %files section must include a > %defattr(...) line. > [+] MUST: Each package must have a %clean section, which contains rm -rf > %{buildroot} (or $RPM_BUILD_ROOT). > [-] MUST: Each package must consistently use macros, as described in the macros Fixed > section of Packaging Guidelines. > [+] MUST: The package must contain code, or permissible content. This is > described in detail in the code vs. content section of Packaging Guidelines. > [+] MUST: Large documentation files should go in a doc subpackage. > [+] MUST: If a package includes something as %doc, it 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: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' > (for directory ownership and usability). > [+] 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: In the vast majority of cases, devel packages must require the base > package using a fully versioned dependency: Requires: %{name} = > %{version}-%{release} > [+] MUST: Packages must NOT contain any .la libtool archives, these should be > removed in the spec. > [+] MUST: Packages containing GUI applications must include a %{name}.desktop > file, and that file must be properly installed with desktop-file-install in the > %install section. > [+] MUST: Packages must not own files or directories already owned by other > packages. > [+] MUST: At the beginning of %install, each package MUST run rm -rf > %{buildroot} (or $RPM_BUILD_ROOT). > [+] MUST: All filenames in rpm packages must be valid UTF-8.
fix for some rpmlint warnings SPEC: http://dl.dropbox.com/u/1338197/1/polipo.spec SRPM: http://dl.dropbox.com/u/1338197/1/polipo-1.0.4.1-2.fc12.src.rpm
(In reply to comment #3) > Which dependencies are missing in BuildRequires? Your requirements list info as requirement. You don't need it. > > > [=] MUST: The spec file MUST handle locales properly. This is done by using the > > [-] MUST: A package must own all directories that it creates. If it does not > > create a directory that it uses, then it should require a package which does > > create that directory. > > How to fix it? I think I already own all directories. > Your spec creates some directories via mkdir. are they owned by your package as well? How do I test your package? (How do I check, if the binary works correct?)
(In reply to comment #5) > (In reply to comment #3) > > > [-] MUST: A package must own all directories that it creates. If it does not > > > create a directory that it uses, then it should require a package which does > > > create that directory. > > > > How to fix it? I think I already own all directories. > > > Your spec creates some directories via mkdir. are they owned by your package as > well? There is probably a misunderstanding. I did not look at the final RPMs, so I did not check whether all directories are owned. But the guideline is targeted at the directories that are created when the RPM is installed, e.g. if the SPEC contains "mkdir %{_bindir}", this does not count as a created directory, because this directory is already included in the filesystem RPM. Btw. there are two trailing tab chars after the "Requires: logrotate", that can be removed, but there is no need to do this immediately.
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #3) > > > > > [-] MUST: A package must own all directories that it creates. If it does not > > > > create a directory that it uses, then it should require a package which does > > > > create that directory. > > > > > > How to fix it? I think I already own all directories. > > > > > Your spec creates some directories via mkdir. are they owned by your package as > > well? > > There is probably a misunderstanding. I did not look at the final RPMs, so I > did not check whether all directories are owned. But the guideline is targeted > at the directories that are created when the RPM is installed, e.g. if the SPEC > contains "mkdir %{_bindir}", this does not count as a created directory, > because this directory is already included in the filesystem RPM. > > Btw. there are two trailing tab chars after the "Requires: logrotate", that can > be removed, but there is no need to do this immediately. Thanks!
(In reply to comment #5) > (In reply to comment #3) > > > Which dependencies are missing in BuildRequires? > > Your requirements list info as requirement. You don't need it. > > > > > > [=] MUST: The spec file MUST handle locales properly. This is done by using the > > > > [-] MUST: A package must own all directories that it creates. If it does not > > > create a directory that it uses, then it should require a package which does > > > create that directory. > > > > How to fix it? I think I already own all directories. > > > Your spec creates some directories via mkdir. are they owned by your package as > well? > > How do I test your package? (How do I check, if the binary works correct?) rpmbuild --rebuild http://dl.dropbox.com/u/1338197/1/polipo-1.0.4.1-2.fc12.src.rpm Please test the initscript and RPM post scripts to see if them work fine on your PC.
> rpmbuild --rebuild > http://dl.dropbox.com/u/1338197/1/polipo-1.0.4.1-2.fc12.src.rpm > > Please test the initscript and RPM post scripts to see if them work fine on > your PC. The reviewer should test the software, not only the build and installation process to verify the software works correct (or at least as designed). Did you inform upstream? There seem to exist more than one fedora-package for polipo, e.g. http://devil.homelinux.org/Tor_related/polipo/
(In reply to comment #9) > > rpmbuild --rebuild > > http://dl.dropbox.com/u/1338197/1/polipo-1.0.4.1-2.fc12.src.rpm > > > > Please test the initscript and RPM post scripts to see if them work fine on > > your PC. > The reviewer should test the software, not only the build and installation > process to verify the software works correct (or at least as designed). > > Did you inform upstream? There seem to exist more than one fedora-package for > polipo, e.g. http://devil.homelinux.org/Tor_related/polipo/ The software is only one file, so the verification of polipo itself is easy to do. Upstream will be informed until polipo is pushed to the repo of fedora. The polipo rpms provided by some third repos didn't meet fedora packaging guidelines.
And how do I verify? Is there a test(-suite)?
service polipo start service polipo stop service polipo status service polipo restart You can test if the local webserver (http://localhost:8123/) works fine after you start polipo.
koji build:http://koji.fedoraproject.org/koji/taskinfo?taskID=2010976 SPEC: http://dl.dropbox.com/u/1338197/1/polipo.spec SRPM: http://dl.dropbox.com/u/1338197/1/polipo-1.0.4.1-2.fc12.src.rpm
wget http://freehaven.net/~chrisd/polipo/polipo-1.0.4.1.tar.gz $ md5sum polipo-1.0.4.1.tar.gz SOURCES/polipo-1.0.4.1.tar.gz bfc5c85289519658280e093a270d6703 polipo-1.0.4.1.tar.gz 92bd22986fe72ccc8a12b518d365e7b6 SOURCES/polipo-1.0.4.1.tar.gz MD5SUM mismatch
(In reply to comment #14) > wget http://freehaven.net/~chrisd/polipo/polipo-1.0.4.1.tar.gz > $ md5sum polipo-1.0.4.1.tar.gz SOURCES/polipo-1.0.4.1.tar.gz > bfc5c85289519658280e093a270d6703 polipo-1.0.4.1.tar.gz > 92bd22986fe72ccc8a12b518d365e7b6 SOURCES/polipo-1.0.4.1.tar.gz > MD5SUM mismatch It's a weird thing, upstream changes the compression ratio of polipo-1.0.4.1.tar.gz. bfc5c85289519658280e093a270d6703 polipo-1.0.4.1.tar.gz 175K 92bd22986fe72ccc8a12b518d365e7b6 SOURCES/polipo-1.0.4.1.tar.gz 880K
$ rpmlint polipo-*.rpm polipo.x86_64: W: conffile-without-noreplace-flag /var/log/polipo Ghosted, not an issue polipo.x86_64: W: non-standard-uid /var/run/polipo polipo polipo.x86_64: W: non-standard-gid /var/run/polipo polipo polipo.x86_64: W: non-standard-uid /var/cache/polipo polipo polipo.x86_64: W: non-standard-gid /var/cache/polipo polipo Fine, UID+GID are created in pre-install. /var/cache is consistent with FSH polipo.x86_64: E: non-standard-dir-perm /var/cache/polipo 0750 I would normally expect that to be 0700 or owned by root polipo.x86_64: W: dangerous-command-in-%pre chown polipo.x86_64: W: dangerous-command-in-%post chmod This looks fine polipo.x86_64: W: missing-lsb-keyword Default-Stop in /etc/rc.d/init.d/polipo 3 packages and 0 specfiles checked; 1 errors, 8 warnings. LSB 3.2 no longer mandates this
The following is based off your spec + the tarball from upstream - = N/A x = Check ! = Problem ? = Not evaluated === REQUIRED ITEMS === [x] Package is named according to the Package Naming Guidelines. [x] Spec file name must match the base package %{name}, in the format %{name}.spec. [x] Package meets the Packaging Guidelines including the Perl specific items [x] Package successfully compiles and builds into binary rpms on at least one supported architecture. Tested on: http://koji.fedoraproject.org/koji/taskinfo?taskID=2061039 [x] Rpmlint output: <see previous comments> [x] Package is not relocatable. [x] Buildroot is correct Buildroot: %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX) [x] Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x] License field in the package spec file matches the actual license. License type: MIT [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] Spec file is legible and written in American English. [!] Sources used to build the package matches the upstream source, as provided in the spec URL. <See comments # 14 & #15> [x] Package is not known to require ExcludeArch [x] All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [-] The spec file handles locales properly. [-] ldconfig called in %post and %postun if required. [x] Package must own all directories that it creates. [x] Package requires other packages for directories it uses. /etc/logrotate.d isn't, however, this is consistent with other packages [x] Package does not contain duplicates in %files. [!] Permissions on files are set properly. <see comment #16 on /var/cache/polipo> - not a blocker [x] Package has a %clean section, which contains rm -fR $RPM_BUILD_ROOT. [x] Package consistently uses macros. [x] Package contains code, or permissable content. [-] Large documentation files are in a -doc subpackage, if required. [x] Package uses nothing in %doc for runtime. [-] Header files in -devel subpackage, if present. [-] Static libraries in -devel subpackage, if present. [-] Package requires pkgconfig, if .pc files are present. [-] Development .so files in -devel subpackage, if present. [-] Fully versioned dependency in subpackages, if present. [x] Package does not contain any libtool archives (.la). [-] Package contains a properly installed %{name}.desktop file if it is a GUI application. [x] Package does not own files or directories owned by other packages. === SUGGESTED ITEMS === [x] Latest version is packaged. [x] Package does not include license text files separate from upstream. [-] Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [x] Reviewer should test that the package builds in mock. Tested on: http://koji.fedoraproject.org/koji/taskinfo?taskID=2061039 [x] Package should compile and build into binary rpms on all supported architectures. Tested on: fedora-rawhide-x86_64 [x] Scriptlets must be sane, if used. [-] The placement of pkgconfig(.pc) files is correct. [x] File based requires are sane. [-] %check is present and the tests pass [x] Package functions as described. Basic functionality works, doesn't start on boot by default. polipo stops and starts fine export http_proxy=http://localhost:8123/ wget then works ===== * I'll leave the permissions on /var/cache/polipo up to you, they don't seem to be a security risk. * Use the new upstream tarball, and I'll approve
(In reply to comment #16) > $ rpmlint polipo-*.rpm > polipo.x86_64: W: conffile-without-noreplace-flag /var/log/polipo > > Ghosted, not an issue I am not sure, whether this is correct. /var/log/polipo is probably a logfile, therefore it is imho wrong to use %conf for it.
Till is somewhat more experienced than I, so please update this prior to approval.
(In reply to comment #18) > (In reply to comment #16) > > $ rpmlint polipo-*.rpm > > polipo.x86_64: W: conffile-without-noreplace-flag /var/log/polipo > > > > Ghosted, not an issue > > I am not sure, whether this is correct. /var/log/polipo is probably a logfile, > therefore it is imho wrong to use %conf for it. /var/log/polipo is a logfile, so I follow https://fedoraproject.org/wiki/PackagingDrafts/Logfiles to add %ghost %config to the logfile, I'm not suse if it's the right pattern to package a logfile. (In reply to comment #16) > Fine, UID+GID are created in pre-install. /var/cache is consistent with FSH > > polipo.x86_64: E: non-standard-dir-perm /var/cache/polipo 0750 > > I would normally expect that to be 0700 or owned by root > Refer to the squid rpm, /var/cache/polipo should be owned by polipo and have a non-standard-dir-perm. See http://koji.fedoraproject.org/koji/fileinfo?rpmID=1871194&filename=/var/spool/squid New SRPM: http://dl.dropbox.com/u/1338197/1/polipo-1.0.4.1-2.fc12.src.rpm
> /var/log/polipo is a logfile, so I follow > https://fedoraproject.org/wiki/PackagingDrafts/Logfiles to add %ghost %config > to the logfile, I'm not suse if it's the right pattern to package a logfile. That's reasonable and I'm happy with that. Note: that's only a draft. There's been some discussion on IRC, and it basically comes down to should a package 'own' it's logs. FPC has yet to make a ruling to at this stage you comply with the guidelines. I personally prefer the RPM owning like this. > > polipo.x86_64: E: non-standard-dir-perm /var/cache/polipo 0750 > > > > I would normally expect that to be 0700 or owned by root > > > Refer to the squid rpm, /var/cache/polipo should be owned by polipo and have a > non-standard-dir-perm. Various packages have done various different things, that was not something I considered a blocker, simply something worth considering. I'm happy with your logic. MD5 Sums now match. In future, even for the small updates, please up the release number when you up load. APPROVED
(In reply to comment #21) > > /var/log/polipo is a logfile, so I follow > > https://fedoraproject.org/wiki/PackagingDrafts/Logfiles to add %ghost %config > > to the logfile, I'm not suse if it's the right pattern to package a logfile. > > That's reasonable and I'm happy with that. Note: that's only a draft. > > There's been some discussion on IRC, and it basically comes down to should a > package 'own' it's logs. FPC has yet to make a ruling to at this stage you > comply with the guidelines. I personally prefer the RPM owning like this. > > > > polipo.x86_64: E: non-standard-dir-perm /var/cache/polipo 0750 > > > > > > I would normally expect that to be 0700 or owned by root > > > > > Refer to the squid rpm, /var/cache/polipo should be owned by polipo and have a > > non-standard-dir-perm. > > Various packages have done various different things, that was not something I > considered a blocker, simply something worth considering. I'm happy with your > logic. > > MD5 Sums now match. > > In future, even for the small updates, please up the release number when you up > load. > > APPROVED Thankee, Mark!
New Package CVS Request ======================= Package Name: polipo Short Description: Polipo is a small and fast caching web proxy. Owners: supercyper Branches: F-13 F-12 F-11 EL-5 InitialCC:
CVS done (by process-cvs-requests.py).
Package Change Request ====================== Package Name: polipo New Branches: epel7 Owners: bjohnson
Git done (by process-git-requests).