Bug 424841 - (ndisc6) Review Request: ndisc6 - IPv6 diagnostic tools
Review Request: ndisc6 - IPv6 diagnostic tools
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Jon Ciesla
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-12-14 05:36 EST by Stjepan Gros
Modified: 2010-02-06 16:02 EST (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-02-06 16:02:17 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
limburgher: fedora‑review+
dennis: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Stjepan Gros 2007-12-14 05:36:19 EST
Spec URL: http://www.zemris.fer.hr/~sgros/stuff/fedora/ndisc6/ndisc6.spec
SRPM URL: http://www.zemris.fer.hr/~sgros/stuff/fedora/ndisc6/ndisc6-0.9.3-1.fc8.src.rpm
Description:

This package gathers a few diagnostic tools for IPv6 networks:
- ndisc6, which performs ICMPv6 Neighbor Discovery in userland,
- rdisc6, which performs ICMPv6 Router Discovery in userland,
- rltraceroute6, yet another IPv6 implementation of traceroute,
- tcptraceroute6, a TCP/IPv6-based traceroute implementation,
- tracert6, a ICMPv6 Echo Request based traceroute,
- tcpspray6, a TCP/IP Discard/Echo bandwidth metter.
Comment 1 Jason Tibbitts 2007-12-21 00:58:03 EST
This builds OK in mock on rawhide; rpmlint says the following:

  ndisc6.x86_64: E: setuid-binary /usr/bin/rltraceroute6 root 04755
  ndisc6.x86_64: E: setuid-binary /usr/bin/rdisc6 root 04755
  ndisc6.x86_64: E: setuid-binary /usr/bin/ndisc6 root 04755
  ndisc6.x86_64: E: non-standard-executable-perm /usr/bin/rltraceroute6 04755
  ndisc6.x86_64: E: non-standard-executable-perm /usr/bin/rdisc6 04755
  ndisc6.x86_64: E: non-standard-executable-perm /usr/bin/ndisc6 04755
Setuid binaries are troubling, expecially when the ipv4 counterparts aren't
setuid.  Has this code been properly audited?  Is it really necessary that
they be setuid?

  ndisc6.x86_64: W: symlink-should-be-relative
    /usr/share/man/man1/tcpspray6.1.gz /usr/share/man/man1/tcpspray.1.gz
  ndisc6.x86_64: W: symlink-should-be-relative
    /usr/share/man/man1/name2addr.1.gz /usr/share/man/man1/addr2name.1.gz
  ndisc6.x86_64: W: symlink-should-be-relative
    /usr/share/man/man8/tracert6.8.gz /usr/share/man/man8/rltraceroute6.8.gz
  ndisc6.x86_64: W: symlink-should-be-relative
    /usr/share/man/man8/tcptraceroute6.8.gz /usr/share/man/man8/rltraceroute6.8.gz
These should be relative symlinks; they don't really need need any pathname
information at all.

  ndisc6.x86_64: W: no-version-in-last-changelog
Changelog entries should be in one of the formats detailed in the Changelogs section of http://fedoraproject.org/wiki/Packaging/Guidelines

  ndisc6.x86_64: W: invalid-license GPL2+
I think you mean "GPLv2+"; see http://fedoraproject.org/wiki/Licensing
Comment 2 Stjepan Gros 2007-12-22 12:23:35 EST
I fixed license, added version in Changelog file, and removed all setuid bits
(as a reslut binares are moved into sbin directory instead of bin). Finally, I
modified Makefile.am, regenerated Makefile.in and generated patch in order to
remove absolute pathnames in man page's symlinks. Is there some more
efficient/elegant way to do that?

Spec URL: http://www.zemris.fer.hr/~sgros/stuff/fedora/ndisc6/ndisc6.spec
SRPM URL:
http://www.zemris.fer.hr/~sgros/stuff/fedora/ndisc6/ndisc6-0.9.3-2.fc8.src.rpm
Comment 3 Jason Tibbitts 2008-01-16 01:05:01 EST
One important question before I assign this to myself: Is this your first
package for Fedora?  You haven't indicated that you need sponsorship, but I
don't see your name on the list of packagers.  What's your FAS ID?

Since I have this package open, I might as well run through my checklist.

Looking at the source, the License: tag should "GPLv2 or GPLv3".  They permit
only those versions, not "any later version".

The latest version now seems to be 0.9.5.  Did you want to update?

You can save yourself a little pain by using the %find_lang macro; at the end of
%install, add 
  %find_lang %{name}
and then change your files section to start with:
  %files -f %{name}.lang
and remove the two %lang(* lines from your %files list.  This will automatically
find all of the .po and related files and include just the right directories and
%lang tags for you.

* source files match upstream:
   13f238cc03e43dd05020755b3a5ec57d3cfa1eecfba71dc00157d26351afe718  
   ndisc6-0.9.3.tar.bz2
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
* build root is OK.
X license field matches the actual license.
* license is open source-compatible.
* license text included in package.
? latest version is 0.9.5.
* BuildRequires are proper (none)
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly
* debuginfo package looks complete.
* rpmlint is silent.
* final provides and requires are sane:
   ndisc6 = 0.9.3-2.fc9
  =
   /bin/sh
   /usr/bin/perl
   perl(Getopt::Std)
   perl(strict)

* %check is not present; no test suite upstream.  No ipv6 here so I can't test 
   this.
* no shared libraries are added to the regular linker search paths.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no scriptlets present.
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* no headers.
* no pkgconfig files.
* no static libraries.
* no libtool .la files.
Comment 4 Stjepan Gros 2008-01-22 11:45:40 EST
Yes, this is my first package. I obviously forgot to request sponsorship. Sorry.

For the rest, I updated to new version, changed revision, generated patch to
allow build on rawhide (builds perfectly on F8 without patch), and applied your
suggestion for language specific files.

I also cleaned up the patch (it doesn't patch Makefile.am and configure.ac
files). I don't know if it's good or not? Also, this version has a perl script
that it installs into /etc/rdnssd. Is it a good place for hook scripts?

Spec URL: http://www.zemris.fer.hr/~sgros/stuff/fedora/ndisc6/ndisc6.spec
SRPM URL:
http://www.zemris.fer.hr/~sgros/stuff/fedora/ndisc6/ndisc6-0.9.5-1.fc8.src.rpm
Comment 5 Stjepan Gros 2008-03-15 06:19:37 EDT
Synchronized the package with upstream version. Patches are removed as original
package doesn't contain absolute paths anymore.

Spec URL: http://www.zemris.fer.hr/~sgros/stuff/fedora/ndisc6/ndisc6.spec
SRPM URL:
http://www.zemris.fer.hr/~sgros/stuff/fedora/ndisc6/ndisc6-0.9.7-1.fc8.src.rpm
Comment 7 Jon Ciesla 2008-10-13 10:41:47 EDT
Starting official review, and I can sponsor you.  In the meantime, please complete some unofficial reviews for others, and post links here.
Comment 8 Jon Ciesla 2008-10-13 10:56:28 EDT
MUST: rpmlint must be run on every package. The output should be posted in the review.

SRPM:
ndisc6.src:76: W: macro-in-%changelog find_lang
Macros are expanded in %changelog too, which can in unfortunate cases lead to
the package not building at all, or other subtle unexpected conditions that
affect the build.  Even when that doesn't happen, the expansion results in
possibly "rewriting history" on subsequent package revisions and generally odd
entries eg. in source rpms, which is rarely wanted.  Avoid use of macros in
%changelog altogether, or use two '%'s to escape them, like '%%foo'.

FIX.

rpmlint on RPMs is clean.


- MUST: The package must be named according to the Package Naming Guidelines .

OK.

- MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption on Package Naming Guidelines .

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.

Only includes v2, but OK.

- MUST: The spec file must be written in American English.

OK.

- MUST: The spec file for the package MUST be legible. If the reviewer is unable to read the spec file, it will be impossible to perform a review. Fedora is not the place for entries into the Obfuscated Code Contest (http://www.ioccc.org/).

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.

OK.

- MUST: The package must successfully compile and build into binary rpms on at least one supported architecture.

OK.

- 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 needs to have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number should then be placed in a comment, next to the corresponding ExcludeArch line. New packages will not have bugzilla entries during the review process, so they should put this description in the comment until the package is approved, then file the bugzilla entry, and replace the long explanation with the bug number. The bug should be marked as blocking one (or more) of the following bugs to simplify tracking such issues: FE-ExcludeArch-x86 , FE-ExcludeArch-x64 , FE-ExcludeArch-ppc , FE-ExcludeArch-ppc64

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.

Running a mock build to test this.

- MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.

OK.

- MUST: Every binary RPM package which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. If the package has multiple subpackages with libraries, each subpackage should also have a %post/%postun section that calls /sbin/ldconfig. An example of the correct syntax for this is:

%post -p /sbin/ldconfig

%postun -p /sbin/ldconfig

NA.

- 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.

NA.

- 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. Refer to the Guidelines for examples.

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, as described in the macros section of Packaging Guidelines .

OK.  Consider using %{version} where you can, like in the Source0.  SAve yourself work later.

- MUST: The package must contain code, or permissable content. This is described in detail in the code vs. content section of Packaging Guidelines .

OK.

- MUST: Large documentation files should 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)

NA.

- 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.

OK.

- MUST: Header files must be in a -devel package.

NA.

- MUST: Static libraries must be in a -static package.

NA.

- MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability).

NA.

- 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.

NA.

- MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}

