Spec URL: https://bitbucket.org/dridi/fedora_packages/downloads/numatop.spec SRPM URL: https://bitbucket.org/dridi/fedora_packages/downloads/numatop-1.0.1-1.fc19.src.rpm Description: NumaTOP is an observation tool for runtime memory locality characterization and analysis of processes and threads running on a NUMA system. It helps the user characterize the NUMA behavior of processes and threads and identify where the NUMA-related performance bottlenecks reside. Fedora Account System Username: dridi Note that I've temporarily put "Intel" in the license tag, because I couldn't identify the license. It looks homemade: https://github.com/01org/numatop/blob/ab7e75d51f92f40541ab4297cd64fb8e5ba9fafa/COPYING
Informal review (I'm not a packager): Looks like a useful tool! The license is "BSD" in Fedora [1] (aka "New BSD License", "BSD-new", etc.). The package is correct and follows all packaging guidelines, afaict. Builds fine [2]. [1] https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#Software_License_List [2] http://koji.fedoraproject.org/koji/taskinfo?taskID=5693645
Forgot to mention rpmlint status: W: invalid-license Intel → should be fixed when the license is corrected to BSD W: spelling-error %description -l en_US runtime → runtime is OK, imho
(In reply to Zbigniew Jędrzejewski-Szmek from comment #1) > Informal review (I'm not a packager): > > Looks like a useful tool! > > The license is "BSD" in Fedora [1] (aka "New BSD License", "BSD-new", etc.). I'm used to the FreeBSD license (mainly because I don't have any trademark to protect...). I saw "Intel" in the middle of the file and didn't think further... Thanks for review. > The package is correct and follows all packaging guidelines, afaict. > Builds fine [2]. > I was wondering if the spec should explicitely exclude non-intel architectures: https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Architecture_Support > [1] > https://fedoraproject.org/wiki/Licensing: > Main?rd=Licensing#Software_License_List > [2] http://koji.fedoraproject.org/koji/taskinfo?taskID=5693645 Spec URL: https://bitbucket.org/dridi/fedora_packages/downloads/numatop.spec SRPM URL: https://bitbucket.org/dridi/fedora_packages/downloads/numatop-1.0.1-2.fc19.src.rpm
(In reply to Dridi Boukelmoune from comment #3) > I was wondering if the spec should explicitely exclude non-intel > architectures: > https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/ > Guidelines#Architecture_Support There's no "intel" architecture in this sense, just "x86_64". I think this case is like installing lm-sensors even if you don't have any sensors -- the program will not give useful results, but is otherwise OK to install it.
> The package is correct and follows all packaging guidelines, afaict. Not yet. > Requires: numactl ncurses https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires > + make -j5 > gcc -g -Wall -O2 -o cmd.o -c common/cmd.c > gcc -g -Wall -O2 -o disp.o -c common/disp.c > … https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags > %{_mandir}/man8/%{name}.8.gz Not in the guidelines, but you'll meet reviewers who recommend %{_mandir}/man8/%{name}.8* so the compression applied by rpmbuild on-the-fly could be customised/changed/disabled without breaking a (re)build.
In the description: you could explicitly mention that it supports Intel processors only. From the manpage "numatop supports the Intel Xeon processors: 5500-series, 6500/7500-series, 5600 series, E7-x8xx-series, and E5-16xx/24xx/26xx/46xx-series". Would be best to find a way to condense this list into something shorter, maybe "Intel Xeon processors". This way people reading the description will immediately know when their hardware certainly isn't supported without going into too much detail.
Hi everyone, Thank you for the review and sorry for the long silence. I have been sponsored for another package review, so I don't know whether this package still needs the FE-NEEDSPONSOR dep. Anyway, I'll take care of this this weekend.
Done. Spec URL: https://bitbucket.org/dridi/fedora_packages/downloads/numatop.spec SRPM URL: https://bitbucket.org/dridi/fedora_packages/downloads/numatop-1.0.1-3.fc19.src.rpm
I'll review it.
Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated ===== MUST items ===== C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs. 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. [x]: %build honors applicable compiler flags or justifies otherwise. [x]: 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. [x]: Package uses nothing in %doc for runtime. [x]: 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: "BSD (3 clause)", "Unknown or generated". 1 files have unknown license. Detailed output of licensecheck in /home/zbyszek/fedora/991221-numatop/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. [x]: Useful -debuginfo package or justification otherwise. [-]: Large documentation must go in a -doc subpackage. Note: Documentation size is 10240 bytes in 3 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. Just one: see below. [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. [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. [!]: Package should compile and build into binary rpms on all supported architectures. See below. [-]: %check is present and all tests pass. There's no %check, but the package is a hardware specific interactive program, so it'd be hard to provide one. [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]: 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: There are rpmlint messages (see attachment). [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: numatop-1.0.1-3.fc19.x86_64.rpm numatop.x86_64: W: spelling-error %description -l en_US runtime -> run time, run-time, rudiment 1 packages and 0 specfiles checked; 0 errors, 1 warnings. Bogus. Rpmlint (installed packages) ---------------------------- # rpmlint numatop numatop.x86_64: W: spelling-error %description -l en_US runtime -> run time, run-time, rudiment 1 packages and 0 specfiles checked; 0 errors, 1 warnings. # echo 'rpmlint-done:' Likewise. Requires -------- numatop (rpmlib, GLIBC filtered): libc.so.6()(64bit) libncurses.so.5()(64bit) libnuma.so.1()(64bit) libnuma.so.1(libnuma_1.2)(64bit) libpthread.so.0()(64bit) libtinfo.so.5()(64bit) rtld(GNU_HASH) Provides -------- numatop: numatop numatop(x86-64) Source checksums ---------------- https://01.org/sites/default/files/downloads/numatop_linux_1.0.1.tar.gz : CHECKSUM(SHA256) this package : a3aeb2573669fb25482886bcd370ac182a81042ad721298eb688d149b8d21216 CHECKSUM(SHA256) upstream package : a3aeb2573669fb25482886bcd370ac182a81042ad721298eb688d149b8d21216 ============================================================= Package doesn't build on i686: http://koji.fedoraproject.org/koji/taskinfo?taskID=5865676 http://koji.fedoraproject.org/koji/taskinfo?taskID=5865679 http://koji.fedoraproject.org/koji/taskinfo?taskID=5865683 Looking at the error messages, it's just not ported to i686. I think failing a bug upstream and adding ExcludeArch tag is the way to go (https://fedoraproject.org/wiki/Packaging:Guidelines#Architecture_Support). Since the program only works on new Intel processors, not having this package for i686 (and other archs, if it fails to build there too) is probably OK.
Upstream issue: https://github.com/01org/numatop/issues/3 It is apparently supposed to build on i686 too, I'll try a scratch build without the %optflags to see whether this is the cause.
(In reply to Dridi Boukelmoune from comment #11) > Upstream issue: > https://github.com/01org/numatop/issues/3 > > It is apparently supposed to build on i686 too, I'll try a scratch build > without the %optflags to see whether this is the cause. The optflags will not be the problem and if it builds without them you have another problem to fix. They aren't an optional extra
The %optflags seem to break the build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5905869 I'm not trying to ditch the flags, but it looked like the ideal culprit. I'll notify the upstream. /// diff --git a/numatop.spec b/numatop.spec index 6a5959a..48625cc 100644 --- a/numatop.spec +++ b/numatop.spec @@ -25,7 +25,7 @@ NumaTOP supports the Intel Xeon processors. %build -make CFLAGS="%{optflags}" %{?_smp_mflags} +make %{?_smp_mflags} %install ///
I'll need help on this one. I have isolated the -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 single flag that fails the build. But this is beyond my gcc knowledge.
the content of redhat-hardened-cc1 is: *cc1_options: + %{!fpie:%{!fPIE:%{!fpic:%{!fPIC:%{!fno-pic:-fPIE}}}}} and as you can see it plays with PIE and PIC flags. The error is ... common/util.c: In function 'cpu_type_get': common/util.c:322:2: error: inconsistent operand constraints in an 'asm' __asm volatile( ^ ... and it means the hand-written assembly code is not compatible with PIC/PIE. The register usage in that code must be fixed to be compatible.
Florian's answer for the record = https://lists.fedoraproject.org/pipermail/devel/2013-September/189001.html
So, upstream fix seems to work [0]. I guess we can proceed to the next issue :) I've also made some small fixes, which you might want to include in the rpm, if upstream doesn't merge them [1]. arm is broken, because numactl-devel is missing. There doesn't seem to be a bugzilla entry for this, just a plain git commit [2]. I think that adding ExcludeArch this time is totally OK. [0] http://koji.fedoraproject.org/koji/taskinfo?taskID=5929745 [1] https://github.com/01org/numatop/pull/4 [2] http://pkgs.fedoraproject.org/cgit/numactl.git/commit/?id=5ed43fff987416 Let's get this done!
> arm is broken, because numactl-devel is missing. There doesn't seem to be a > bugzilla entry for this, just a plain git commit [2]. I think that adding > ExcludeArch this time is totally OK. ARM doesn't currently support NUMA. When support is adding (soon I believe) we can easily query what packages are numa based to enable appropriate support and remove the ExcludeArch so that's fine
(In reply to Zbigniew Jędrzejewski-Szmek from comment #17) > So, upstream fix seems to work [0]. I guess we can proceed to the next issue > :) No it doesn't, but I've sent a pull request with Florian's solution: https://github.com/01org/numatop/pull/ > I've also made some small fixes, which you might want to include in the rpm, > if upstream doesn't merge them [1]. Added ! I've learned the "PRI" stuff thanks to you :) The patch is named `numatop.cpuid.patch' but it really contains your fixes. > arm is broken, because numactl-devel is missing. There doesn't seem to be a > bugzilla entry for this, just a plain git commit [2]. I think that adding > ExcludeArch this time is totally OK. Added ExludeArch for %{arm} > [0] http://koji.fedoraproject.org/koji/taskinfo?taskID=5929745 > [1] https://github.com/01org/numatop/pull/4 > [2] http://pkgs.fedoraproject.org/cgit/numactl.git/commit/?id=5ed43fff987416 > > Let's get this done! Ok then, I've got a new spec for you ;) Spec URL: https://bitbucket.org/dridi/fedora_packages/downloads/numatop.spec SRPM URL: https://bitbucket.org/dridi/fedora_packages/downloads/numatop-1.0.1-4.fc19.src.rpm
(In reply to Dridi Boukelmoune from comment #19) > (In reply to Zbigniew Jędrzejewski-Szmek from comment #17) > > So, upstream fix seems to work [0]. I guess we can proceed to the next issue > > :) > > No it doesn't, but I've sent a pull request with Florian's solution: > https://github.com/01org/numatop/pull/ Ooops, indeed I built the wrong thing on koji. I'll comment on the upstream bug since this doesn't really matter for the fedora package - either solution is OK. > Ok then, I've got a new spec for you ;) > > Spec URL: https://bitbucket.org/dridi/fedora_packages/downloads/numatop.spec > SRPM URL: > https://bitbucket.org/dridi/fedora_packages/downloads/numatop-1.0.1-4.fc19. > src.rpm One *suggestion*: change summary to something more descriptive: htop : Interactive process viewer atop : An advanced interactive monitor to view the load on system and iftop : Command line tool that displays bandwidth usage on an interface numatop : Memory access locality characterization and analysis Maybe take the upstream description and shorten it to fit in 72 characters: "an observation tool for runtime memory locality on a NUMA system" Package is APPROVED.
New Package SCM Request ======================= Package Name: numatop Short Description: Memory access locality characterization and analysis Owners: dridi Branches: f18 f19 f20 InitialCC:
Git done (by process-git-requests).
numatop-1.0.1-4.fc20 has been submitted as an update for Fedora 20. https://admin.fedoraproject.org/updates/numatop-1.0.1-4.fc20
numatop-1.0.1-4.fc19 has been submitted as an update for Fedora 19. https://admin.fedoraproject.org/updates/numatop-1.0.1-4.fc19
numatop-1.0.1-4.fc18 has been submitted as an update for Fedora 18. https://admin.fedoraproject.org/updates/numatop-1.0.1-4.fc18
numatop-1.0.1-4.fc20 has been pushed to the Fedora 20 testing repository.
numatop-1.0.1-5.fc19 has been submitted as an update for Fedora 19. https://admin.fedoraproject.org/updates/numatop-1.0.1-5.fc19
numatop-1.0.1-5.fc18 has been submitted as an update for Fedora 18. https://admin.fedoraproject.org/updates/numatop-1.0.1-5.fc18
numatop-1.0.1-5.fc20 has been submitted as an update for Fedora 20. https://admin.fedoraproject.org/updates/numatop-1.0.1-5.fc20
Any chance of getting this for EL-6 too?
There is, but I need to read the epel guidelines first.
numatop-1.0.1-5.fc18 has been pushed to the Fedora 18 stable repository.
numatop-1.0.1-5.fc19 has been pushed to the Fedora 19 stable repository.
numatop-1.0.1-5.fc20 has been pushed to the Fedora 20 stable repository.
@Peter I'm not sure to understand the epel6 guidelines, especially around the release number [1]. I have uploaded a spec [2] that could have worked for both fedora and epel but starting with f20 rpmdev-bumpspec fails to properly bump the release number. I'd like someone to review the epel6 spec and if it's OK, I'll maintain two separate specs. Dridi [1] https://fedoraproject.org/wiki/EPEL:Packaging#Limited_Arch_Packages [2] https://bitbucket.org/dridi/fedora_packages/downloads/numatop-el6.spec
(In reply to Dridi Boukelmoune from comment #35) > @Peter > > I'm not sure to understand the epel6 guidelines, especially around the > release number [1]. I have uploaded a spec [2] that could have worked for [1] only matters if mainline RHEL is shipping it for say x86-64 but not PPC. In this case it's not an issue. In the case of EL-6 builds it builds fine on x86 32/64 but there's issues with PPC (same as on Fedora) but the issue is in the package. I have a partial patch which gets it further along but it's not complete. I can add what I have if someone can help complete it.