Bug 495579

Created attachment 339381 [details]
Build log

rpmlint is complaining:

[fab@laptop24 SRPMS]$ rpmlint ifstatus-1.1.0-1.src.rpm 
ifstatus.src: W: non-standard-group Applications/Utilities
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

See 'less /usr/share/doc/rpm-*/GROUPS' for valid groups.

Isn't 'ncurses' picked up automatically by RPM?

I'm not able to rebuild this package. See attachment.

I'm really sorry but I don't understand the language that the output in the attached file is in. Can I by any chance get that in English?

I have fixed the group:
Spec URL: http://maxamillion.fedorapeople.org/ifstatus.spec
SRPM URL: http://maxamillion.fedorapeople.org/ifstatus-1.1.0-1.src.rpm

Personally I would prefer the group "Applications/Internet", as tcpdump, 
tcpick and iftop are very closed to the functionality (especially iftop)
and they've the group "Applications/Internet".

Package currently does not build in Rawhide:

g++    -c -o BorderDecorator.o BorderDecorator.cc
In file included from BorderDecorator.cc:25:
Main.h:74: warning: 'typedef' was ignored in this declaration
BorderDecorator.cc: In member function 'virtual void BorderDecorator::Draw(Interfaces&, bool)':
BorderDecorator.cc:44: warning: deprecated conversion from string constant to 'char*'
BorderDecorator.cc: In member function 'void BorderDecorator::DrawHelp(Interface&, bool)':
BorderDecorator.cc:90: warning: deprecated conversion from string constant to 'char*'
BorderDecorator.cc:91: warning: deprecated conversion from string constant to 'char*'
BorderDecorator.cc:92: warning: deprecated conversion from string constant to 'char*'
BorderDecorator.cc:93: warning: deprecated conversion from string constant to 'char*'
g++    -c -o Config.o Config.cc
In file included from Config.cc:25:
Main.h:74: warning: 'typedef' was ignored in this declaration
Config.cc: In member function 'void Config::LoadHomePath()':
Config.cc:119: error: 'getenv' was not declared in this scope
Config.cc: In member function 'void Config::LoadDefaultConfig()':
Config.cc:132: error: 'getenv' was not declared in this scope
make: *** [Config.o] Error 1

Created attachment 339394 [details]
ifstatus-1.1.0-gcc44.patch

Created attachment 339395 [details]
ifstatus-1.1.0-fedora.patch

Created attachment 339396 [details]
Diff of original ifstatus.spec with my suggested changes

1. Personally I would prefer the group "Applications/Internet", as tcpdump, 
   tcpick and iftop are very closed to the functionality (especially iftop)
   and they've the group "Applications/Internet" (see comment #6)

2. Building your package in Rawhide fails for several reasons:
   a) Upstream has written not really C++ compliant code and lacks several
      #include lines (see comment #3, #4)
   b) Upstream has a non autotools generated makefile which lacks some needed
      functionality. I added the missing functionality and corrected a few
      things from your patch more etc. (see comment #5)
   c) Please write upstream an e-mail and explain, that these patches (or any
      better) have to make it upstream and should be included into the next
      release of ifstatus. Please set me on Cc: at that e-mail, thank you.