NA.

- MUST: Packages must NOT contain any .la libtool archives, these should be removed in the spec.

OK.

- 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. This is described in detail in the desktop files section of the Packaging Guidelines . 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.

NA.

- 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 ). See Prepping BuildRoot For %install for details.

OK.

- MUST: All filenames in rpm packages must be valid UTF-8.

OK.
Comment 9 Jon Ciesla 2008-10-13 11:11:47 EDT
BRs are good, so fix the macro in the changelog, show me some sample reviews, and we're good to approve and sponsor.
Comment 10 Jon Ciesla 2008-12-29 09:22:53 EST
Ping?
Comment 11 Stjepan Gros 2008-12-29 13:25:49 EST
I'm here, but I was busy doing my PhD and then came Christmas. I'll look into packages that need review to see if there are related ones.
Comment 12 Jon Ciesla 2008-12-29 13:33:45 EST
I suppose that's a pretty good excuse. :)  Hope it went well.
Comment 13 Stjepan Gros 2008-12-29 17:07:40 EST
Ok, I fixed the omission in the changelog section. The new spec and src.rpm files are available on the following URLs:

Spec URL: http://www.zemris.fer.hr/~sgros/stuff/fedora/ndisc6/ndisc6.spec
SRPM URL:
http://www.zemris.fer.hr/~sgros/stuff/fedora/ndisc6/ndisc6-0.9.8-2.fc8.src.rpm

