Bug 1054933 - (esteidcerts) Review Request: esteidcerts - Estonian ID card certificates
Review Request: esteidcerts - Estonian ID card certificates
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Rex Dieter
Fedora Extras Quality Assurance
:
Depends On:
Blocks: esteidpkcs11loader
  Show dependency treegraph
 
Reported: 2014-01-17 14:01 EST by Mihkel Vain
Modified: 2014-04-17 02:01 EDT (History)
3 users (show)

See Also:
Fixed In Version: esteidcerts-3.8.0.9128-1.fc20
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-04-17 02:01:11 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
rdieter: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Mihkel Vain 2014-01-17 14:01:38 EST
Spec URL: http://mihkel.fedorapeople.org/SPECS/esteidcerts.spec
SRPM URL: http://mihkel.fedorapeople.org/SRPMS/esteidcerts-3.8.0.9128-1.fc20.src.rpm
Description: Estonian ID card root, intermediate and OCSP certificates
Fedora Account System Username: mihkel

I'm looking for a sponsor for a suite of packages that are needed to use Estonian ID card software in Fedora. This is the first package and also the first packaging job for me :)

koji task link: http://koji.fedoraproject.org/koji/taskinfo?taskID=6419147
Comment 1 Mihkel Vain 2014-01-18 11:05:09 EST
I updated spec file. Moved TEST certs to a separate devel package. Spec and srpm urls are same, but just in case here is updated koji task:  http://koji.fedoraproject.org/koji/taskinfo?taskID=6423902
Comment 2 Rex Dieter 2014-02-17 11:37:56 EST
I'd be happy to help review this...
Comment 3 Rex Dieter 2014-02-17 12:01:32 EST
Some initial comments:

