This service will be undergoing maintenance at 00:00 UTC, 2017-10-23 It is expected to last about 30 minutes
Bug 991221 - (numatop) Review Request: numatop - Memory access locality characterization and analysis
Review Request: numatop - Memory access locality characterization and analysis
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Zbigniew Jędrzejewski-Szmek
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 1496867
  Show dependency treegraph
 
Reported: 2013-08-01 17:38 EDT by Dridi Boukelmoune
Modified: 2017-09-29 02:51 EDT (History)
9 users (show)

See Also:
Fixed In Version: numatop-1.0.1-5.fc20
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-10-22 23:27:34 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
zbyszek: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Dridi Boukelmoune 2013-08-01 17:38:49 EDT
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
Comment 1 Zbigniew Jędrzejewski-Szmek 2013-08-02 14:06:50 EDT
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
Comment 2 Zbigniew Jędrzejewski-Szmek 2013-08-02 14:12:05 EDT
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
Comment 3 Dridi Boukelmoune 2013-08-02 16:37:26 EDT
(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
Comment 4 Zbigniew Jędrzejewski-Szmek 2013-08-02 16:41:30 EDT
(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.
Comment 5 Michael Schwendt 2013-08-05 12:42:24 EDT
> 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.
Comment 6 Zbigniew Jędrzejewski-Szmek 2013-08-05 15:19:38 EDT
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.
Comment 7 Dridi Boukelmoune 2013-08-23 04:16:52 EDT
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.
Comment 9 Zbigniew Jędrzejewski-Szmek 2013-08-28 10:34:36 EDT
I'll review it.
Comment 10 Zbigniew Jędrzejewski-Szmek 2013-08-28 12:57:01 EDT
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.
Comment 11 Dridi Boukelmoune 2013-09-06 07:09:10 EDT
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.
Comment 12 Peter Robinson 2013-09-06 08:19:32 EDT
(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
Comment 13 Dridi Boukelmoune 2013-09-06 13:35:04 EDT
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
///
Comment 14 Dridi Boukelmoune 2013-09-10 18:09:34 EDT
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.
Comment 15 Dan Horák 2013-09-11 02:18:45 EDT
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.
Comment 16 Dan Horák 2013-09-11 05:11:45 EDT
Florian's answer for the record = https://lists.fedoraproject.org/pipermail/devel/2013-September/189001.html
Comment 17 Zbigniew Jędrzejewski-Szmek 2013-09-12 17:48:11 EDT
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!
Comment 18 Peter Robinson 2013-09-13 02:02:56 EDT
> 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
Comment 19 Dridi Boukelmoune 2013-09-14 16:00:47 EDT
(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
Comment 20 Zbigniew Jędrzejewski-Szmek 2013-09-14 19:10:04 EDT
(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.
Comment 21 Dridi Boukelmoune 2013-09-15 06:10:02 EDT
New Package SCM Request
=======================
Package Name: numatop
Short Description: Memory access locality characterization and analysis
Owners: dridi
Branches: f18 f19 f20
InitialCC:
Comment 22 Gwyn Ciesla 2013-09-16 08:44:52 EDT
Git done (by process-git-requests).
Comment 23 Fedora Update System 2013-09-17 07:38:54 EDT
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
Comment 24 Fedora Update System 2013-09-17 07:40:39 EDT
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
Comment 25 Fedora Update System 2013-09-17 07:42:07 EDT
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
Comment 26 Fedora Update System 2013-09-17 14:15:28 EDT
numatop-1.0.1-4.fc20 has been pushed to the Fedora 20 testing repository.
Comment 27 Fedora Update System 2013-10-12 10:48:57 EDT
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
Comment 28 Fedora Update System 2013-10-12 10:56:08 EDT
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
Comment 29 Fedora Update System 2013-10-12 11:02:40 EDT
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
Comment 30 Peter Robinson 2013-10-16 18:55:13 EDT
Any chance of getting this for EL-6 too?
Comment 31 Dridi Boukelmoune 2013-10-17 02:27:46 EDT
There is, but I need to read the epel guidelines first.
Comment 32 Fedora Update System 2013-10-22 23:27:34 EDT
numatop-1.0.1-5.fc18 has been pushed to the Fedora 18 stable repository.
Comment 33 Fedora Update System 2013-10-22 23:27:54 EDT
numatop-1.0.1-5.fc19 has been pushed to the Fedora 19 stable repository.
Comment 34 Fedora Update System 2013-11-10 02:13:37 EST
numatop-1.0.1-5.fc20 has been pushed to the Fedora 20 stable repository.
Comment 35 Dridi Boukelmoune 2013-11-25 10:47:58 EST
@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
Comment 36 Peter Robinson 2013-11-26 18:40:19 EST
(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.

Note You need to log in before you can comment on or make changes to this bug.