Bug 1054941 - Review Request: firefox-esteid-plugin - EstEID browser plugin for digital signing
Review Request: firefox-esteid-plugin - EstEID browser plugin for digital sig...
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Lubomir Rintel
Fedora Extras Quality Assurance
:
Depends On: esteidpkcs11loader
Blocks: 1092935
  Show dependency treegraph
 
Reported: 2014-01-17 14:05 EST by Mihkel Vain
Modified: 2015-07-26 15:01 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2015-07-26 15:01:40 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
lkundrak: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Mihkel Vain 2014-01-17 14:05:11 EST
Spec URL: http://mihkel.fedorapeople.org/SPECS/esteidfirefoxplugin.spec
SRPM URL: http://mihkel.fedorapeople.org/SRPMS/esteidfirefoxplugin-3.8.0.1115-1.fc20.src.rpm
Description: EstEID Firefox plugin
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 third package.

koji task link: http://koji.fedoraproject.org/koji/taskinfo?taskID=6421481
Comment 1 Michael Schwendt 2014-01-28 15:33:26 EST
A brief look only:


> Name:           esteidfirefoxplugin

The modern package Naming Guidelines suggest adding a firefox- prefix, since this is a plugin that extends Firefox.

  https://fedoraproject.org/wiki/Packaging:Guidelines#Naming
   -> https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Addon_Packages_.28General.29


> Summary:        EstEID browser plugin for digital signing

Is the plugin specific to Firefox? Or is only stored in Firefox's plugin path but based on the old Netscape Plugin API and then would be compatible with any browser that supports the NPAPI?

If the summary mentioned Firefox it would be more clear, e.g.

  Summary: Firefox plugin for signing with Estonian ID cards


> License:        LGPLv2

https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#.22or_later_version.22_licenses


> Requires:       opensc
> Requires:       pcsc-lite
> Requires:       esteidpkcs11loader

It's good practise to add comments to explicit Requires and explain what exactly is needed. A dependency on a package name could be broken easily, if a file moves into a different (sub-)package, for example.


> %description
> EstEID Firefox plugin

That's way too short and too lazy for a package description.


> make plugin %{?_smp_mflags}

https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags

Some packagers like doing something similar to

  %configure || :
  make …

to have the %configure macro export Fedora's CFLAGS and LDFLAGS, if the used Makefile picks them up.


> %install
> rm -rf %{buildroot}

If you want to target EL5, too, better be explicit about that during package review because of:
https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag


> %defattr(-,root,root,-)

Not necessary anymore for any of the active dist releases including EL5.


> %doc README.txt 

https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text


> %{_libdir}/mozilla/plugins/npesteid-firefox-plugin.so

Please review the File and Directory Ownership section in the guidelines, since without a dependency on firefox, there would be "unowned" directories.
https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership
Comment 2 Mihkel Vain 2014-01-31 10:49:31 EST
Spec URL: http://mihkel.fedorapeople.org/SPECS/firefox-esteid.spec
SRPM URL: http://mihkel.fedorapeople.org/SRPMS/firefox-esteid-3.8.0.1115-1.fc20.src.rpm

Koji task: http://koji.fedoraproject.org/koji/taskinfo?taskID=6475735

Hi and thank you for your notes. 

(In reply to Michael Schwendt from comment #1)
> > Summary:        EstEID browser plugin for digital signing
> 
> Is the plugin specific to Firefox? Or is only stored in Firefox's plugin
> path but based on the old Netscape Plugin API and then would be compatible
> with any browser that supports the NPAPI?
> 
> If the summary mentioned Firefox it would be more clear, e.g.
> 
>   Summary: Firefox plugin for signing with Estonian ID cards
> 

This plugin is not strictly Firefox specific (as upstream just told me) and it probably works on other NPAPI browsers too, but officially upstream supports only Firefox.


> > Requires:       opensc
> > Requires:       pcsc-lite
> > Requires:       esteidpkcs11loader
> 
> It's good practise to add comments to explicit Requires and explain what
> exactly is needed. A dependency on a package name could be broken easily, if
> a file moves into a different (sub-)package, for example.
>

I added some comments best to my knowledge and removed unnecessary dependencies. However I'm not sure how informative my comments are :) 
 

> 
> > %{_libdir}/mozilla/plugins/npesteid-firefox-plugin.so
> 
> Please review the File and Directory Ownership section in the guidelines,
> since without a dependency on firefox, there would be "unowned" directories.
> https://fedoraproject.org/wiki/Packaging:
> Guidelines#File_and_Directory_Ownership

I made a requirement on mozilla-filesystem
As I said this plugin probably works on other NPAPI supported browsers too, therefore I made it require mozilla-filesystem not firefox.
Comment 3 Mihkel Vain 2014-01-31 10:53:31 EST
And about EL5. No, I don't plan to target that.
Comment 4 Mihkel Vain 2014-06-22 10:15:46 EDT
I renamed package to firefox-esteid-plugin.

SPEC URL: http://mihkel.fedorapeople.org/SPECS/firefox-esteid-plugin.spec
SPRM URL: http://mihkel.fedorapeople.org/SRPMS/firefox-esteid-plugin-3.8.0.1115-3.fc20.src.rpm
Comment 5 Lubomir Rintel 2014-09-29 04:41:53 EDT
1.) Compiler flags still not good

> %configure || :

This does not work; Makefile uses C_FLAGS variable. Even if it worked, it's ugly and relies implementation details which can change. This might be a better idea:

make plugin %{?_smp_mflags} C_FLAGS='%{optflags} -std=gnu99'

2.) Unnecessary make install

> make install DESTDIR=%{buildroot}

Just drop this. Apart from that it installs nothing, it is broken and does an unnecessary rebuild, which is unacceptable at %install phase.
Comment 7 Lubomir Rintel 2014-09-30 10:48:01 EDT
* Package named correctly
* Latest version packaged
* Version & Release tags correct
* Proper compiler flags used
* RPMlint happy
* SPEC file clean and legible
* License tag correct
* License text included in package
* License good for fedora
* Builds fine in mock
* Requires/Provides fine
* Filelist sane

I won't block the review with this, but there's one more thing to do before you import the package:

Please remove the extra blank line in the beginning of the description. It is considered to be actual part of the description and looks ugly on output of rpm -qi and possibly other tools.

The package is APPROVED
Comment 8 Mihkel Vain 2014-09-30 13:33:30 EDT
New Package SCM Request
=======================
Package Name: firefox-esteid-plugin
Short Description: EstEID browser plugin for digital signing
Upstream URL: https://installer.id.ee/media/sources/
Owners: mihkel
Branches: f19 f20 f21
InitialCC:
Comment 9 Jon Ciesla 2014-09-30 15:59:07 EDT
Git done (by process-git-requests).

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