Bug 1054938 (esteidpkcs11loader)

There already exists a package with similar functionality: esteid-browser-plugin. Are there reasons to package also eesti.ee version?

Issues found:

1) %clean section can be removed

2) license should be "LGPLv2+"

3) missing license file in %doc

4) description should end with dot :-)

5) requires shouldn't be arch-specific, esteidpkcs11loader is noarch package

6) there should be dependency on mozilla-filesystem (because of /usr/share/mozilla and /usr/share/mozilla/extensions directories)

SPEC file: http://mihkel.fedorapeople.org/SPECS/esteidpkcs11loader.spec
SRPM: http://mihkel.fedorapeople.org/SRPMS/esteidpkcs11loader-3.8.0.1052-2.fc20.src.rpm

(In reply to František Dvořák from comment #1)
> There already exists a package with similar functionality:
> esteid-browser-plugin. Are there reasons to package also eesti.ee version?

Version that is now in Fedora repos is based on old codebase. It was developed by another company, but due to some "confusion", contract with this company was ended by RIA and another company took over the development (AS Sertifitseerimiskeskus). Estonian ID-card software that is currently in Fedora is too old and full of bugs, so I'm drying to get this new version in, because this software is essential to Estonians and linux users in Estonia are basically choosing distribution based on whether it has option to install it or not. As a Fedora user I'm trying to make Fedora more appealing to Estonian linux users and of course more convenient for myself :)


> Issues found:
> 
> 1) %clean section can be removed

Removed

> 
> 2) license should be "LGPLv2+"

Done

> 3) missing license file in %doc

LICENSE.LGPL added to docs

> 4) description should end with dot :-)

Done

> 5) requires shouldn't be arch-specific, esteidpkcs11loader is noarch package

Ok

> 6) there should be dependency on mozilla-filesystem (because of
> /usr/share/mozilla and /usr/share/mozilla/extensions directories)

Dependency on mozilla-filesystem added



I also changed URL to http://www.ria.ee since it is specified in other spec files and also in spec file from tarball.

(In reply to Mihkel Vain from comment #2)
> 
> Version that is now in Fedora repos is based on old codebase. It was
> developed by another company, but due to some "confusion", contract with
> this company was ended by RIA and another company took over the development
> (AS Sertifitseerimiskeskus). Estonian ID-card software that is currently in
> Fedora is too old and full of bugs

OK. You can consider adding that information for users to description of the plugin package in #1054941. (inserting words "new official" or something like that? ;-))


Other stuff to discuss (I'm sorry I haven't noticed and brought it before):

1) package name

This command:
  repoquery -f '*ec8030f7-c20a-464f-9b0e-13a3a9e97384*'

... returns all packages with "mozilla-" prefix. This package could be renamed to "mozilla-esteidpkcs11loader". Any opinion?


2) enhancement for other browsers

In esteid-browser-plugin and other packages is nice way how to support other browsers: http://pkgs.fedoraproject.org/cgit/esteid-browser-plugin.git/plain/esteid-browser-plugin.spec . Of course that can be done only if the other browser are supported.


And would you willing to review this package?: https://bugzilla.redhat.com/show_bug.cgi?id=1052852 . It is quite trivial I guess, one specific thing there is custom configure script written in perl (=more dependencies), but that was reviewed already by several reviewers in other packages.

I'm struggling to decide what should I call this package and #1054941. #1054941 is called firefox-esteid right now because as upstream told me and as I wrote in #1054941 comment:

""
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.
""

Maybe I should rename #1054941 to mozilla-* and this one as well, because there appears to be no firefox-* packages in repos right now? But then again upstream only supports firefox... 

I could really use some ideas right now :)

What would be the least surprise to the user?

One approach could be not much thinking about it (mozilla-* here, although not strictly right according to the upstream support; and keep the "firefox" in the name of the broswer plugin itself #1054941 - it is part of the upstream name anyway).

Another approach: only firefox is officially supported, so using prefix "firefox-" for both packages (firefox just happen to use mozilla paths). And slight different package name will correspond to less broad of browsers supported also in this package.

Another option is to use same naming as its currently used in Fedora repos. Meaning:

esteid-browser-plugin (digital signing browser plugin) instead of firefox-esteid (#1054941) 
and
mozilla-esteid (Mozilla extension) instead of esteidpkcs11loader (#1054938)

But since those packages are already in Fedora repos now I just don't know how to act :)
In this case review should not be required any more. Just upgrading existing packages, but since those are my first set of packages, I'd still like that someone would check those spec files.

Kalev, I see you are getting those emails as well. What is your opinion? Is it ok to go the same path with #1054938 and #1054941 as we did with qdigidoc, qesteidutil, libdigidoc and libdigidocpp. Eg I just upgrade esteid-browser-plugin and mozilla-esteid in rawhide and you check specs :D

If I understand correctly, this is different software than the current plugin in Fedora?

I think this would be the proper way to do it:

1) get new set of packages to Fedora, for example:
  - mozilla-esteidpkcs11loader + esteid-browser-plugin2
  - or something like: firefox-esteidpkcs11loader + firefox-esteid-plugin

Note, mozilla-esteid is currently subpackage of esteid-browser-plugin, so creating new package (+review) is needed anyway for this one package.

2) (optionally) use Provides and Obsoletes for transparent update (http://fedoraproject.org/wiki/Packaging:Guidelines#Renaming.2FReplacing_Existing_Packages):

  - browser plugin package:
  Provides: esteid-browser-plugin%{?_isa} = %{version}-%{release}
  Obsoletes: esteid-browser-plugin < 1.3.4

  - this package:
  Provides: mozilla-esteid = %{version}-%{release}
  Obsoletes:  mozilla-esteid < 1.3.4

3) (optionally) retire old package set (http://fedoraproject.org/wiki/How_to_remove_a_package_at_end_of_life)


Thanks to 1) users will really see during update there is major change. Automatic transition to new version would be possible thanks to 2), if desired.

