Bug 1027770 - Review Request: ocserv - OpenConnect SSL VPN server
Review Request: ocserv - OpenConnect SSL VPN server
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Alec Leamas
Fedora Extras Quality Assurance
:
Depends On: 1029002
Blocks:
  Show dependency treegraph
 
Reported: 2013-11-07 06:53 EST by Nikos Mavrogiannopoulos
Modified: 2014-01-21 09:04 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-12-06 08:34:24 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
leamas.alec: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)
Licenses as listed by fedora-review (16.19 KB, text/plain)
2013-11-09 04:32 EST, Alec Leamas
no flags Details
Updated license check list (10.59 KB, text/plain)
2013-11-11 13:53 EST, Alec Leamas
no flags Details

  None (edit)
Description Nikos Mavrogiannopoulos 2013-11-07 06:53:36 EST
Hello, this is my first fedora package and I need a sponsor. Note that I am also the upstream maintainer of this application.

Spec URL: ftp://ftp.infradead.org/pub/ocserv/srpm/ocserv.spec
SRPM URL: ftp://ftp.infradead.org/pub/ocserv/srpm/ocserv-0.2.1-1.src.rpm

Description: OpenConnect server (ocserv) is an SSL VPN server. Its purpose is to be a secure, small, fast and configurable VPN server that uses standard protocols such as TLS 1.2, and Datagram TLS. It implements the OpenConnect SSL VPN protocol, which is compatible with the AnyConnect SSL VPN protocol.

Fedora Account System Username: nmav
Comment 1 Christopher Meng 2013-11-07 07:00:23 EST
I don't think that you need a sponsor, right?

