Bug 1054941

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

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.

And about EL5. No, I don't plan to target that.

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

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.

SPEC URL: http://mihkel.fedorapeople.org/SPECS/firefox-esteid-plugin.spec
SPRM URL: https://mihkel.fedorapeople.org/SRPMS/firefox-esteid-plugin-3.8.0.1115-4.fc20.src.rpm

Changes made.

* 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

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:

Git done (by process-git-requests).