Where does the tarball come from?  Upstream doesn't seem to have any actual
tarball abailable for download.  If it's from a CVS checkout, can you detail
in the spec (or in a script) how you do the checkout, and name the tarball and
choose the release appropriately based on the checkout date as detailed in the
naming guidelines at http://fedoraproject.org/wiki/Packaging/NamingGuidelines

Basically, the version-release pair should be something like
1.84-11.20070203cvs.  If you're checking out a tag, I'm not sure what the best
way to name it is.

Or perhaps we could consider whether or not one of the other netcat variants
is a better choice.

Rpmlint has a few complaints:

W: nc summary-ended-with-dot Reads and writes data across network connections using TCP or UDP.
W: nc summary-ended-with-dot Reads and writes data across network connections using TCP or UDP.
  Easy to fix these up.

W: nc doc-file-dependency /usr/share/doc/nc-1.84/scripts/alta /bin/sh
W: nc doc-file-dependency /usr/share/doc/nc-1.84/scripts/bsh /bin/sh
W: nc doc-file-dependency /usr/share/doc/nc-1.84/scripts/dist.sh /bin/sh
W: nc doc-file-dependency /usr/share/doc/nc-1.84/scripts/irc /bin/sh
W: nc doc-file-dependency /usr/share/doc/nc-1.84/scripts/iscan /bin/sh
W: nc doc-file-dependency /usr/share/doc/nc-1.84/scripts/ncp /bin/sh
W: nc doc-file-dependency /usr/share/doc/nc-1.84/scripts/probe /bin/sh
W: nc doc-file-dependency /usr/share/doc/nc-1.84/scripts/web /bin/sh
W: nc doc-file-dependency /usr/share/doc/nc-1.84/scripts/webproxy /bin/sh
W: nc doc-file-dependency /usr/share/doc/nc-1.84/scripts/webrelay /bin/sh
W: nc doc-file-dependency /usr/share/doc/nc-1.84/scripts/websearch /bin/sh
  Documentation shouldn't be executable.

Review:
X I can't check whether the source files match upstream.
X package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* dist tag is present.
X build root is not correct; should be
  %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
X license field matches the actual license.
   I can find no mention of the GPL; this looks to me to be more like the X11
   license.
* license is open source-compatible.  License text not included upstream.
? latest version is being packaged.
* BuildRequires are proper.  The requirement for pkgconfig is unnecessary,
  though, as glib2-devel requires it.
* compiler flags are appropriate.
* %clean is present.
* %makeinstall is not used.
* package builds in mock.
* debuginfo package looks complete.
X rpmlint is silent.
* final provides and requires are sane:
   nc = 1.84-10.fc7
  =
   /bin/sh
   glib2
* %check is not present; no test suite upstream.
* 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 libtool .la droppings.

The source comes from OpenBSD cvs. No tarball available. I will look at these
hopefully next week or the week after. Patches are welcome.

Add upstream source. I have no idea when the checkout was made either which tag.
I would like to keep the source file as it is. 


W: nc doc-file-dependency /usr/share/doc/nc-1.84/scripts/alta /bin/sh
- these are only warnings and I'm not going to change it. The directory contains
test scripts, examples which are obviously part of documentation and it's
correct that they are executable.

See changes in 1.84-11

Another ticket that disappeared from my list due to assignment pingponging. 
Fortunately we don't need to do that any longer.  I'll get back to this today.

Unfortunately the source thing is really problematic for me and I suspect many
others; it's pretty much completely antithetical to the goal of Fedora to track
upstream as much as possible.  Currently we have an unreproducible tarball and
17 patches that taken together are larger than the source itself.

I understand that it would be truly difficult to rebase all of these patches, so
I don't really know what should happen here.  Perhaps we need a few volunteers
to work on the problem.  Or perhaps switching to one of the forked upstream
projects would be better, especially if they support ipv6.  But I would be
really uncomfortable about putting my "approved" stamp on this package without
having some public discussion on the issue.

The issue I have with OpenBSD source is that it's not compileable with our
gcc/kernel. Basically I guess that BSD sockets have some new features we don't
support yet. Current OpenBSD source cannot be compiled on Linux without patching
it. I would rather make nc obsolete in favor of nc6 which implements also better
IPv6 support. At one point the nc6 upstream was quite active, but right now they
seem to be rather silent.

OK, let's take a pragmatic view.  We can't just pull nc from the distro and what
we have is working.  According to the spec there are only nine patches instead
if the 17 patches that are in CVS.  So let's do this:

Document in the spec that the tarball was extracted from whatever CVS repository
it came from, but indicate that the pull date or upstream tag is unknown.

Clean up the unapplied patches from the CVS repo so the situation doesn't look
so bad.

