https://bugzilla.redhat.com/show_bug.cgi?id=968339
Bug ID: 968339 Summary: Review Request: ps_mem - Memory profiling tool Product: Fedora Version: rawhide Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: jcapik@redhat.com QA Contact: extras-qa@fedoraproject.org CC: notting@redhat.com, package-review@lists.fedoraproject.org
Spec URL: http://jcapik.fedorapeople.org/files/ps_mem/ps_mem.spec SRPM URL: http://jcapik.fedorapeople.org/files/ps_mem/ps_mem-3.1-1.fc20.src.rpm Description: Memory profiling tool Fedora Account System Username: jcapik
https://bugzilla.redhat.com/show_bug.cgi?id=968339
--- Comment #1 from Greg Bailey gbailey@lxpro.com --- I've used this script for a long time and would love to see it packaged. I can probably review this later today or tomorrow. Are you open to the idea of packaging this for EPEL 5 and/or EPEL 6 as well?
https://bugzilla.redhat.com/show_bug.cgi?id=968339
--- Comment #2 from Jaromír Cápík jcapik@redhat.com --- Hi Greg.
It would be nice if you could do the review. Yes, I'm open to that idea.
Thank you.
Regards, Jaromir.
https://bugzilla.redhat.com/show_bug.cgi?id=968339
Greg Bailey gbailey@lxpro.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Assignee|nobody@fedoraproject.org |gbailey@lxpro.com
--- Comment #3 from Greg Bailey gbailey@lxpro.com --- Hi Jaromir,
A couple of quick notes before doing the formal review:
Regarding github references: http://fedoraproject.org/wiki/Packaging:SourceURL#Github
I'd suggest defining the git commit as a %global instead of hardcoding it in the Source0 URL, so you'd have:
%global commit 961ff24c805a474080520403409872b04e18f4d9
Source0: https://raw.github.com/pixelb/scripts/%%7Bcommit%7D/scripts/ps_mem.py
The "ps_mem.py" script defines its own short description:
line 175: help_msg = 'ps_mem.py - Show process memory usage\n'\
So, you may consider changing your summary line to:
Summary: Show process memory usage
but your summary of "Memory profiling tool" is OK too if that's what you prefer.
You should add an empty "%build" section; the fedora-review tool seems to fail without it.
It's a good idea to preserve timestamps when copying the files around. http://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps
I'd use: cp -p %{SOURCE0} %{name} cp -p %{SOURCE1} LICENSE
-and-
install -D -p -m755 %{name} %{buildroot}%{_bindir}/%{name}
(You might want to re-retrieve your sources using the suggested tools in the above URL; your source RPM includes ps_mem.py with a timestamp in 2011, which is too old for the newest version 3.1. It should be approx. May 10, 2013)
For building against EPEL 5, could you add the following? (see: fedoraproject.org/wiki/EPEL:Packaging )
BuildRoot: %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
%install rm -rf %{buildroot} install -D -p -m755 %{name} %{buildroot}%{_bindir}/%{name}
-and-
%clean rm -rf %{buildroot}
If you'd prefer me to just attach my spec file, let me know.
https://bugzilla.redhat.com/show_bug.cgi?id=968339
--- Comment #4 from Jaromír Cápík jcapik@redhat.com --- Hi Greg.
Few reactions to your notes:
Defining the commit hash in form of global is unfortunately more work for the mainainer, since it is easier to simply copy'n'paste the whole link when browsing the scripts in the raw mode. As any additional line makes the spec file longer and less synoptic, I would try to avoid globals in this case.
The mentioned description "Show process memory usage" unfortunately goes contrary to the main behavioral difference against other ps tools. The purpose of this script is to provide users with memory usage per application (not per process) and that's why I see this as an inconsistence even when it is printed directly by the script.
According to the following bug, the fedora-review issue has been fixed. Please update to the fixed package and recheck. https://bugzilla.redhat.com/show_bug.cgi?id=967571
Regarding timestamps. It's good to preserve timestamps of the files stored in the upstream archive, but in this case the file was downloaded directly without consequent unpacking and the timestamp was invalidated during the download phase even when I tried to use the wget -N / curl -R options. I rechecked that few minutes ago and I'm always getting today's date, so it seems the server doesn't offer this capability. The previous timestamp was incorrect due to a strange clock skew in my virtual machine. There's no reason for preserving invalid timestamps. Anyway, I see this issue as minor and don't want it to block the script introduction.
Regarding the EPEL stuff. At the moment I'm waiting for PM to decide if the script is worthy for being included in the official branches and therefore I don't want to introduce this package with explicit EPEL5/6 artifacts which make the spec file more dirty for non-EPEL Fedora branches.
If you have no objections, please, do the review as if it was a non-EPEL package. I'll add the EPEL stuff later if it is needed, since that would make the merging between non-EPEL and EPEL branches easier.
Thanks and regards, Jaromir.
https://bugzilla.redhat.com/show_bug.cgi?id=968339
Greg Bailey gbailey@lxpro.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |fedora-review?
--- Comment #5 from Greg Bailey gbailey@lxpro.com --- (In reply to Jaromír Cápík from comment #4)
Defining the commit hash in form of global is unfortunately more work for the mainainer, since it is easier to simply copy'n'paste the whole link when browsing the scripts in the raw mode. As any additional line makes the spec file longer and less synoptic, I would try to avoid globals in this case.
OK, style difference then...
According to the following bug, the fedora-review issue has been fixed. Please update to the fixed package and recheck. https://bugzilla.redhat.com/show_bug.cgi?id=967571
The new "fedora-review" RPM is not yet available in the testing repository; it'd be quicker for you to just add the empty '%build' tag.
Regarding timestamps. It's good to preserve timestamps of the files stored in the upstream archive, but in this case the file was downloaded directly without consequent unpacking and the timestamp was invalidated during the download phase even when I tried to use the wget -N / curl -R options. I rechecked that few minutes ago and I'm always getting today's date, so it seems the server doesn't offer this capability. The previous timestamp was incorrect due to a strange clock skew in my virtual machine. There's no reason for preserving invalid timestamps. Anyway, I see this issue as minor and don't want it to block the script introduction.
I think it's a good idea to at least add the "-p" option to the cp and install commands. It's more useful to preserve the timestamp of when you (the packager) retrieved the file(s) from the original URL vs. the timestamp of when the RPM was built.
Regarding the EPEL stuff. At the moment I'm waiting for PM to decide if the script is worthy for being included in the official branches and therefore I don't want to introduce this package with explicit EPEL5/6 artifacts which make the spec file more dirty for non-EPEL Fedora branches.
I'm not sure what "waiting for PM to decide" means. Are you saying that this might be added to the Red Hat Enterprise distribution?
If you're uncomfortable adding the parts required for EPEL 5, you can leave them out for now. I'd ask that you at least create a branch for EPEL 6; the spec file will work as is for that.
https://bugzilla.redhat.com/show_bug.cgi?id=968339
Jaromír Cápík jcapik@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED
--- Comment #6 from Jaromír Cápík jcapik@redhat.com --- Hi Greg.
The new "fedora-review" RPM is not yet available in the testing repository; it'd be quicker for you to just add the empty '%build' tag.
Sorry, I thought you know koji ... http://koji.fedoraproject.org/koji/packageinfo?packageID=12860
The packages can be downloaded from there without waiting for testing. Just select the latest release for your fedora version and download the right package from the RPMs section. Let me know if you encounter any problem. I don't wanna introduce unwanted empty sections just because of the fedora-review bug.
Regarding timestamps. It's good to preserve timestamps of the files stored in the upstream archive, but in this case the file was downloaded directly without consequent unpacking and the timestamp was invalidated during the download phase even when I tried to use the wget -N / curl -R options. I rechecked that few minutes ago and I'm always getting today's date, so it seems the server doesn't offer this capability. The previous timestamp was incorrect due to a strange clock skew in my virtual machine. There's no reason for preserving invalid timestamps. Anyway, I see this issue as minor and don't want it to block the script introduction.
I think it's a good idea to at least add the "-p" option to the cp and install commands. It's more useful to preserve the timestamp of when you (the packager) retrieved the file(s) from the original URL vs. the timestamp of when the RPM was built.
Ok. That's definitely doable. Let me upload new files.
If you're uncomfortable adding the parts required for EPEL 5, you can leave them out for now. I'd ask that you at least create a branch for EPEL 6; the spec file will work as is for that.
Don't worry. I'll create the branches and they'll eventually stay empty once we know the decision.
https://bugzilla.redhat.com/show_bug.cgi?id=968339
--- Comment #7 from Jaromír Cápík jcapik@redhat.com --- Spec URL: http://jcapik.fedorapeople.org/files/ps_mem/2/ps_mem.spec SRPM URL: http://jcapik.fedorapeople.org/files/ps_mem/2/ps_mem-3.1-2.fc20.src.rpm
https://bugzilla.redhat.com/show_bug.cgi?id=968339
--- Comment #8 from Greg Bailey gbailey@lxpro.com --- (In reply to Jaromír Cápík from comment #6)
Hi Greg.
The new "fedora-review" RPM is not yet available in the testing repository; it'd be quicker for you to just add the empty '%build' tag.
Sorry, I thought you know koji ... http://koji.fedoraproject.org/koji/packageinfo?packageID=12860
The packages can be downloaded from there without waiting for testing. Just select the latest release for your fedora version and download the right package from the RPMs section. Let me know if you encounter any problem. I don't wanna introduce unwanted empty sections just because of the fedora-review bug.
I know koji...
I would have included an explicit empty %build section anyway (regardless of the fedora-review bug), but I see you prefer to not include an empty section. I pulled the newer fedora-review RPM.
https://bugzilla.redhat.com/show_bug.cgi?id=968339
Greg Bailey gbailey@lxpro.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? | Flags| |fedora-review+
--- Comment #9 from Greg Bailey gbailey@lxpro.com --- Looks good. The only thing I marked with '!' (under "SHOULD") is:
[!]: Package does not include license text files separate from upstream.
In this case, I think it's acceptable because ps_mem is really part of a larger collection that is outside the scope of this particular package. The script clearly indicates LGPLv2, and the license text file is pulled directly from the FSF.
Approved.
Package Review ==============
Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed
===== MUST items =====
Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [-]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: 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 [x]: Package requires other packages for directories it uses. [-]: Package uses nothing in %doc for runtime. [-]: Package is not known to require ExcludeArch. [x]: Package complies to the Packaging Guidelines [x]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated". 1 files have unknown license. Detailed output of licensecheck in /home/gbailey/968339-ps_mem/licensecheck.txt [x]: Package consistently uses macro is (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [x]: Package must own all directories that it creates. [x]: Package does not own files or directories owned by other packages. [x]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [-]: Large documentation must go in a -doc subpackage. Note: Documentation size is 30720 bytes in 1 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]: 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: There are rpmlint messages (see attachment).
===== 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. [x]: Final provides and requires are sane (see attachments). [x]: Package functions as described. [x]: Latest version is packaged. [!]: 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. [?]: Package should compile and build into binary rpms on all supported architectures. [-]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [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]: 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: There are rpmlint messages (see attachment). [x]: Spec file according to URL is the same as in SRPM.
Rpmlint ------- Checking: ps_mem-3.1-2.fc18.noarch.rpm ps_mem.noarch: W: spelling-error %description -l en_US ps -> PS, pa, pd ps_mem.noarch: W: spelling-error %description -l en_US mem -> men, me, mm ps_mem.noarch: W: no-manual-page-for-binary ps_mem 1 packages and 0 specfiles checked; 0 errors, 3 warnings.
Rpmlint (installed packages) ---------------------------- # rpmlint ps_mem ps_mem.noarch: W: spelling-error %description -l en_US ps -> PS, pa, pd ps_mem.noarch: W: spelling-error %description -l en_US mem -> men, me, mm ps_mem.noarch: W: no-manual-page-for-binary ps_mem 1 packages and 0 specfiles checked; 0 errors, 3 warnings. # echo 'rpmlint-done:'
Requires -------- ps_mem (rpmlib, GLIBC filtered): /usr/bin/env python2
Provides -------- ps_mem: ps_mem
Source checksums ---------------- https://raw.github.com/pixelb/scripts/961ff24c805a474080520403409872b04e18f4... : CHECKSUM(SHA256) this package : 4856fbd39b9f2f7d2532e467b2f822c164bd2b0cf2a6275c46d0a46ce38405d6 CHECKSUM(SHA256) upstream package : 4856fbd39b9f2f7d2532e467b2f822c164bd2b0cf2a6275c46d0a46ce38405d6 http://www.gnu.org/licenses/lgpl-2.1.txt : CHECKSUM(SHA256) this package : dc626520dcd53a22f727af3ee42c770e56c97a64fe3adb063799d8ab032fe551 CHECKSUM(SHA256) upstream package : dc626520dcd53a22f727af3ee42c770e56c97a64fe3adb063799d8ab032fe551
Generated by fedora-review 0.4.1 (b2e211f) last change: 2013-04-29 Buildroot used: fedora-18-x86_64 Command line :/usr/bin/fedora-review -b 968339
https://bugzilla.redhat.com/show_bug.cgi?id=968339
Jaromír Cápík jcapik@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |fedora-cvs?
--- Comment #10 from Jaromír Cápík jcapik@redhat.com --- New Package SCM Request ======================= Package Name: ps_mem Short Description: Memory profiling tool Owners: jcapik Branches: f19 el6 el5 InitialCC:
https://bugzilla.redhat.com/show_bug.cgi?id=968339
Jon Ciesla limburgher@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-cvs? | Flags| |fedora-cvs+
https://bugzilla.redhat.com/show_bug.cgi?id=968339
--- Comment #11 from Jon Ciesla limburgher@gmail.com --- Git done (by process-git-requests).
https://bugzilla.redhat.com/show_bug.cgi?id=968339
Jaromír Cápík jcapik@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution|--- |RAWHIDE Last Closed| |2013-05-31 16:13:17
--- Comment #12 from Jaromír Cápík jcapik@redhat.com --- Thanks guys. The package has been imported and built for rawhide. Closing,
https://bugzilla.redhat.com/show_bug.cgi?id=968339
Pádraig Brady pbrady@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |pbrady@redhat.com
--- Comment #13 from Pádraig Brady pbrady@redhat.com --- Note ps_mem.py is an importable module too. So one might support that in future (perhaps as a python-ps_mem subpackage).
package-review@lists.fedoraproject.org