Bug 463266 - (globalplatform) Review Request: globalplatform - Access OpenPlatform and GlobalPlatform smart cards library
Review Request: globalplatform - Access OpenPlatform and GlobalPlatform smart...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 473775
  Show dependency treegraph
 
Reported: 2008-09-22 15:04 EDT by François Kooman
Modified: 2008-12-10 10:05 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-12-10 10:05:00 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
Patch to provide globalplatform.pc (1.17 KB, patch)
2008-10-05 09:16 EDT, Mamoru TASAKA
no flags Details | Diff
Patch to build shared library with libtool 2.2 (351 bytes, patch)
2008-12-03 12:57 EST, Mamoru TASAKA
no flags Details | Diff

  None (edit)
Description François Kooman 2008-09-22 15:04:55 EDT
Spec URL: http://users.tuxed.net/fkooman/rpmbuild/SPECS/globalplatform.spec
SRPM URL: http://users.tuxed.net/fkooman/rpmbuild/SRPMS/globalplatform-5.0.0-1.fc9.src.rpm
Description: This is a library for providing access to OpenPlatform 2.0.1 and
GlobalPlatform 2.1.1 conforming smart cards.

(It will be used by GlobalPlatform shell (gpshell) development tool)
Comment 1 François Kooman 2008-09-22 15:09:19 EDT
This is my first package, I need a sponsor :)
Comment 2 Mamoru TASAKA 2008-10-03 12:04:13 EDT
Some notes:

* License
  - As far as I check the source codes the license tag
    should be "LGPLv3+".

* SourceURL
  - For tarballs hosted by sourceforge, please refer to:
    https://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net

* Requires
  - GlobalPlatform.h contains:
------------------------------------------------
    44  #include <winscard.h>
    45  #include "unicode.h"
    46  #include <stdio.h>
------------------------------------------------
    So -devel package should have "Requires: pcsc-lite-devel"

  ! By the way it may be preferable that you provide pkgconfig
    file for globalplatform-devel package because winscard.h
    is not under %_includedir but under %_includedir/PCSC so
    to use header files in globalplatform-devel you have
    to add $(pkg-config --cflags libpcsclite) to CFLAGS.

* Documents
  - Please consider to add the following file(s) to %doc:
------------------------------------------------
AUTHORS
------------------------------------------------

* Timestamps
  - Please consider to use
------------------------------------------------
make install DESTDIR=%{buildroot} CPPROG="cp -p"
------------------------------------------------
    to keep timestamps on installed header files?
    This method usually works for Makefiles using install-sh
    when installing files.

* %files entry
  - build log shows:
------------------------------------------------
   521  Processing files: globalplatform-devel-5.0.0-1.fc10
   522  warning: File listed twice: /usr/include/GlobalPlatform/GlobalPlatform.h
   523  warning: File listed twice: /usr/include/GlobalPlatform/unicode.h
------------------------------------------------
    %files entry
------------------------------------------------
%files
%{_includedir}/GlobalPlatform/
------------------------------------------------
     contains the directory %_includedir/GlobalPlatform itself and
     all files/directories/etc under the directory, so the following
------------------------------------------------
%{_includedir}/GlobalPlatform/*
------------------------------------------------
      entry is not needed. If you want to list only the directory
      %_includedir/GlobalPlatform, use
------------------------------------------------
%dir %{_includedir}/GlobalPlatform/
------------------------------------------------
Comment 3 François Kooman 2008-10-03 17:54:50 EDT
Updated SPEC and SRPM:

Spec URL: http://users.tuxed.net/fkooman/rpmbuild/SPECS/globalplatform.spec
SRPM URL:
http://users.tuxed.net/fkooman/rpmbuild/SRPMS/globalplatform-5.0.0-2.fc10.src.rpm

I've addressed all issues (see changelog in spec and spec itself) except the pkg-config one as I'm not quite sure how to proceed with this.

I was digging through the configure output and checking the config.log file to see what is going on there. The globalplatform configure script seems to use pkg-config to detect the location of pcsc-libs:

configure:20726: $PKG_CONFIG --exists --print-errors "libpcsclite >= 1.2.9-beta7"
configure:20729: $? = 0
configure:20781: result: yes
configure:20815: checking winscard.h usability
configure:20832: gcc -c -g -O2 -pthread -I/usr/include/PCSC    conftest.c >&5
configure:20838: $? = 0
configure:20852: result: yes
configure:20856: checking winscard.h presence
configure:20871: gcc -E  conftest.c
conftest.c:27:22: error: winscard.h: No such file or directory
configure:20877: $? = 1
configure: failed program was:
| /* confdefs.h.  */
| #define PACKAGE_NAME "GlobalPlatform"
| #define PACKAGE_TARNAME "globalplatform"
| #define PACKAGE_VERSION "5.0.0"
| #define PACKAGE_STRING "GlobalPlatform 5.0.0"
| #define PACKAGE_BUGREPORT "k_o_@users.sourceforge.net"
| #define PACKAGE "globalplatform"
| #define VERSION "5.0.0"
| #define STDC_HEADERS 1
    <more>