Make a plan for what's going to happen post-F7, be that to rebase to current
upstream (though I doubt they'll take our patches) or to switch to something else.

Not sure how the component got changed. 

BTW, it took me just a few minutes to figure out that you can get the upstream
source used in the tarball with:

CVSROOT=anoncvs@anoncvs1.usa.openbsd.org:/cvs cvs co -D 2005-10-26 src/usr.bin/nc

However, the scripts directory that appears in the tarball present in the SRPM
has a scripts directory that does not come from upstream CVS.

The version of netcat that is in fedora now has a bug where it refuses to connect to proxies that return "HTTP/1.1 200" messages (it only accepts ("HTTP/1.0 200" as valid).  I discovered this bug when attempting to ssh out through a web proxy using openssh's ProxyCommand directive like this:

ProxyCommand = /usr/bin/nc -X connect -x proxy:8080 %h %p

Looking at the OpenBSD repository it looks like socks.c was patched to fix this over 2 years ago.  Would be nice if this change got pulled down into the Fedora package.

http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/nc/socks.c.diff?r1=1.16;r2=1.17;f=h

Ben, you should open a ticket for your issue as it is not relevant to this review.

Hi, I'm current maintainer of nc and I'd like to finish this review. Could you please check latest build in Koji? I think everything is ok now.

OK, I checked out the current CVS and indeed it builds fine and rpmlint is silent.

However, there are still a couple of issues:

There's still no documentation on actually recreating the CVS checkout.  I don't know if the information I posted nearly three years ago in comment #8 is still valid, but we really so need to get some information on precisely where the source comes from.  http://fedoraproject.org/wiki/Packaging:SourceURL

When a package accumulates so many patches, it is really important to keep track of their upstream status.  http://fedoraproject.org/wiki/Packaging:Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment
At this point, given the number of patches with no information about their upstream status and the lack of information on the upstream source, it begins to seem as if Fedora is maintaining its own fork of nc, which I doubt is something that we want to be doing.

Nothing seems to own /usr/share/nc; you should probably just add "%{_datadir}/nc" to %files and remove the two lines referencing %{_datadir}/nc/scripts.  But then there are a couple of other points:

If you expect that people will actually want to run those scripts, why not give them sensible non-conflicting names and actually install them in the usual path for executables?

If they aren't supposed to be run, why not keep them in %doc?

I have prepared another commit, but first I need to resolve the patch tracking thing. The thing is that the situation is very similar to what you described. Fedora took sources from OpenBSD nc and some patches are there as a result of porting process, so in fact, we kinda have Linux fork of nc.

My point here is that describing patches and making tracking bugs for them is IMO an unnecessary work. First reason I already wrote. Then there is the fact, that nc upstream seems to be almost dead, there are only small changes in CVS and they appear like twice a year. There have also been almost no big changes in Fedora version of netcat (rebase didn't appear in last 4 years). And finally - I don't intent to make some major changes in nc. In fact, I'd like to get ncat to Fedora and continue major development there. For now, I want to keep nc and maybe in time it will be appropriate to retire nc entirely.

So what do you think - should I really try to track all the patches that haven't been tracked for years or is it ok to skip this step considering my arguments?

Honestly I'm not sure what the best course of action would be.  The current situation is something we want to get away from; we don't want to be maintaining the source directly inside the Fedora package.  We really don't like pointless forking and all the "Fedora doesn't work with upstream" grousing that comes with it, and we've worked very hard to avoid that kind of thing.

Stopping short of filing a bunch of upstream tickets for each of our patches, how hard would it be to let upstream know that regardless of what's happened in the past, Fedora would like to reduce or eliminate the current divergence and show them the current set of patches we're carrying?  If they reject or ignore the attempt at contact then we can at least get a quick fedorahosted project set up so that there's some public source for what's in this package.

Alternately, I'm happy to open a FESCo ticket and ask them for guidance.  They could grant an exception, but more likely they'd recommend something not much different than I detail above.

I re-wrote all patches and emailed them to upstream (email to their mailing list is the only way they handle bug reports and similar stuff).

Is it enough to comment those patches which I've sent them (not all are convenient for upstream, they are Linux and GCC specific) that I sent them to upstream? So far I cannot find usable link to their mailing list archives. The rest of patches will be commented that they are Fedora specific.

I had typed up several comments and then lost them all because I added the wrong @redhat.com address for the current maintainer.  Let me try again.

OK, that worked.

Looking at the package again after all this time, I see that it's considerably cleaned up.  Really there are two issues:

The line for doing the CVS checkout will get whatever's at the head of the tree at the point when you do the checkout.  It's not generally useful to get what's in the tarball (and indeed, I get something different if I check out now).  If upstream doesn't actually provide tarballs and doesn't actually tag their releases then the best you can do is provide a checkout date.

The bile b64_ntop.c appeared without any providence.  It appears to be pulled from another project.  While I find it hard to believe there's no small library around with base64-decoding function, I guess that's the case.  I'll run this by the FPC to see if there's agreement that this is an issue.

(In reply to comment #17)
> OK, that worked.
> 
> Looking at the package again after all this time, I see that it's considerably
> cleaned up.  Really there are two issues:
> 
> The line for doing the CVS checkout will get whatever's at the head of the tree
> at the point when you do the checkout.  It's not generally useful to get what's
> in the tarball (and indeed, I get something different if I check out now).  If
> upstream doesn't actually provide tarballs and doesn't actually tag their
> releases then the best you can do is provide a checkout date.

That's a good point.  I've just been following the versioning tradition used before I took this package.  It would be fine if the whole project was just netcat.c :)

> The bile b64_ntop.c appeared without any providence.  It appears to be pulled
> from another project.  While I find it hard to believe there's no small library
> around with base64-decoding function, I guess that's the case.  I'll run this
> by the FPC to see if there's agreement that this is an issue.

This is somehow available on BSDs.  One option was to add this to libbsd but I saw it was quite common to "bundle" this implementation with other projects.  I don't think this case, although ugly, is against our guidelines.  However,  checking with FPC is a good idea.

I filed https://fedorahosted.org/fpc/ticket/172 to track the bundling issue, but Toshio commented that the function in question is part of glibc already so there appears to be no reason to bundle it already.  You just need to include resolv.h and link with libresolv.

You obviously know more about this than I do, so I'm sure there's something I'm missing.  Is the glibc implementation not useful somehow?

FPC approved a bundling exception for this code.  So it's up to you; use what's already in glibc, or continue to bundle and add Provides: bundled(base64-isc) to the spec.

I've tried using the glibc implementation and it seems to work flawlessly.  I'll happily unbundle what we have now.

About the version -- I guess I should switch to <netcat.c version>.<checkout date> to be more clear what's actually in the package.

In the long run, I'd prefer killing nc and nc6 in Fedora in favor of nmap ncat which I think is the best netcat around now.  The only problem is the nmap package is rather large and not all nc users probably need or want it.  I wonder if some of its tools could be subpackaged? CC'ing nmap maintainer for insight.

> In the long run, I'd prefer killing nc and nc6 in Fedora in favor of nmap
> ncat which I think is the best netcat around now.  The only problem is the
> nmap package is rather large and not all nc users probably need or want it. 
> I wonder if some of its tools could be subpackaged? CC'ing nmap maintainer
> for insight.

It's possible to create nmap sub-package, but I'm going to invest that effort only if nc is really going to die.

(In reply to comment #22)
> > In the long run, I'd prefer killing nc and nc6 in Fedora in favor of nmap
> > ncat which I think is the best netcat around now.  The only problem is the
> > nmap package is rather large and not all nc users probably need or want it. 
> > I wonder if some of its tools could be subpackaged? CC'ing nmap maintainer
> > for insight.
> 
> It's possible to create nmap sub-package, but I'm going to invest that
> effort only if nc is really going to die.

Sounds like a tough task when you put it like this :)

I've just pushed the suggested changes to rawhide and f17 (https://admin.fedoraproject.org/updates/nc-1.107.20120403-1.fc17).

ncat does not support talking over unix socket which might break existing use cases.

Have you considered calling the subpackage "ncat" instead of "nmap-ncat"? The package name is prominently visible when searching/installing for packages, so it might make sense to keep it simple.

(In reply to comment #24)
> ncat does not support talking over unix socket which might break existing
> use cases.

A workaround script calling socat is now in place.

(In reply to comment #25)
> Have you considered calling the subpackage "ncat" instead of "nmap-ncat"?
> The package name is prominently visible when searching/installing for
> packages, so it might make sense to keep it simple.

Since it's an nmap subpackage, such a name is not possible.
Whether something is visible or not is a matter of package management tools; I believe searching for 'ncat' should return this package too -- or that's something I expect from any of those tools out there :)

Anyhow, the previous name 'nc' is still provided so if you search for that, you get nmap-ncat too.

(In reply to comment #26)
> Since it's an nmap subpackage, such a name is not possible.

It actually is possible, with the -n switch that %package, %description and %files take. It makes it possible to use any name for binary packages, not only those that are prefixed with the source package name.

I'm attaching a patch that demonstrates this.

Created attachment 601219 [details]
Rename nmap-ncat to ncat

That's actually nice.  I didn't know about this; thank you.

(In reply to comment #25)
> Have you considered calling the subpackage "ncat" instead of "nmap-ncat"?
> The package name is prominently visible when searching/installing for
> packages, so it might make sense to keep it simple.

Yes, I considered it, but I don't want to change its name. First, I don't think there is any trouble with the name as it is right now. I don't find it confusing and it's short and simple. Second, it'll be easier for users to find out what is a correct component in bugzilla when filing new bug reports.

Given that nc is retired and not branched for f18, I'm going to go ahead and close this out.  Thanks for making things happen.