3. "Requires: ncurses" is not needed, because "Requires: libc.so.6()(64bit) 
   libc.so.6(GLIBC_2.2.5)(64bit) libgcc_s.so.1()(64bit) 
   libgcc_s.so.1(GCC_3.0)(64bit) libm.so.6()(64bit) libncurses.so.5()(64bit) 
   libstdc++.so.6()(64bit) libstdc++.so.6(CXXABI_1.3)(64bit)
   libstdc++.so.6(GLIBCXX_3.4)(64bit) libtinfo.so.5()(64bit) rtld(GNU_HASH)"
   from "rpm -qp --requires ifstatus-1.1.0-2.fc10.x86_64.rpm" already lists
   "libncurses.so.5", which makes a runtime dependency to ncurses, that will
   be resolved e.g. by yum (see comment #6)

4. "make CFLAGS=%{?_smp_mflags}" looks strange - CFLAGS and %_smp_mflags are
   two completely different things. CFLAGS are the compiler flags passed to
   the makefile while %_smp_mflags are SMP flags expanded into "-j2" for e.g.
   parallel builds during %build.

   As per my patch from comment #5 the $RPM_OPT_FLAGS are already honored to
   CFLAGS, just using "make %{?_smp_mflags}" is enough (see comment #6)

5. "make install DESTDIR=%{buildroot}%{_bindir}/%{name}" would have worked
   with the original patch, but I think, it's better to less touch upstream
   work and (sorry) my patch is hopefully near to get accepted by upstream.
   Thus replaced by a hopefully better construct (see comment #6)

6. As far as I can see, ifstatus requires either root permissions or the +s
   option (suid bit) to work. For security reasons, it's better, if only root
   can execute the binary in case of a security issue at ifstatus (this is
   how you've packaged it now - fine). And as it doesn't make sense to have
   root-only executable binaries in %{_bindir}, I'm hereby putting ifstatus 
   into %{_sbindir} to satisfy that (see comment #6)

Any objections to my suggestions and explanations so far? Rest looks fine so
far to me, but the formal/official review is still missing. I will do that,
once we've clarified all my points above... ;-)

Oh, I forgot something to 4.: The makefile currently doesn't support
parallel builds, thus "printdone" will show an error which is not true.
I think, we just can ignore that echo, as it is nonsense. If you like,
you can remove the %{?_smp_mflags} in %build and explain, that makefile
is not ready for %{?_smp_mflags} - that's an alternative to the echo.
Which one is up to you, I don't care here...

Spec URL: http://maxamillion.fedorapeople.org/ifstatus.spec
SRPM URL: http://maxamillion.fedorapeople.org/ifstatus-1.1.0-3.src.rpm

I have applied the patches suggested, I also fixed my spec file as well as sent the requested email upstream and have CC'd you on it.

I would like to thank you for the level of detail that you covered in your review thus far, it has been extremely informative and I appreciate the amount of time and effort you put into it.

Looking forward to further review :)

Thanks, I'm trying my best. As it was a tiny package, it didn't consume that
much time anyway. Okay, let's go for the official review.

[  OK  ] MUST: rpmlint must be run on every package
         $ rpmlint /var/lib/mock/fedora-rawhide-x86_64/result/ifstatus-*
         3 packages and 0 specfiles checked; 0 errors, 0 warnings.
         $
[  OK  ] MUST: The package must be named according to the Package Naming 
         Guidelines
[  OK  ] MUST: The spec file name must match the base package %{name} [...]
[  OK  ] MUST: The package must meet the Packaging Guidelines
[  OK  ] MUST: The package must be licensed with a Fedora approved license
         and meet the Licensing Guidelines
[  OK  ] MUST: The License field in the package spec file must match the 
         actual license
[  OK  ] 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
[  OK  ] MUST: The spec file must be written in American English.
[  OK  ] MUST: The spec file for the package MUST be legible.
[  OK  ] MUST: The sources used to build the package must match the upstream 
         source, as provided in the spec URL. Reviewers should use md5sum for 
         this task. If no upstream URL can be specified for this package, 
         please see the Source URL Guidelines for how to deal with this.
         -> f4d413f880754fd6677290160f8bc5d7  ifstatus-v1.1.0.tar.gz
         -> f4d413f880754fd6677290160f8bc5d7  ifstatus-v1.1.0.tar.gz.1
         -> 64dc0d893fe58bfe94174db8717ece23ecc285d9  ifstatus-v1.1.0.tar.gz
         -> 64dc0d893fe58bfe94174db8717ece23ecc285d9  ifstatus-v1.1.0.tar.gz.1
[  OK  ] MUST: The package MUST successfully compile and build into binary 
         rpms on at least one primary architecture
[  N/A ] 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. Each architecture listed in ExcludeArch MUST 
         have a bug filed in bugzilla, describing the reason that the package 
         does not compile/build/work on that architecture. The bug number MUST 
         be placed in a comment, next to the corresponding ExcludeArch line
[  OK  ] MUST: All build dependencies must be listed in BuildRequires, except 
         for any that are listed in the exceptions section of the Packaging 
         Guidelines ; inclusion of those as BuildRequires is optional. Apply 
         common sense.
[  N/A ] MUST: The spec file MUST handle locales properly. This is done by 
         using the %find_lang macro. Using %{_datadir}/locale/* is strictly 
         forbidden
[  N/A ] MUST: Every binary RPM package (or subpackage) which stores shared 
         library files (not just symlinks) in any of the dynamic linker's 
         default paths, must call ldconfig in %post and %postun. [10]
[  N/A ] MUST: If the package is designed to be relocatable, the packager must 
         state this fact in the request for review, along with the 
         rationalization for relocation of that specific package. Without 
         this, use of Prefix: /usr is considered a blocker. [11]
[  OK  ] 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.
[  OK  ] MUST: A package must not contain any duplicate files in the %files 
         listing.
[  OK  ] 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.
[  OK  ] MUST: Each package must have a %clean section, which contains rm -rf
         %{buildroot} (or $RPM_BUILD_ROOT).
[  OK  ] MUST: Each package must consistently use macros.
[  OK  ] MUST: The package must contain code, or permissable content.
[  N/A ] MUST: Large documentation files must go in a -doc subpackage. (The 
         definition of large is left up to the packager's best judgement, but 
         is not restricted to size. Large can refer to either size or 
         quantity).
[  OK  ] MUST: If a package includes something as %doc, it must not affect the 
         runtime of the application. To summarize: If it is in %doc, the 
         program must run properly if it is not present.
[  N/A ] MUST: Header files must be in a -devel package.
[  N/A ] MUST: Static libraries must be in a -static package.
[  N/A ] MUST: Packages containing pkgconfig(.pc) files must 'Requires: 
         pkgconfig' (for directory ownership and usability).
[  N/A ] 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.
[  N/A ] MUST: In the vast majority of cases, devel packages must require the 
         base package using a fully versioned dependency: Requires: %{name} =
         %{version}-%{release}
[  N/A ] MUST: Packages must NOT contain any .la libtool archives, these must 
         be removed in the spec if they are built.
[  N/A ] 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. If you feel that your 
         packaged GUI application does not need a .desktop file, you must put 
         a comment in the spec file with your explanation.
[  OK  ] MUST: Packages must not own files or directories already owned by 
         other packages. The rule of thumb here is that the first package to 
         be installed should own the files or directories that other packages 
         may rely upon. This means, for example, that no package in Fedora 
         should ever share ownership with any of the files or directories 
         owned by the filesystem or man package. If you feel that you have a 
         good reason to own a file or directory that another package owns, 
         then please present that at package review time.
[  OK  ] MUST: At the beginning of %install, each package MUST run rm -rf
         %{buildroot} (or $RPM_BUILD_ROOT).
[  OK  ] MUST: All filenames in rpm packages must be valid UTF-8.

I'm not a native english speaker, but you might want to write "Command line" 
rather "Command Line" in Summary: tag. But that's so minor and not a must, so
it can happen after review and before CVS import if you like.

Nevertheless, the package looks well now, thus: APPROVED.

New Package CVS Request
=======================
Package Name: ifstatus
Short Description: Command line real time interface graphs using ncurses
Owners: maxamillion
Branches: F-10 F-11 EL-4 EL-5
InitialCC:

cvs done.

Ping Adam?

Forgot to tag bodhi with the bug to close it, it has been released into updates. I will close it now, sorry about that.