https://admin.fedoraproject.org/accounts/user/view/nmav
Comment 2 Nikos Mavrogiannopoulos 2013-11-07 07:23:00 EST
(In reply to Christopher Meng from comment #1)
> I don't think that you need a sponsor, right?

Indeed sorry. I meant I need someone to review it.
Comment 3 Alec Leamas 2013-11-07 12:28:33 EST
Hi! By setting the fedora-review flag to '?' you indicate that the review is ongoing. It's normally set by the reviewer. When set it this way, reviewers looking for a package to review will not find it. Details:
http://fedoraproject.org/wiki/Package_Review_Process.
Comment 4 Nikos Mavrogiannopoulos 2013-11-08 03:00:31 EST
(In reply to Alec Leamas from comment #3)
> Hi! By setting the fedora-review flag to '?' you indicate that the review is
> ongoing. It's normally set by the reviewer. When set it this way, reviewers
> looking for a package to review will not find it. Details:
> http://fedoraproject.org/wiki/Package_Review_Process.

Thanks. Now it's removed.
Comment 5 Alec Leamas 2013-11-08 08:17:28 EST
Hi again!

I'm no sponsor, but I noticed some issues (there are certainly more) while skimming through your spec:

First, there are some things which are not needed unless you intend to use this on EPEL. If not, just remove them: 
- rm -rf %{buildroot}
- %clean (whole section)
- %defattr(-,root,root,-)
OTOH, if you are heading for EPEL you need a Buildroot:  tag.

You are using /etc/pam.d without requiring the owner of this dir. Also,
you have a file dependency on /etc/pamd.d/system-auth. Replacing that Requires: with Requires: pam solves both problems (since pam owns bot the dir and the file).

Is the BuildRequires: autogen really needed?

You have placed the ocserv state files in /var/ocserv. This is not really as intended, use /var/lib/ocserv instead. See http://www.pathname.com/fhs.

Thou shall not use %makeinstall [1]

You have a lot of licenses, not just GPLv2 such as GPLv3+, LGPL, LGPL2.1 and X11. Use  the licensecheck tool to get the complete story, and look into [2] to write a proper license tag. 

Add the disttag to Release: It's not mandatory, but should be :) [3]

If you cannot use %{?_smp_mflags}, add a comment line above explaining
the situation.

Add the -p option to all install commands in order to preserve modification times.

Use a /etc/ocserv dir to hold ocserve.conf. There's a link about that somewhere, I don't find. It's just a little cleaner.

Use a wildcard for the manpage file types (the compression might change) like in  %{_mandir}/man8/ocpasswd.8*

Add Requires(pre): shadow-utils for the %pre snippet, and remove Requires: /usr/sbin/useradd.

What are you trying to achieve with the truggerun macro? Triggers are normally used to execute code in one package when some other package is (un)installed. Here, both trigger and target package is ocserv, which does not really make sense. Furthermore, the net effect of that macro seems to be that you start the service by default which requires a FESCO permission [4]. 

Did I say that this is a good spec? It is. Don't take these remarks as something else :)

BTW: Don't forget to update the changelog after fixing these remarks!


[1] http://fedoraproject.org/wiki/Packaging:Guidelines#Why_the_.25makeinstall_macro_should_not_be_used
[2] http://fedoraproject.org/wiki/Packaging:LicensingGuidelines
[3] http://fedoraproject.org/wiki/Packaging:DistTag
[4] https://fedoraproject.org/wiki/Starting_services_by_default
Comment 6 Alec Leamas 2013-11-08 08:27:40 EST
Oh, you don't need a sponsor. I might be able t review this later then. Leaving it unassigned if someone else wants to step in.
Comment 7 Nikos Mavrogiannopoulos 2013-11-08 10:06:15 EST
(In reply to Alec Leamas from comment #5)

Thanks for the nice comments. I've addressed them (all except the first).

> I'm no sponsor, but I noticed some issues (there are certainly more) while
> skimming through your spec:
> First, there are some things which are not needed unless you intend to use
> this on EPEL. If not, just remove them: 
> - rm -rf %{buildroot}
> - %clean (whole section)
> - %defattr(-,root,root,-)
> OTOH, if you are heading for EPEL you need a Buildroot:  tag.

I'm not sure whether I want that or not :) I left it for now and added the Buildroot tag.

> Is the BuildRequires: autogen really needed?

Unfortunately yes.

> Thou shall not use %makeinstall [1]
I don't this it was used. I was using %make_install.

> You have a lot of licenses, not just GPLv2 such as GPLv3+, LGPL, LGPL2.1 and
> X11. Use  the licensecheck tool to get the complete story, and look into [2]
> to write a proper license tag. 

Well, the library used that has the LGPLv3, GPLv3 license options (libopts) is also available under simplified BSD (COPYING.mbsd), so GPLv3 or even LGPLv3 don't apply here. I only install the mbsd license to make that clear. So overall the package is under GPLv2+.
Comment 8 Alec Leamas 2013-11-09 04:31:02 EST
(In reply to Nikos Mavrogiannopoulos from comment #7)
> (In reply to Alec Leamas from comment #5)
>
> > Thou shall not use %makeinstall [1]
> I don't this it was used. I was using %make_install.
Oops, sorry, my bad. Need new glasses, it seems.

> 
> > You have a lot of licenses, not just GPLv2 such as GPLv3+, LGPL, LGPL2.1 and
> > X11. Use  the licensecheck tool to get the complete story, and look into [2]
> > to write a proper license tag. 
> 
> Well, the library used that has the LGPLv3, GPLv3 license options (libopts)
> is also available under simplified BSD (COPYING.mbsd), so GPLv3 or even
> LGPLv3 don't apply here. I only install the mbsd license to make that clear.
> So overall the package is under GPLv2+.

I attach the licenses list as produced by fedora-review. At a glance it looks like the license after promoting LGPL nd GPLv2+ would be (GPLv3 and MIT). Could you please comment on this?

Here are also some bad FSF addresses, see [1] for handling.

More important:  It seems that here is a bundling issue: Some of the files with Public Domain "license" seems to be from [2]. As this is a defined upstream, these files should be unbundled [3].

Other bundled files seems to be from the libopts library. Here is probably more, these was just the first I found.

It might be that these files are actrually not used in the build. IN this case they should be removed in %prep  [4].

[1] https://fedoraproject.org/wiki/Common_Rpmlint_issues#incorrect-fsf-address
[2] http://burtleburtle.net/bob/
[3] https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries
[4] https://fedoraproject.org/wiki/Packaging:Treatment_Of_Bundled_Libraries
Comment 9 Alec Leamas 2013-11-09 04:32:19 EST
Created attachment 821858 [details]
Licenses as listed by fedora-review
Comment 10 Nikos Mavrogiannopoulos 2013-11-09 11:53:46 EST
(In reply to Alec Leamas from comment #9)
> Created attachment 821858 [details]
> Licenses as listed by fedora-review

Hello, are the licenses uploaded because you see any issue with them?
Comment 11 Alec Leamas 2013-11-10 04:32:03 EST
Now, I don't think we understand each other. I have run the fedora-review tool, which basically runs the licensecheck tool but presents the results in a slightly different way. These tools check all source files (after possible patching in %prep), and the result is in the attachment. Bottom line is that here is a lot of licenses.However, it should not be that hard to sort out since they are all compatible.

The bundling issues will probably require more work. After a closer look I find at least the following bundled libraries:
- build-assert, check_type, container_of, hash, htable and list from http://ccodearchive.net/ (ccan dir).
- http_parser from the existing http-parser package
- pcl from https://github.com/knz/pcl.
- The build-aux files from lib.idn.h
- Several files form autogen-libopts (.../libopts/...). Other packages have a bundling exception for these, so it should not be a problem to apply for one for those. Before that, we need to check on the mailing list, there might be some  kind of overall decision for these libs, don't know. 
- The gl directory might also be something bundled, but I havn't looked more into this.

According to the links in comment #8, these must be unbundled. If there is an existing fedora package, it should be used. If not, you should package these dependencies as separate packages. patch the build system to use them and remove them in %prep i. e., unbundle it. In some case you might need to apply for bundling exception, but normally this is the last resort if nothing else works.
Comment 12 Nikos Mavrogiannopoulos 2013-11-10 09:27:43 EST
Ok now I understand. The other libraries are:
1. from CCAN, which is a repository of code. There is no library there that could be added to fedora. So I don't think there is something that can be done there.
2. from gnulib, which is also a repository of code with no library. It is used by copying code from their repository to the project's repository.
3. build-aux: These are files installed by the GNU auto-tools. They are not used by ocserv, As far as I understand they are only used for building and the test cases.
4. libopts: It is bundled but in the spec, the fedora's libopts is being used.

Now the problematic seems to be:
5. http-parser: Need to link with fedora's http-parser.
6. libpcl: Need to add libpcl to fedora as well.

So I'll try to handle the last two, and also remove the code of them (as well as libopts).
Comment 13 Alec Leamas 2013-11-10 09:59:09 EST
Seems reasonable to begin with those, which are simple. However, the argument that "this upstream is not a library, just a repository" is simply not enough to avoid handling the bundled code.

So, in the end you will need to get bundling exception(s) or unbundle also the ccan and gnulib files. Perhaps it could be handled as copylibs(?), but you will then still need a formal bundling exception.  I suggest you seek some help on the mailing list on how to handle ccan and gnulib. There are knowledgeable people on that list... Sending two separate messages, one about  each lib is probably the best. Making the sources available with a link in the message is also good.

This will be some work for you. Assigning the review, we need to complete this.
Comment 14 Nikos Mavrogiannopoulos 2013-11-11 07:27:44 EST
I've put the new spec and SRPMs at:
http://people.redhat.com/nmavrogi/fedora/

That includes the pcllib.
Comment 15 Alec Leamas 2013-11-11 07:35:00 EST
OK, next step is that you make a new review request for pcllib. When you have done that, mark this review as blocked by the new review request (enter new bug # in the 'Depends On' field).

I'll try to follow you with this. It's certainly not an easy pick for being your first package, but it should be possible to sort it out given some time.

Noted you message about ccan on the list. Good!
Comment 16 Alec Leamas 2013-11-11 08:02:59 EST
Answer on the list. You need to apply for a bundling exception at [1] for the ccan files. Things to include in the request:

- That you apply for these files being handled as "copylib" as of [2].
- The answer to the standard questions, also as of [2].
- That this is your first package (makes things easier...)


[1] https://fedorahosted.org/fpc/
[2] https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries
Comment 17 Alec Leamas 2013-11-11 08:16:14 EST
Great! Looking into [2] in comment #16, there is already a copy-lib exception for gnulib. Locate the link which should be referenced in a comment and don't forget to add a 'Provides: bundled(gnulib)'  and: gnulib done!
Comment 18 Nikos Mavrogiannopoulos 2013-11-11 08:22:56 EST
Thank you for all the help. I have just submitted the exception request and updated the spec for gnulib and to remove the bundled files.
Comment 19 Alec Leamas 2013-11-11 08:56:02 EST
(In reply to Nikos Mavrogiannopoulos from comment #18)
> Thank you for all the help. 
You're welcome. I remember the feeling when I did my first package.,..

> I have just submitted the exception request and
> updated the spec for gnulib and to remove the bundled files.
You missed the standard questions. It might work, but chances are that FPC just requests more info. That would cost you a week, at least. The GL are specific: you must have those answers (that is not to say everybody follows this strictly)
Comment 20 Alec Leamas 2013-11-11 13:50:19 EST
Progress, indeed. The bundling issues are resolved besides ccan. (The build-aux files can be shipped under the general autotools blanket). For ccan, we must wait for fpc. Things will also become easier once pcllib hits rawhide, but we can go around that for now.

Next major task is the licenses. I will attach a new version of the licensecheck output. fedora-review tells us:

     Note: Checking patched sources after %prep for licenses. Licenses found:
     "LGPL", "GPL (v2 or later)", "GPL (v3 or later)", "Unknown or generated",
     "LGPL (v2.1 or later)", "BSD (3 clause) GPL (v2 or later)". 17 files have
     unknown license. Detailed output of licensecheck in [attachment].

This is *not* as simple as GPLv2+. Furthermore, when looking into the unknown license files many (most?) are the ccan files. However, they all have a license (the scanning fails). Some are already existing licenses, some are "Public Domain".

Also, the one who copied those files from ccan did not copy the LICENSE file. Each ccan module has a license file, and that file should be added to the package. You should also talk to upstream about this. A module contains a license file for a reason.

Summing up:
 - Check all licenses in the package, try to understand the situation.
 - Write the new License: tag, something like (GPLv3+ and MIT and Public Domain...)
 - You will need to make a license break-down, and add that to the package.Use the attachment as a starting point.
 - Add all missing license files from ccan.

License links: comment #5
Comment 21 Alec Leamas 2013-11-11 13:53:16 EST
Created attachment 822571 [details]
Updated license check list
Comment 22 Alec Leamas 2013-11-11 13:58:43 EST
Oops, my bad again! The ccan license files are already in place. Sorry, forget what I said about those"
Comment 23 Nikos Mavrogiannopoulos 2013-11-11 14:28:12 EST
Hi Alec, is the situation with licenses that complicated? My understanding is that the combination of MIT+BSD+GPLv2+ = GPLv2+ . That is because GPLv2 has the strongest requirements on the final binary and the other licenses don't impose any additional requirements to GPLv2 clauses. 

For example the linux kernel includes (or included) BSD files but still is considered GPLv2. If I mention that the project license is GPLv2+ and MIT and Public Domain, it may imply it is triple-licensed which is not the case (one may not chose to distribute ocserv under MIT).

The GPLv3 files are not relevant for licensing purposes as they are only applicable to building -autotools (and it's pretty unfortunate that auto-tools installs them).
Comment 24 Alec Leamas 2013-11-11 14:45:46 EST
Hi!

(In reply to Nikos Mavrogiannopoulos from comment #23)
> Hi Alec, is the situation with licenses that complicated? My understanding
> is that the combination of MIT+BSD+GPLv2+ = GPLv2+ . 
No. It works as described in [1]

>  The GPLv3 files are not relevant for licensing purposes as they are only
> applicable to building -autotools (and it's pretty unfortunate that
> auto-tools installs them).
You don't really build autotools here. You build some files which are used to build the package. That said, you might very well be right in this. But there is still a lot of licenses.

[1]http://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios
Comment 25 Alec Leamas 2013-11-11 14:48:36 EST
(In reply to Nikos Mavrogiannopoulos from comment #23)
 
> The GPLv3 files are not relevant for licensing purposes as they are only
> applicable to building -autotools (and it's pretty unfortunate that
> auto-tools installs them).

If this is correct you should be able to remove them in %prep since you run autoreconf (possibly adding -fi options).
Comment 26 Nikos Mavrogiannopoulos 2013-11-11 15:43:37 EST
(In reply to Alec Leamas from comment #25)
> (In reply to Nikos Mavrogiannopoulos from comment #23)
>  
> > The GPLv3 files are not relevant for licensing purposes as they are only
> > applicable to building -autotools (and it's pretty unfortunate that
> > auto-tools installs them).
> 
> If this is correct you should be able to remove them in %prep since you run
> autoreconf (possibly adding -fi options).

I've checked it further and it seems that these are gnulib installed files. I've asked a question in gnulib-bugs lists. It may be a bug in the gnulib-tool, that needs to be addressed. For the moment I'm adding the GPLv3 to the license list.
Comment 27 Nikos Mavrogiannopoulos 2013-11-12 07:25:16 EST
(In reply to Nikos Mavrogiannopoulos from comment #26)

> I've checked it further and it seems that these are gnulib installed files.
> I've asked a question in gnulib-bugs lists. It may be a bug in the
> gnulib-tool, that needs to be addressed. For the moment I'm adding the GPLv3
> to the license list.

Indeed it seems to be a gnulib bug [0]. Alec thanks for insisting on that issue, as it uncovered an issue that affects quite some projects.

[0]. http://lists.gnu.org/archive/html/bug-gnulib/2013-11/msg00062.html
Comment 28 Alec Leamas 2013-11-12 07:44:53 EST
Hm... As for ocserv, I think upstream has confirmed that the proper license for those files should be GPLv2+. This makes it possible to patch the files, with the reference above in a comment.

The easiest is perhaps to add some sed patching in %prep. Don't forget the reference, patching licenses is no walk in the park ;) That's one license less.

While we're on it: Although the GL makes it necessary to list all the licenses, you can still promote things as long as the license allows. E. g., you can promote LGPLv2.1 to GPLv2+ according to specific LGPL rules. The simple way is to list the files as LGPLv2.1  in the break-down, but add a note that you promote those to GPLv2+ in the same break-down. This makes it possible to exclude LGPLv2.1 from the License: tag. One less...
Comment 29 Nikos Mavrogiannopoulos 2013-11-12 09:19:10 EST
I have updated the srpm to combine the licenses and fix the build-aux license text (from GPLv3 -> GPLv2). I don't understand how I can add a note in the breakdown as you mention though. May I ask which fedora-review (application) check did you use to generate the attached license list?
Comment 30 Alec Leamas 2013-11-12 09:57:55 EST
The License: tag looks good. However, as stated in [2] you need to provide a breakdown describing what files have what license (writing this is a comment will become too much).

The breakdown is just a text file. You can use the attachment as a starting point, edit it in whatever way you want, call it e. g., LICENSE-BREAKDOWN and include it in the package. You will need to fix it manually, since the automatic license scanning reports that files have no license although they have (e. g., the ccan files). And you can add whatever other notes you want.

The tool is named fedora-review in the package fedora-review. If you are running on f20 you might need to use the latest git version as described in [1], there is a blocking bug in released package. It creates a review-directory, in there you find the file licensecheck.txt which is what I attached.

The command line I used is (using the git version):

 $  ./try-fedora-review -rn ocserv-0.2.1-3.fc20.src.rpm  -m fedora-rawhide-i386 -L pcllib  -DEPEL5

The pcllib directory contains local build for pcllib, yet not in rawhide, required for the build:

$ ls pcllib
pcllib-1.12-1.fc21.i686.rpm
pcllib-1.12-1.fc21.src.rpm
pcllib-debuginfo-1.12-1.fc21.i686.rpm
pcllib-devel-1.12-1.fc21.i686.rpm


[1] https://fedorahosted.org/FedoraReview/wiki/UseDevelopmentVersion
[2] http://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios
Comment 31 Nikos Mavrogiannopoulos 2013-11-12 11:07:51 EST
(In reply to Alec Leamas from comment #30)
> The License: tag looks good. However, as stated in [2] you need to provide a
> breakdown describing what files have what license (writing this is a comment
> will become too much).
> 
> The breakdown is just a text file. You can use the attachment as a starting
> point, edit it in whatever way you want, call it e. g., LICENSE-BREAKDOWN
> and include it in the package. You will need to fix it manually, since the
> automatic license scanning reports that files have no license although they
> have (e. g., the ccan files). And you can add whatever other notes you want.

It wasn't that easy adding a packager's file into %doc. I've managed by installing the PACKAGE-LICENSE into %{buildroot}/%{_defaultdocdir}/%{name}/, but doesn't look so good (even though it was taken from fedora packaging tricks).

Anyway, the current package contains the PACKAGE-LICENSE which clarifies the licenses used per-file.
Comment 32 Alec Leamas 2013-11-12 12:01:02 EST
Oops, you are such such a resourceful person I forget that this is your first package. A somewhat easier way is something like;

Source4: PACKAGE-LICENSING

%install
cp -a %{SOURCE4} PACKAGE-LICENSING

%files
%doc PACKAGE-LICENSING


There is really no need to install in a subdirectory, %doc fixes that. BTW, don't hesitate to ask here or on the list before spending too much time on such things!

Basically I think the license now looks good. Time for some nitpicks before doing the review:
- You need to claim the /etc/ocserv dir. Add a %dir %{_sysconfdir}/ocserv  to %files.
- With gnutls-devel at 3.1.16 for f19, what's the point requiring > 3.1.10?
- ccan files are bundled, we must wait for FPC (meeting Thursday 16.00 UTC). Add a note in the spec for now.
- The "normal" way is to run autoreconf -fi. Is there a particular reason why you omit  the -fi options?
- You forgot the changelog last time. While not that important during review, it's essential once committed. So I nag about it.

Some style remarks. These are preferences, don't consider them as review remarks:
- Some lines are really long, too long in my eyes. Comments are easy to
split, One can have multiple %doc lines. A long link is a long link, though.
- Having one BR on each line makes it clearer, sorting them is even better.
- One extra blank line before main sections (%description, %prep, %install, scriptlets, %files, %changelog) enhances readability.
Comment 33 Nikos Mavrogiannopoulos 2013-11-13 04:31:20 EST
Thanks, it should be better now. Let's see about the bundling exception.
Comment 34 Alec Leamas 2013-11-14 14:31:20 EST
The FPC didn't make a decision on it this week, they ran out of time. It should be on top of the agenda next week, though. Also, I chatted a little with them after the meeting and the ticket is seemingly OK, they probably have the info they need to make a decision. Of course, I can't be sure.
Comment 35 Nikos Mavrogiannopoulos 2013-11-15 04:42:52 EST
Let's wait then :)
Comment 36 Alec Leamas 2013-11-15 07:09:51 EST
Yup. In the meantime, you can close bug #1029002 as described in http://fedoraproject.org/wiki/New_package_process_for_existing_contributors.
Comment 37 Nikos Mavrogiannopoulos 2013-11-22 03:28:34 EST
Bummer. Still no final decision for the bundled CCAN. Another weekly delay...
Comment 38 Alec Leamas 2013-11-22 04:06:23 EST
Yup. Pasting the link for tracking: https://fedorahosted.org/fpc/ticket/364.
Comment 39 Nikos Mavrogiannopoulos 2013-12-05 14:59:22 EST
As I understand permission seem to be granted!
Comment 40 Alec Leamas 2013-12-05 15:57:33 EST
Indeed. If you just update the  Provides: + adds a link to the fpc ticket I will approve this. 

Please check your links, they seem broken right now.
Comment 41 Nikos Mavrogiannopoulos 2013-12-06 03:33:14 EST
The updated spec and SRPMs are at the following location:
http://people.redhat.com/nmavrogi/fedora/ocserv.spec
http://people.redhat.com/nmavrogi/fedora/ocserv-0.2.1-6.fc20.src.rpm
Comment 42 Alec Leamas 2013-12-06 04:47:39 EST
Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated

Issues
======

- Patches link to upstream bugs/comments/lists or are otherwise justified.
  The last patch need's a link or justification.


===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Provides: bundled(gnulib) in place as required.
[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]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "LGPL", "LGPL (v2.1 or later)", "BSD (3 clause) GPL (v2 or later)", "GPL
     (v2 or later)", "Unknown or generated". 17 files have unknown license.
     Detailed output of licensecheck in
     /home/mk/FedoraReview/1027770-ocserv/licensecheck.txt
[x]: If the package is under multiple licenses, the licensing breakdown must
     be documented in the spec.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[!]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf %{buildroot} present but not required
[x]: Sources contain only permissible code or content.
[!]: Each %files section contains %defattr if rpm < 4.4
     Note: %defattr present but not needed
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (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]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[x]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 174080 bytes in 11 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[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 requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: %config files are marked noreplace or the reason is justified.
[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]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: No %config files under /usr.
[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

===== SHOULD items =====

Generic:
[!]: Buildroot is not present
     Note: Buildroot: present but not needed
[!]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: %clean present but not required
[-]: 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).
[?]: 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.
[-]: %check is present and all tests pass.
[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]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Uses parallel make %{?_smp_mflags} macro.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[x]: Package should not use obsolete m4 macros
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: ocserv-0.2.1-6.fc21.x86_64.rpm
          ocserv-0.2.1-6.fc21.src.rpm
ocserv.x86_64: W: only-non-binary-in-usr-lib
ocserv.src:38: W: unversioned-explicit-provides bundled(gnulib)
ocserv.src:40: W: unversioned-explicit-provides bundled(bobjenkins-hash)
ocserv.src:40: W: unversioned-explicit-provides bundled(ccan-container_of)
ocserv.src:41: W: unversioned-explicit-provides bundled(ccan-htable)
ocserv.src:41: W: unversioned-explicit-provides bundled(ccan-list)
ocserv.src:42: W: unversioned-explicit-provides bundled(ccan-check_type)
ocserv.src:42: W: unversioned-explicit-provides bundled(ccan-build_assert)
2 packages and 0 specfiles checked; 0 errors, 8 warnings.


Rpmlint (installed packages)
----------------------------
# rpmlint ocserv
ocserv.x86_64: W: only-non-binary-in-usr-lib
1 packages and 0 specfiles checked; 0 errors, 1 warnings.
# echo 'rpmlint-done:'


Requires
--------
ocserv (rpmlib, GLIBC filtered):
    /bin/sh
    config(ocserv)
    iproute
    libc.so.6()(64bit)
    libcrypt.so.1()(64bit)
    libgnutls.so.28()(64bit)
    libgnutls.so.28(GNUTLS_1_4)(64bit)
    libgnutls.so.28(GNUTLS_2_10)(64bit)
    libgnutls.so.28(GNUTLS_2_12)(64bit)
    libgnutls.so.28(GNUTLS_3_0_0)(64bit)
    libgnutls.so.28(GNUTLS_3_1_0)(64bit)
    libhttp_parser.so.2()(64bit)
    libopts.so.25()(64bit)
    libpam.so.0()(64bit)
    libpam.so.0(LIBPAM_1.0)(64bit)
    libpcl.so.1()(64bit)
    libutil.so.1()(64bit)
    libwrap.so.0()(64bit)
    pam
    rtld(GNU_HASH)
    shadow-utils
    systemd



Provides
--------
ocserv:
    bundled(bobjenkins-hash)
    bundled(ccan-build_assert)
    bundled(ccan-check_type)
    bundled(ccan-container_of)
    bundled(ccan-htable)
    bundled(ccan-list)
    bundled(gnulib)
    config(ocserv)
    ocserv
    ocserv(x86-64)



Source checksums
----------------
ftp://ftp.infradead.org/pub/ocserv/ocserv-0.2.1.tar.xz :
  CHECKSUM(SHA256) this package     : f98a052319cf9fbef09c7733054520779f327cd3730773cd619fcd675cc2234b
  CHECKSUM(SHA256) upstream package : f98a052319cf9fbef09c7733054520779f327cd3730773cd619fcd675cc2234b


Generated by fedora-review 0.5.0 (928f2b2) last change: 2013-10-14
Command line :./try-fedora-review -b 1027770
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api, C/C++
Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, EPEL5, BATCH, DISTTAG
Comment 43 Alec Leamas 2013-12-06 04:50:08 EST
The last patch needs a justification. With this change:

*** Approved

End of bumpy road :)
Comment 44 Nikos Mavrogiannopoulos 2013-12-06 05:09:42 EST
Thank you!
Comment 45 Nikos Mavrogiannopoulos 2013-12-06 05:16:34 EST
New Package SCM Request
=======================
Package Name: ocserv
Short Description: OpenConnect server (ocserv) is an SSL VPN server. 
Owners: nmav
Branches: f19 f20
InitialCC:
Comment 46 Alec Leamas 2013-12-06 06:54:09 EST
You accidentally reset the fedora-review flag to '?'. It's the browser caches which causes this. It's a real PITA, but one need to be careful when updating a bug - the best is to clear the browses caches or simply restart it before updating. Resetting flag.
Comment 47 Alec Leamas 2013-12-06 06:56:03 EST
After saying that, I squashed the fedora-cvs-flag. Sorry. It's better if you reset it, though.
Comment 48 Alec Leamas 2013-12-06 06:59:25 EST
No, it actually seems ok, it just looked bad. Hate this.
Comment 49 Nikos Mavrogiannopoulos 2013-12-06 07:27:55 EST
New Package SCM Request
=======================
Package Name: ocserv
Short Description: OpenConnect server (ocserv) is an SSL VPN server. 
Owners: nmav
Branches: f19 f20
InitialCC:
Comment 50 Gwyn Ciesla 2013-12-06 08:07:13 EST
Git done (by process-git-requests).
Comment 51 Fedora Update System 2013-12-06 08:46:32 EST
ocserv-0.2.1-6.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/ocserv-0.2.1-6.fc19
Comment 52 Fedora Update System 2013-12-06 08:47:30 EST
ocserv-0.2.1-6.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/ocserv-0.2.1-6.fc20
Comment 53 Fedora Update System 2013-12-14 22:28:52 EST
ocserv-0.2.1-6.fc20 has been pushed to the Fedora 20 stable repository.
Comment 54 Fedora Update System 2013-12-14 22:33:09 EST
ocserv-0.2.1-6.fc19 has been pushed to the Fedora 19 stable repository.
Comment 55 Nikos Mavrogiannopoulos 2014-01-20 07:03:36 EST
Package Change Request
======================
Package Name: ocserv
New Branches: el6 epel7
Owners: nmav
Comment 56 Gwyn Ciesla 2014-01-21 09:04:13 EST
Git done (by process-git-requests).

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