Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=495579 --- Comment #7 from Robert Scheck <redhat-bugzilla@xxxxxxxxxxxx> 2009-04-13 19:21:15 EDT --- 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... ;-) -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review