rpmlint is clean:
$ rpmlint *.rpm */*.rpm
3 packages and 0 specfiles checked; 0 errors, 0 warnings


sources verified:
92b79074e4f6db85453f561f9d9ab35f  esteidcerts-3.8.0.9128.tar.gz


1.  SHOULD remove deprecated .spec items
Group: tags, %defattr

these are deprecated and unused, see also:
https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Group_tag
and
https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#File_Permissions
respectively


2. -devel subpkg name SHOULD be named something else
This does seem really a Development package in the
https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Devel_Packages
sense to me.  I'd suggest naming it something else, like -test


3.  MUST fix directory ownership
Nothing owns
%{_datadir}/esteid
I'd suggest adding to base pkg:
%dir %{_datadir}/esteid

and -devel (or whatever it ends up called), misses anything owning parent dirs:
%{_datadir}/esteid
%{_datadir}/esteid/certs
I'd suggest adding a dependency on the base pkg:
Requires: %{name} = %{version}-%{release}
or add here:
%dir %{_datadir}/esteid
%dir %{_datadir}/esteid/certs

See also:
https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#File_and_Directory_Ownership


4.  licensing cannot be verified.  the source archive doesn't seem to mention licensing anywhere.  where do you get
License: LGPLv2
from?
Comment 4 Mihkel Vain 2014-02-17 14:48:04 EST
Spec URL: http://mihkel.fedorapeople.org/SPECS/esteidcerts.spec
SRPM URL: http://mihkel.fedorapeople.org/SRPMS/esteidcerts-3.8.0.9128-1.fc20.src.rpm
Koji task: http://koji.fedoraproject.org/koji/taskinfo?taskID=6540301

Hi.

Thanks for you time and review. I hope I got everything sorted out the way you suggested.


(In reply to Rex Dieter from comment #3)
 
> 1.  SHOULD remove deprecated .spec items
> Group: tags, %defattr

I remove Group lines and %defattr lines

I'll do the same for other spec files also, but not today.

 
> 2. -devel subpkg name SHOULD be named something else
> This does seem really a Development package in the
> https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/
> Guidelines#Devel_Packages
> sense to me.  I'd suggest naming it something else, like -test

-test it is

> 3.  MUST fix directory ownership
> Nothing owns
> %{_datadir}/esteid
> I'd suggest adding to base pkg:
> %dir %{_datadir}/esteid
> 
> and -devel (or whatever it ends up called), misses anything owning parent
> dirs:
> %{_datadir}/esteid
> %{_datadir}/esteid/certs
> I'd suggest adding a dependency on the base pkg:
> Requires: %{name} = %{version}-%{release}
> or add here:
> %dir %{_datadir}/esteid
> %dir %{_datadir}/esteid/certs
> 

I added 
%dir %{_datadir}/esteid
to %files section and a
Requires:       %{name} = %{version}-%{release}
line

> 
> 4.  licensing cannot be verified.  the source archive doesn't seem to
> mention licensing anywhere.  where do you get
> License: LGPLv2
> from?

My mistake. According to esteidcerts.spec file in source tarball, these certs are public domain. Although they should include a LICENSE file in tarball. I'll mention that to upstream when (if) I'll finish packaging these packages and then they probably include that file in next release.
Comment 5 Rex Dieter 2014-02-18 09:55:45 EST
Item 3 was fixed incorrectly, you put
Requires:       %{name} = %{version}-%{release}
in the main pkg (it requires itself? :) ), when it should be in the %package test section

5. SHOULD remove from %install:
rm -rf %{buildroot}

This is another deprecation-like item, that rpm handles automatically these days.
Comment 6 Mihkel Vain 2014-02-18 11:06:21 EST
Spec file: http://mihkel.fedorapeople.org/SPECS/esteidcerts.spec

(In reply to Rex Dieter from comment #5)
> Item 3 was fixed incorrectly, you put
> Requires:       %{name} = %{version}-%{release}
> in the main pkg (it requires itself? :) ), when it should be in the %package
> test section
> 

It should be in right place now.


> 5. SHOULD remove from %install:
> rm -rf %{buildroot}
> 
> This is another deprecation-like item, that rpm handles automatically these
> days.

I removed that line.
Comment 7 Mihkel Vain 2014-02-25 04:05:57 EST
Spec file: http://mihkel.fedorapeople.org/SPECS/esteidcerts.spec

Just a little update. I took gnome-music.spec as a example of what other lines are deprecated and removed 
mkdir -p %{buildroot}
as well.

Also I removed Group: tags, %defattr and rm -rf %{buildroot} (from install section) from other spec files as well.
http://mihkel.fedorapeople.org/SPECS/

There are in total of 7 spec files (I've made only 3 bugzilla reports so far) plus I'd like to create a metapackage or a software group which installs all required packages for Estonian national id card.
Comment 8 Mihkel Vain 2014-04-02 08:14:02 EDT
Hi Rex.

Its been little over a month. Are you still interested in reviewing my packages?

Meanwhile I've set up a copr build:
http://copr.fedoraproject.org/coprs/mihkel/esteid/
and just for fun an epel build too:
http://copr.fedoraproject.org/coprs/mihkel/esteid-epel/

I'm still not sure weather I should target EL 6 and 7, because AFAIR in el7 I needed to build additional packages that were not currently available in epel repo and in el6 I needed to use newer compiler and probably some other hacks too.

Anyway it would be nice to hear what are your plans regarding my packages.
Comment 9 Rex Dieter 2014-04-02 09:06:35 EDT
Thanks for the reminder, I'll pick things up today.
Comment 10 Rex Dieter 2014-04-02 11:37:16 EDT
Technically, things look good.

My only last concern is licensing.  In particular, I don't see anything in the tarball to support that these items are in the Public domain.

I tried looking around a bit on http://www.id.ee/ and https://installer.id.ee , but my not knowing the language means I didn't get very far.

Though looking over 
https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing

helps:

"Being in the public domain is not a license; rather, it means the material is not copyrighted and no license is needed."

which I think is satisfied in this case.


APPROVED
Comment 11 Rex Dieter 2014-04-02 11:39:51 EDT
OK, I just sponsored you in FAS too, welcome to Fedora.

You can now continue on the process at this step:
https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner

Please let me know if you have any other questions or concerns.  (here, email, or irc, #fedora-devel are all good ways to get me or others to help)
Comment 12 Mihkel Vain 2014-04-02 14:22:49 EDT
Thanks. I guess the next thing to do is to request a repo for esteidcerts.

New Package SCM Request
=======================
Package Name: esteidcerts
Short Description: Estonian ID-card certificates
Owners: mihkel
Branches: f19 f20
InitialCC:
Comment 13 Gwyn Ciesla 2014-04-02 15:03:04 EDT
Git done (by process-git-requests).
Comment 14 Mihkel Vain 2014-04-03 08:39:33 EDT
Okay. I've added this package to SCM system and I think this one is ready. Should I close this bug?

Also. Rex are you interested in reviewing my other packages as well (6 packages to go plus one metapackage)?
Next one would be esteidpkcs11loader:
https://bugzilla.redhat.com/show_bug.cgi?id=1054938
Comment 15 Rex Dieter 2014-04-07 08:47:13 EDT
Yes, this can be closed or let bodhi autoclose it when/if you submit package updates for f19/f20

I could possibly do more reviews as time allows, but not sure when that will happen.
Comment 16 Fedora Update System 2014-04-07 12:43:54 EDT
esteidcerts-3.8.0.9128-1.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/esteidcerts-3.8.0.9128-1.fc19
Comment 17 Fedora Update System 2014-04-07 12:45:07 EDT
esteidcerts-3.8.0.9128-1.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/esteidcerts-3.8.0.9128-1.fc20
Comment 18 Fedora Update System 2014-04-08 20:54:48 EDT
esteidcerts-3.8.0.9128-1.fc19 has been pushed to the Fedora 19 testing repository.
Comment 19 Fedora Update System 2014-04-17 02:01:11 EDT
esteidcerts-3.8.0.9128-1.fc19 has been pushed to the Fedora 19 stable repository.
Comment 20 Fedora Update System 2014-04-17 02:01:44 EDT
esteidcerts-3.8.0.9128-1.fc20 has been pushed to the Fedora 20 stable repository.

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