| #define HAVE_MEMCPY 1
| #define HAVE_VSYSLOG 1
| /* end confdefs.h.  */
| #include <winscard.h>
configure:20891: result: no
configure:20897: WARNING: winscard.h: accepted by the compiler, rejected by the preprocessor!
configure:20899: WARNING: winscard.h: proceeding with the compiler's result
configure:20924: checking for winscard.h
configure:20931: result: yes
configure:20958: checking for SCardEstablishContext in -lpcsclite
configure:20993: gcc -o conftest -g -O2   -lpcsclite   conftest.c -lpcsclite   >&5
configure:20999: $? = 0
configure:21017: result: yes

So it already seems that globalplatform needs pkgconfig as a dependency as otherwise it would never find the the winscard.h... Because it succeeds somehow in finding it can the rest of the configure stuff be ignored? It seems a bit "dirty"...

GPshell (which used globalplatform) has the same pkg-config stuff for libpcsclite as globalplatform so that solves the problem for GPshell in that way...

So, I'm not really sure what the best way to go would be...

Thanks for your time and review!
Comment 4 Mamoru TASAKA 2008-10-05 09:16:24 EDT
Created attachment 319494 [details]
Patch to provide globalplatform.pc

(In reply to comment #3)
> I've addressed all issues (see changelog in spec and spec itself) except the
> pkg-config one as I'm not quite sure how to proceed with this.
> 
> I was digging through the configure output and checking the config.log file to
> see what is going on there. The globalplatform configure script seems to use
> pkg-config to detect the location of pcsc-libs:

Yes, and it is not what I mentioned before. What I said is that it is better
that you create globalplatform.pc(.in) and install it under %_libdir/pkgconfig
so that other applications using globalplatform-devel package will find
the needed CFLAGS easily. How do you think?

Note: When applying this patch, your spec file should be fixed like:
-------------------------------------------------------------------
Name: globalplatform
Version: 5.0.0
......
BuildRequires: openssl-devel >= 0.9.7e
BuildRequires: zlib-devel >= 1.2.3
BuildRequires: pcsc-lite-devel
BuildRequires: automake
BuildRequires: libtool
......
%prep
%setup -q
%patch0 -p1 -b .add

aclocal
autoconf
automake
touch -r Makefile.in configure aclocal.m4

%build
........
%files devel
%defattr(-,root,root,-)
%{_libdir}/*.so
%{_includedir}/GlobalPlatform/
%{_libdir}/pkgconfig/*.pc
-------------------------------------------------
Comment 5 Mamoru TASAKA 2008-10-05 09:18:53 EDT
By the way:
-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package collection review requests which are waiting for someone to
review can be checked on:
http://fedoraproject.org/PackageReviewStatus/NEW.html
(NOTE: please don't choose "Merge Review")


Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------
Comment 6 François Kooman 2008-10-09 19:09:25 EDT
I updated them again:

Spec URL: http://users.tuxed.net/fkooman/rpmbuild/SPECS/globalplatform.spec
SRPM URL:
http://users.tuxed.net/fkooman/rpmbuild/SRPMS/globalplatform-5.0.0-3.fc10.src.rpm

A few more questions:

- It seems to me that pkg-config --cflags globalplatform should not just return "-pthread -I/usr/include/PCSC" but something like "-pthread -I/usr/include/GlobalPlatform -I/usr/include/PCSC" right? So maybe
   includedir=@includedir@
should become:
   includedir=@includedir@/GlobalPlatform 
instead?

- Does it make sense to submit the inclusion of the pkgconfig file upstream (as the requirement that every patch has a bug report upstream)?

- How about the openssl-devel dependency now that we are moving to NSS? Is this still allowed? I didn't yet look into how comprehensive the use of openssl is...
Comment 7 Mamoru TASAKA 2008-10-10 11:25:18 EDT
For -3:

(In reply to comment #6)
> A few more questions:
> 
> - It seems to me that pkg-config --cflags globalplatform should not just return
> "-pthread -I/usr/include/PCSC" but something like "-pthread
> -I/usr/include/GlobalPlatform -I/usr/include/PCSC" right? So maybe
>    includedir=@includedir@
> should become:
>    includedir=@includedir@/GlobalPlatform 
> instead?
  - Oh, I overlooked this point. The correct one is
---------------------------------------------------------------
Cflags: -I${includedir}/GlobalPlatform
---------------------------------------------------------------

> - Does it make sense to submit the inclusion of the pkgconfig file upstream (as
> the requirement that every patch has a bug report upstream)?
  - I think shipping pkgconfig file in the tarball is preferable (for
    this package) and I would appreciate it if you submit to upstream.
  
> - How about the openssl-devel dependency now that we are moving to NSS? Is this
> still allowed? 
  - I think if openssl is to be removed from Fedora there will be such
    an announce.

Then:
* Dependency for -devel subpackage
  - Every package which contains pkgconfig .pc file should
    have "Requires: pkgconfig" so please add this to -devel subpackage.

And I will wait for your another review request submit or your
pre-review of other person's review request.
Comment 8 François Kooman 2008-10-15 05:51:09 EDT
Done:

Spec URL: http://users.tuxed.net/fkooman/rpmbuild/SPECS/globalplatform.spec
SRPM URL:
http://users.tuxed.net/fkooman/rpmbuild/SRPMS/globalplatform-5.0.0-4.fc10.src.rpm

I'll submit a review request for GPshell (which depends on globalplatform) shortly. 

Thanks! :)
Comment 9 Mamoru TASAKA 2008-10-15 11:34:46 EDT
Well, this package itself is now good, so now I will wait for
your another review request or your pre-review.
Comment 10 Mamoru TASAKA 2008-10-22 09:46:53 EDT
ping?
Comment 11 François Kooman 2008-10-22 11:14:45 EDT
pong :)

I'm still alive and looking at GPshell packaging, but it's not quite so easy to get this into acceptable shape as it for example includes "HelloWorld.cap" which comes from Sun's Wireless Toolkit as an example with a license which most likely wouldn't be acceptable for Fedora (some weird proprietary license). So I guess I would have to recreate the tar file myself. Next to this it would be nice if GPshell would use pkgconfig for the library locations which it doesn't do right now afaik. 

So I didn't get around to this yet unfortunately :(
Comment 12 Mamoru TASAKA 2008-11-12 12:15:56 EST
If it seems difficult to package GPshell, would you try to do a pre-review
of other person's review request?
Comment 13 Mamoru TASAKA 2008-11-20 09:48:40 EST
ping again?
Comment 14 Mamoru TASAKA 2008-11-27 02:57:45 EST
ping again?
Comment 15 François Kooman 2008-11-30 09:28:30 EST
https://bugzilla.redhat.com/show_bug.cgi?id=473775

Review request for GPshell (which depends on globalplatform). My apologies for the delay...
Comment 16 Mamoru TASAKA 2008-12-02 09:17:36 EST
Okay, now gpshell is in good shape, I will approve this package

----------------------------------------------------------------
    This package (globalplatform) is APPROVED by mtasaka
----------------------------------------------------------------

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Get a Fedora Account".
After you request for sponsorship a mail will be sent to sponsor 
members automatically (which is invisible for you) which notifies 
that you need a sponsor. After that, please also write on
this bug for confirmation that you requested for sponsorship and
your FAS (Fedora Account System) name. Then I will sponsor you.

If you want to import this package into Fedora 9/10, you also have
to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.
Comment 17 François Kooman 2008-12-02 11:51:50 EST
FAS name: fkooman

I requested sponsoring. Thanks :)
Comment 18 Mamoru TASAKA 2008-12-02 11:58:09 EST
Now I am sponsoring you. Please follow "Join" wiki again.
Comment 19 François Kooman 2008-12-02 15:21:33 EST
I used Koji scratch build, which runs perfectly on all platforms for F10, but on Rawhide it breaks (because of libtool-2.2). I don't run rawhide here yet. I'll try to fix this libtool issue if I can find what needs changing...

dist-f10: http://koji.fedoraproject.org/koji/taskinfo?taskID=971571
dist-f11: http://koji.fedoraproject.org/koji/taskinfo?taskID=971550
Comment 20 Mamoru TASAKA 2008-12-03 11:15:40 EST
Well, I tried to add "libtoolize --force" before calling aclocal,
however for this package this does not work because with it
only static archive is created and shared library is not created.
The created ./libtool file contains "build_libtool_libs=no".

I will try to debug what is causing this....
Comment 21 Mamoru TASAKA 2008-12-03 12:57:35 EST
Created attachment 325567 [details]
Patch to build shared library with libtool 2.2

Okay, with this patch added as %Patch1,

%prep
%setup -q
%patch0 -p1 -b add_pkgconfig.patch
%patch1 -p1 -b .order

%if 0%{?fedora} >= 11
libtoolize --force --copy
%endif
aclocal
autoconf
automake
touch -r Makefile.in configure aclocal.m4

will make build succeed:
http://koji.fedoraproject.org/koji/taskinfo?taskID=973859
http://koji.fedoraproject.org/koji/taskinfo?taskID=973864

By the way, please follow "Join" wiki again.
Comment 22 François Kooman 2008-12-03 14:34:09 EST
New Package CVS Request
=======================
Package Name: globalplatform
Short Description: Library for access to OP 2.0.1 and GP 2.1.1 conforming smart cards
Owners: fkooman
Branches: F-9 F-10
InitialCC:
Comment 23 François Kooman 2008-12-03 14:54:46 EST
Spec URL: http://users.tuxed.net/fkooman/rpmbuild/SPECS/globalplatform.spec
SRPM URL:
http://users.tuxed.net/fkooman/rpmbuild/SRPMS/globalplatform-5.0.0-5.fc10.src.rpm

- make it build with libtool 2.2 (rawhide)
Comment 24 Kevin Fenzi 2008-12-03 19:52:51 EST
cvs done.
Comment 25 Mamoru TASAKA 2008-12-07 10:59:51 EST
Would you rebuilt this package for older branches?
Comment 26 François Kooman 2008-12-07 12:45:49 EST
Yes, I'll look at uploading/pushing them to F9/F10 tomorrow (Monday), I wanted to wait for everything to show up in Rawhide, which it did. Thanks for your assistance and patience with everything!
Comment 27 François Kooman 2008-12-08 04:43:04 EST
I ran make build in the globalplatform directories for F9 and F10 and that works great. For gpshell it doesn't because globalplatform is (still) missing. Should I first push globalplatform to stable before I can run gpshell's make build? Or would testing be enough? I guess no one will be able to test globalplatform without gpshell :)

Is it appropriate to push immediately to stable for a new package? I also saw that there is a type "newpackage" in Bodhi's new package creation form, is that one used now for new packages as opposed to "enhancement" as it is described in the wiki pages?
Comment 28 Mamoru TASAKA 2008-12-08 05:33:38 EST
(In reply to comment #27)
> For gpshell it doesn't because globalplatform is (still) missing.
> Should I first push globalplatform to stable before I can run gpshell's make
> build? Or would testing be enough?

- If you want to rebuild gpshell before waiting globalplatform to be pushed
  into stable repositories (testing state is not enough here for rebuilding
  gpshell), login on
  https://fedorahosted.org/rel-eng/
  and submit a new ticket to make globalplatform tagged as override tag like

  "To rebuild gpshell, please tag the following packages as dist-f{10,9}-override:
   globalplatform-5.0.0-5.fc10
   globalplatform-5.0.0-5.fc9"

> I guess no one will be able to test globalplatform without gpshell :)
- When gpshell is rebuild as above, you can request on bodhi to push multiple
  packages altogether.

> Is it appropriate to push immediately to stable for a new package?
- It is up to how you judge.

> I also saw
> that there is a type "newpackage" in Bodhi's new package creation form, is that
> one used now for new packages as opposed to "enhancement" as it is described in
> the wiki pages?
- It seems.
Comment 29 Mamoru TASAKA 2008-12-10 10:05:00 EST
Closing.

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