Spec URL: http://mihkel.fedorapeople.org/SPECS/firefox-esteidpkcs11loader.spec
SRPM URL: http://mihkel.fedorapeople.org/SRPMS/firefox-esteidpkcs11loader-3.8.0.1052-3.fc20.src.rpm

I've decided to go with firefox-* prefix. -- firefox-esteidpkcs11loader (this one) and firefox-esteid-plugin (#1054941)

I also added Provides and Obsoletes lines except I used 3.8.0 version number instead of 1.3.4... just in case :P

And I guess this package should be released later, together with upcoming firefox-esteid-plugin? (or let the Provides+Obsoletes commented out for now?)

It looks good to me. Package approved!



Package Review
==============

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



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

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. No licenses
     found. Please check the source files for licenses manually.
[x]: Package does not own files or directories owned by other packages.
     Note: Dirs in package are owned also by:
     /usr/share/mozilla/extensions/{ec8030f7-c20a-464f-9b0e-
     13a3a9e97384}(firefox, esteidpkcs11loader),
     /usr/share/mozilla/extensions/{ec8030f7-c20a-464f-9b0e-
     13a3a9e97384}/{aa84ce40-4253-a00a-
     8cd6-0800200f9a66}/chrome(esteidpkcs11loader),
     /usr/share/mozilla/extensions/{ec8030f7-c20a-464f-9b0e-
     13a3a9e97384}/{aa84ce40-4253-a00a-
     8cd6-0800200f9a66}/chrome/content(esteidpkcs11loader),
     /usr/share/mozilla/extensions/{ec8030f7-c20a-464f-9b0e-
     13a3a9e97384}/{aa84ce40-4253-a00a-8cd6-0800200f9a66}(esteidpkcs11loader)

That's OK. The package is addon, but it can be installed separatelly.

[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: 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.
[x]: 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.
[-]: Package contains systemd file(s) if in need.
[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 30720 bytes in 2 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: No rpmlint messages.
[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]: 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]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[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]: 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:
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[-]: 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.
[x]: 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]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[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: No rpmlint messages.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: firefox-esteidpkcs11loader-3.8.0.1052-3.fc21.noarch.rpm
          firefox-esteidpkcs11loader-3.8.0.1052-3.fc21.src.rpm
2 packages and 0 specfiles checked; 0 errors, 0 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint firefox-esteidpkcs11loader
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
# echo 'rpmlint-done:'



Requires
--------
firefox-esteidpkcs11loader (rpmlib, GLIBC filtered):
    esteidcerts
    mozilla-filesystem
    opensc
    pcsc-lite



Provides
--------
firefox-esteidpkcs11loader:
    firefox-esteidpkcs11loader
    mozilla-esteid



Source checksums
----------------
https://installer.id.ee/media/sources/esteidpkcs11loader-3.8.0.1052.tar.gz :
  CHECKSUM(SHA256) this package     : 331d7fb59554a4cf7c3cce8a263e5248476d994e3124f0b5adc06ba3cad1b091
  CHECKSUM(SHA256) upstream package : 331d7fb59554a4cf7c3cce8a263e5248476d994e3124f0b5adc06ba3cad1b091


Generated by fedora-review 0.5.1 (bb9bf27) last change: 2013-12-13
Command line :/usr/bin/fedora-review -m fedora-rawhide-x86_64 -b 1054938
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api
Disabled plugins: Java, C/C++, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, EPEL5, BATCH, DISTTAG

(In reply to František Dvořák from comment #9)
> And I guess this package should be released later, together with upcoming
> firefox-esteid-plugin? (or let the Provides+Obsoletes commented out for now?)
> 

Yes. I will not request a build for rawhide before firefox-esteid-plugin is in too.


New Package SCM Request
=======================
Package Name: firefox-esteidpkcs11loader
Short Description: Loads PKCS#11 module for web authentication with smart cards.
Owners: mihkel
Branches: f19 f20
InitialCC:

WARNING: Requested package name firefox-esteidpkcs11loader doesn't match bug
summary esteidpkcs11loader 

Please correct.

New Package SCM Request
=======================
Package Name: firefox-esteidpkcs11loader
Short Description: Loads PKCS#11 module for web authentication with smart cards.
Owners: mihkel
Branches: f19 f20
InitialCC:

Git done (by process-git-requests).