As for review of some packages, I will be hard to find one that's not already reviewed. :) Do you maybe have some suggestion?
Comment 14 Jon Ciesla 2008-12-30 09:05:51 EST
Who says it can't already be in the process of being reviewed?  Just mention that it's practice and not binding.  Pick something you feel somewhat confident in, maybe in a language you have some experience with.
Comment 15 Jon Ciesla 2009-03-31 09:20:29 EDT
Ping?
Comment 16 Stjepan Gros 2009-04-02 03:40:10 EDT
Sorry for not letting you know, but I wont be able to do much until summer. Feel free to take this package.
Comment 17 Jon Ciesla 2009-04-02 08:52:51 EDT
Ok.  CC list, if I want to put in this package under the aegis of my own ownership, will it require a fresh review, or can I essentially self-approve this one?
Comment 18 Stjepan Gros 2009-11-15 04:24:34 EST
What happened with this package? I don't see it appeared in rawhide or F12. This ticket is still open. Did anyone taken it?
Comment 19 Jon Ciesla 2009-11-16 08:30:25 EST
I never got an answer to #17.  Are you back?
Comment 20 Stjepan Gros 2009-11-16 11:38:01 EST
Actually, I'm here for some time already. But I thought that everything was settled with this entry, i.e. that you'll take it, so I didn't post anything to this thread?
Comment 21 Jon Ciesla 2009-11-16 11:45:04 EST
Nope, it's a resounding nothing.  I've had a. . .full. . .personal life since post #17, so I've not touched it.  If you want to do the practice reviews, we can pick up where we left off.
Comment 22 Stjepan Gros 2009-11-16 12:17:24 EST
Ok, I'll repackage ndisc6 (there is a new version) and submit it here and then we'll see what to do next.
Comment 23 Jon Ciesla 2009-11-16 13:14:08 EST
Removed NEEDSPONSOR blocker.  I'll look over the new version when you post it, and approve it nothing new creeps up.
Comment 25 Jon Ciesla 2009-11-17 08:03:40 EST
SRPM is 404.
Comment 27 Jon Ciesla 2009-11-17 09:14:27 EST
This looks good too, APPROVED.
Comment 28 Stjepan Gros 2009-11-18 03:00:53 EST
New Package CVS Request
=======================
Package Name: ndisc6
Short Description: IPv6 diagnostic tools
Owners: sgros
Branches: F-12
InitialCC: sgros
Comment 29 Itamar Reis Peixoto 2009-11-18 09:55:05 EST
why not F-11 and EL-5 too ?
Comment 30 Stjepan Gros 2009-11-18 10:05:26 EST
No particular reason, inertia primarily. I can request CVS branches for F-11 and EL-5 too, but first I'll have to look how that's done. :)
Comment 31 Jon Ciesla 2009-11-18 10:10:39 EST
Similarly to #28.  Set the cvs flag to ?, and see this:

https://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure
Comment 32 Stjepan Gros 2009-11-19 03:21:56 EST
New Package CVS Request
=======================
Package Name: ndisc6
Short Description: IPv6 diagnostic tools
Owners: sgros
Branches: F-11 F-12 EPEL
InitialCC: sgros
Comment 33 Jason Tibbitts 2009-11-19 18:06:06 EST
Your CVS request is not well-formed; there is no branch named "EPEL".  We can make either EL-4 or EL-5 branches (or both); please indicate which you would like in a corrected CVS request.
Comment 34 Stjepan Gros 2009-11-20 02:53:26 EST
New Package CVS Request
=======================
Package Name: ndisc6
Short Description: IPv6 diagnostic tools
Owners: sgros
Branches: F-11 F-12 EL-5
InitialCC: sgros
Comment 35 Dennis Gilmore 2009-11-20 17:44:26 EST
CVS Done
Comment 36 Fedora Update System 2009-11-23 07:15:43 EST
ndisc6-0.9.9-1.el5 has been submitted as an update for Fedora EPEL 5.
http://admin.fedoraproject.org/updates/ndisc6-0.9.9-1.el5
Comment 37 Fedora Update System 2009-11-23 07:18:43 EST
ndisc6-0.9.9-1.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/ndisc6-0.9.9-1.fc11
Comment 38 Fedora Update System 2009-11-23 07:20:25 EST
ndisc6-0.9.9-1.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/ndisc6-0.9.9-1.fc12
Comment 39 Fedora Update System 2009-11-25 10:04:28 EST
ndisc6-0.9.9-1.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 40 Fedora Update System 2009-11-25 10:12:34 EST
ndisc6-0.9.9-1.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 41 Fedora Update System 2009-12-09 23:01:02 EST
ndisc6-0.9.9-1.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 42 Stjepan Gros 2010-02-06 16:02:17 EST
Finally closing this ticket. Package for a long time in repositories.

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