Bug 196393 - (svrcore) Review Request: svrcore
Review Request: svrcore
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jarod Wilson
Fedora Package Reviews List
:
: 196392 (view as bug list)
Depends On:
Blocks: FE-ACCEPT mozldap
  Show dependency treegraph
 
Reported: 2006-06-22 17:56 EDT by Rich Megginson
Modified: 2007-11-30 17:11 EST (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-01-05 17:24:46 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Rich Megginson 2006-06-22 17:56:10 EDT
Spec URL: ftp://ftp.mozilla.org/pub/mozilla.org/directory/svrcore/releases/4.0.2/svrcore.spec
SRPM URL: ftp://ftp.mozilla.org/pub/mozilla.org/directory/svrcore/releases/4.0.2/svrcore-devel-4.0.2-1.src.rpm 
Description: svrcore-devel is a library which provides applications that use NSS a standard way to allow PIN input
from a file or from the terminal.  There is also a PIN handler
that caches the PIN in memory after encrypting it with a key on
a device (such as a Fortezza card). This allowed a server to
restart without having to reenter the PIN. However since the PIN
is encrypted, a core dump would not expose it.  In addition,
removing the device would also make the PIN inaccessible.

It's only a devel package, containing only a static library and a header file.  It is currently only used by the Mozilla LDAP C SDK, but may find a wider audience since NSS is now part of Fedora Core and svrcore is very useful for some applications which use NSS.
Comment 1 Paul Howarth 2006-06-22 18:34:39 EDT
*** Bug 196392 has been marked as a duplicate of this bug. ***
Comment 2 Parag AN(पराग) 2006-06-23 01:04:21 EDT
Not an official review as I'm not yet sponsored
Mock build for i386 development gave
/var/tmp/rpm-tmp.45547: line 42: /usr/bin/pkg-config: No such file or directory

MUST Items:
     - MUST: rpmlint shows error 
      E: svrcore-devel invalid-spec-name svrcore.spec
     - MUST: dist tag is NOT present
     - MUST: The package is NOT named according to the Package Naming Guidelines.
     - MUST: The spec file name did NOT matching the base package svrcore-devel,
in the format svrcore-devel.spec
      - MUST: This package meets the Packaging Guidelines.
      - MUST: The package is licensed with an open-source compatible license GPL.
      - MUST: The License field in the package svrcore.spec file did NOT match
any license file in tarball.
       - MUST: This package  have a %clean section, which contains rm -rf
$RPM_BUILD_ROOT.
      - MUST: This package used macros.
      - MUST: No Document files are included in package.
      * Source URL is NOT present.
      * BuildRoot is correct BuildRoot:       
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
      * BuildRequires is NOT correct


You need to do:-
       * Add dist tag to release as
        Release: 1%{?dist}
       * Change SPEC file name to svrcore-devel.spec
       * Add license file in upstream tarball
       * Source is not URL
       * Add pkgconfig in BuildRequires

When i added pkgconfig and check this package in mock still i got some errors
 make EXPORTS= RELEASE= REQUIRES= MODULE= IMPORTS= OBJDIR=. INSTALL=true
syntax error at -e line 3, near "while"
syntax error at -e line 7, near "}"
Execution of -e aborted due to compilation errors.
  I confused here. Can you check whats happening in make that made above error
Though package was built i have not checked in by installing it. First correct
above things and upload new version.
check
http://fedoraproject.org/wiki/Packaging/Guidelines?highlight=%28packaging%29 for
packaging guidelines
  
Comment 3 Rich Megginson 2006-06-23 11:31:59 EDT
Thank you for the review.

I think the -e errors are coming from a missing perl or missing sed.  I have
added these to the BuildRequires.  I also added the source tarball to the same
ftp site as the other files, as well as making the other specified changes. 
Please check them again.
Comment 4 Parag AN(पराग) 2006-06-24 01:00:47 EDT
Each time you chnage SEPC or tarball increase version/release accordingly and
put that updated SPEC as well SRPM link here
Comment 6 Parag AN(पराग) 2006-06-26 23:34:52 EDT
SPEC looks ok
Comment 7 Jarod Wilson 2006-07-13 10:14:27 EDT
I don't have sponsor status, but can do the 'official' review and then kick
someone who does have sponsor status when the package is approved.
Comment 8 Jarod Wilson 2006-07-13 11:41:28 EDT
Okay, first pass through, spec is NOT yet okay. :)

1) Per http://fedoraproject.org/wiki/Extras/FullExceptionList, gawk, perl and
sed shouldn't be in BuildRequires:, not sure why Parag had issues there, package
builds okay in an FC6/x86_64 mock buildroot with them removed.

2) Provides: svrcore-devel is not necessary, its already named that and will
auto-provide it. (rpmlint says "E: svrcore-devel useless-explicit-provides
svrcore-devel")

3) You can make the spec cleaner using 'export VAR=value' all on one line.

4) The '%{__mkdir_p} $RPM_BUILD_ROOT/%{_libdir}/pkgconfig' line should not be
under %build, it should be in %install (rpmlint complains about this: "W:
svrcore-devel rpm-buildroot-usage %build %{__mkdir_p}
$RPM_BUILD_ROOT%{_libdir}/pkgconfig")

5) The first line in %install should be an rm -rf of the buildroot (rpmlint: "E:
svrcore-devel no-cleaning-of-buildroot").

6) You have extraneous slashes throughout the spec. (ex:
$RPM_BUILD_ROOT/%{_libdir} should be just $RPM_BUILD_ROOT%{_libdir}, because
%{_libdir} expands out to /usr/lib or /usr/lib64).

7) Source includes a LICENSE and README file, they should be installed as docs
(rpmlint warns about lack of docs).

8) %defattr should generally be (-,root,root,-)

Here's a spec diff that cleans up all of the above issues:
----------

--- svrcore-devel-orig.spec     2006-06-26 11:18:43.000000000 -0400
+++ svrcore-devel.spec  2006-07-13 11:45:08.000000000 -0400
@@ -13,11 +13,7 @@
 BuildRoot:        %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
 BuildRequires:    nspr-devel >= %{nspr_version}
 BuildRequires:    nss-devel >= %{nss_version}
-BuildRequires:    gawk
-BuildRequires:    perl
-BuildRequires:    sed
 BuildRequires:    pkgconfig
-Provides:         svrcore-devel

 Source0:         
ftp://ftp.mozilla.org/pub/mozilla.org/directory/svrcore/releases/4.0.2/%{name}-%{version}.tar.gz

@@ -33,28 +29,19 @@
 %build

 # Enable compiler optimizations and disable debugging code
-BUILD_OPT=1
-export BUILD_OPT
+export BUILD_OPT=1

 # Generate symbolic info for debuggers
-XCFLAGS=$RPM_OPT_FLAGS
-export XCFLAGS
+export XCFLAGS="$RPM_OPT_FLAGS"

-PKG_CONFIG_ALLOW_SYSTEM_LIBS=1
-PKG_CONFIG_ALLOW_SYSTEM_CFLAGS=1
+export PKG_CONFIG_ALLOW_SYSTEM_LIBS=1
+export PKG_CONFIG_ALLOW_SYSTEM_CFLAGS=1

-export PKG_CONFIG_ALLOW_SYSTEM_LIBS
-export PKG_CONFIG_ALLOW_SYSTEM_CFLAGS
-
-NSPR_INCLUDE_DIR=`/usr/bin/pkg-config --variable=includedir nspr`
-export NSPR_INCLUDE_DIR
-
-NSS_INCLUDE_DIR=`/usr/bin/pkg-config --variable=includedir nss`
-export NSS_INCLUDE_DIR
+export NSPR_INCLUDE_DIR=`/usr/bin/pkg-config --variable=includedir nspr`
+export NSS_INCLUDE_DIR=`/usr/bin/pkg-config --variable=includedir nss`

 %ifarch x86_64 ppc64 ia64 s390x
-USE_64=1
-export USE_64
+export USE_64=1
 %endif

 cd mozilla/security/svrcore
@@ -63,35 +50,36 @@
 # dependencies, etc.
 make EXPORTS="" RELEASE="" REQUIRES="" MODULE="" IMPORTS="" OBJDIR=. INSTALL=true

+%install
+%{__rm} -rf $RPM_BUILD_ROOT
+
 # Set up our package file
-%{__mkdir_p} $RPM_BUILD_ROOT/%{_libdir}/pkgconfig
-%{__cat} svrcore.pc.in | sed -e "s,%%libdir%%,%{_libdir},g" \
+%{__mkdir_p} $RPM_BUILD_ROOT%{_libdir}/pkgconfig
+%{__cat} mozilla/security/svrcore/svrcore.pc.in | sed -e
"s,%%libdir%%,%{_libdir},g" \
                           -e "s,%%prefix%%,%{_prefix},g" \
                           -e "s,%%exec_prefix%%,%{_prefix},g" \
                           -e "s,%%includedir%%,%{_includedir},g" \
                           -e "s,%%NSPR_VERSION%%,%{nspr_version},g" \
                           -e "s,%%NSS_VERSION%%,%{nss_version},g" \
                           -e "s,%%SVRCORE_VERSION%%,%{version},g" > \
-                          $RPM_BUILD_ROOT/%{_libdir}/pkgconfig/%{name}.pc
-
-%install
+                          $RPM_BUILD_ROOT%{_libdir}/pkgconfig/%{name}.pc

 # There is no make install target so we'll do it ourselves.

-%{__mkdir_p} $RPM_BUILD_ROOT/%{_includedir}
-%{__mkdir_p} $RPM_BUILD_ROOT/%{_libdir}
+%{__mkdir_p} $RPM_BUILD_ROOT%{_includedir}
+%{__mkdir_p} $RPM_BUILD_ROOT%{_libdir}

 cd mozilla/security/svrcore
 # Copy the binary libraries we want
 for file in libsvrcore.a
 do
-  %{__install} -m 644 $file $RPM_BUILD_ROOT/%{_libdir}
+  %{__install} -m 644 $file $RPM_BUILD_ROOT%{_libdir}
 done

 # Copy the include files
 for file in svrcore.h
 do
-  %{__install} -m 644 $file $RPM_BUILD_ROOT/%{_includedir}
+  %{__install} -m 644 $file $RPM_BUILD_ROOT%{_includedir}
 done


@@ -99,7 +87,8 @@
 %{__rm} -rf $RPM_BUILD_ROOT

 %files
-%defattr(0644,root,root)
+%defattr(-,root,root,-)
+%doc mozilla/security/svrcore/LICENSE mozilla/security/svrcore/README
 %{_libdir}/pkgconfig/%{name}.pc
 %{_libdir}/libsvrcore.a
 %{_includedir}/svrcore.h


----------

With these changes, I have the rpmlint output down to:
E: svrcore-devel-debuginfo empty-debuginfo-package

Not quite sure yet why the debuginfo package is empty, will investigate more a
bit later...
Comment 9 Rich Megginson 2006-07-13 11:47:55 EDT
Thanks.  I'll get working on it.
One question though, about export var=value - I thought that was a ksh/bash-ism,
not a plain old posix bourne shell feature?  I'd rather have the scripting parts
be maximally portable.
Comment 10 Jarod Wilson 2006-07-13 14:36:20 EDT
(In reply to comment #9)
> One question though, about export var=value - I thought that was a ksh/bash-ism,
> not a plain old posix bourne shell feature?  I'd rather have the scripting parts
> be maximally portable.

True, it won't work on plain old bourne, but there's no plain old bourne on
anything we ship or support anymore so I'm not sure what target platform you
have in mind that it would be an issue on. Not a blocker though if you want to
keep them split, I just like to keep things as short as possible. :)
Comment 11 Jarod Wilson 2006-07-13 14:42:06 EDT
Er, actually, I'm not certain what we have on RHEL3 (or RHEL2.1, but I dunno if
that's a platform we want people running the DS stuff on)...
Comment 13 Warren Togami 2006-07-17 13:25:07 EDT
Is there any reason why this *must* be a static-only library?  Normally this is
frowned upon.
Comment 14 Rich Megginson 2006-07-17 13:38:01 EDT
I don't think so.  We inherited this package from the Netscape acquisition - it
was always a static lib there and here.  It's only used by the Mozilla LDAP C
SDK command line programs for secure SSL pin management, so it gets statically
compiled into its ldapsearch, ldapmodify, etc.
(https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=196401).  It's hoped that
once more apps begin using NSS for crypto, there will be more apps that could
use it, at which point we could look at making it a shared lib.

But really, the only thing we need it for now is so we can get mozldap into
Fedora Extras.  The only reason we haven't just put the code into mozldap is
because we hope to have other apps use it in the future.
Comment 15 Rich Megginson 2006-07-24 14:04:38 EDT
Fedora Directory Server uses svrcore also.
Comment 16 Rich Megginson 2006-10-18 18:46:40 EDT
This is a devel only package - it is not needed at runtime.  It is only needed
when building mozldap and fedora directory server.  Would it be possible to add
this package as a static library given these considerations?
Comment 17 Tom "spot" Callaway 2006-11-02 13:48:42 EST
"we hope to have other apps use it in the future" should be a motivator to do it
right (shared libraries). I'm not seeing a valid reason to permit static
libraries here.
Comment 18 Patrice Dumas 2006-11-02 16:20:46 EST
When upstream ships only static libs it is not a blocker not to
have shared libs.
Comment 19 Rich Megginson 2006-11-02 16:29:59 EST
(In reply to comment #18)
> When upstream ships only static libs it is not a blocker not to
> have shared libs.

Plus, it's only a -devel package, not a runtime package (or perhaps that's
redundant).

On the other hand, we (the Fedora DS team) are the upstream for svrcore-devel.
Comment 20 Jarod Wilson 2006-11-06 11:51:42 EST
Its a bit of a balancing act here, I suppose... We want FDS in Extras sooner
than later, but we can't loosen the guidelines for something just because we
really want it. Yes, when upstream ships only a static lib, its not a blocker,
but since we *are* upstream, it seems we ought to fix this, not skate around it
-- I think the rule is generally meant to apply to situations where the packager
has no direct control over what upstream does.
Comment 21 Warren Togami 2006-12-06 17:33:58 EST
Upstream has decided to make svrcore into a proper dynamic so in future
versions, which should satisfy this requirement.  Unfortunately, they don't have
time to prioritize to this at the moment.  If we want FDS in directly in Fedora
sooner, then volunteers need to step up and help the Fedora Directory Server
project.

http://directory.fedora.redhat.com/
Comment 22 Toshio Kuratomi 2006-12-08 00:41:26 EST
I'm looking into autotooling this.  It's not impossible but I'm going to have to
look into the coreconf stuff more closely to make sure I'm not missing something
in my initial pass.  Some questions:

* Why is this package called srvcore-devel?  The library looks like it's called
libsrvcore so the -devel extension looks extraneous. (This will also cause you
packaging headaches as there's usually a main package with the shared libs and a
subpackage named %{name}-devel for static libraries and headers.  If you did
that in this case you'd have svrcore-devel-devel.  So you can either rename at
the upstream level or the packaging level.)

* Is it okay if I change the directory structure or are you still committed to
building this in a mozilla tree where the directory structure is important?

* Does anyone understand coreconf and want to walk me through it on IRC -- if
not, I'll figure it out on my own.  It's just a question of speed and assurance
that I'm not taking a false start.
Comment 23 Toshio Kuratomi 2006-12-08 03:08:19 EST
http://www.tiki-lounge.com/~toshio/fedora/svrcore-4.0.2.01-0.src.rpm

Initial work to port this to autotools.
There's a tarball and x86_64 rpms in that directory as well.  Please be warned
that I am pretty good with autotools but I don't know svrcore or the netscape
build system at all.  I have no programs to test this against.  Therefore, this
could be totally borked (most likely, borked on certain archs but not other as
replacing the buildsystem usually hits portability hacks).

That said, here's the status as I currently see it:
* New layout.  The tarball contains two directories.  svrcore-VERSION/ with
supporting files (README, LICENSE, etc) and svrcore-VERSION/src with the source
code.
* Autotools configures and builds shared and static libraries on linux-x86_64.
* Autotools take care of generating the pkgconfig file as well.
* Running make distcheck passes and creates distribution tarballs.
* Cleaned up the rpm spec file a bit for the new build system.  Commented a
bunch of the env variable defining as I've only done an initial read through to
figure out if it was necessary.
* Documentation needs to be updated: NEWS, AUTHORS, and ChangeLog are new files
that can be filled with useful information :-)  INSTALL needs to be updated with
the new install proceedures.
* configure.ac has an email address for bug reports in one of its macros.  Just
need to replace what's there with the proper address.
* I don't have a Windows box so I don't know how autotools works on Windows (or
if this is something you place a high priority on.)
* I need to run ifnames on the source files and figure out where all the cpp
variables get set.  Most of them looked Windows specific so there's a high
probablility things work on *nix boxes but not Windows.  We need to figure out
hy these defines are set and implement tests in configure.ac to set them
instead.  This is where talkign with someone who understands svrcore/coreconf
would be helpful.
Comment 24 Warren Togami 2006-12-08 09:22:13 EST
svrcore-devel was the old name, only because this was a static only library in
the past.  svrcore should be the new .src.rpm name.
Comment 25 Axel Thimm 2006-12-08 09:31:57 EST
I tested the build of Toshio's new tarball/spec on FC6 for i386/x86_64/ppc and
all have a

o clean rpmlint output (aside from missing doc warnings in the devel subppackage)
o sane buildlogs, e.g. both confugure and make output looks fine for all archs
o mozldap builds against it

I didn't test any runtime behaviour, but I think it's already worthwhile to
submit Toshio's changes upstream. I think that's also the only place where
Windows builds could be tested, if that's a requirement.
Comment 26 Rich Megginson 2006-12-08 11:11:39 EST
Yes, Windows is a requirement.  I can take care of building on Windows.

Here is the upstream bug for the work that you guys have done:
https://bugzilla.mozilla.org/show_bug.cgi?id=363168

Comment 27 Dennis Gilmore 2006-12-08 11:37:11 EST
i tested the build of Toshio's build on Aurora Corona  the equivelant of FC-6 
sparc.  build was ok  but i have not tested runtime or building against it 
Comment 28 Toshio Kuratomi 2006-12-08 12:40:27 EST
New srpm:
http://www.tiki-lounge.com/~toshio/fedora/svrcore-4.0.2.02-0.src.rpm

I've gone through the #ifdefs now and made a few minor changes.  I think this
tarball is good to go as far as replacing coreconf with autotools.  It will need
some tweaking from your end (documentation, runtime testing, etc) but is largely
complete.

Changes:
* configure.ac: We don't have any fallbacks in code for missing string.h,
termios.h, or unistd.h so Have configure error if any of those are missing.

* ntgetpin.c, user.c: Code was using both #ifdef WIN32 and #ifdef _WIN32. 
Changed all to the _WIN32 form.

* user.c: Instead of checking if we're building on XP_UNIX, check that we aren't
building on _WIN32.  This matches with code later in the file that uses the
functions define here if we are not on _WIN32.


I'll post this to the upstream bugzilla as well.
Comment 29 Rich Megginson 2006-12-14 12:52:10 EST
Thank you very much, Toshio-san.  I have taken your changes and added some code
to make finding nspr and nss flexible depending on the build environment.  I
unfortunately could not make configure/libtool work on Windows with the MSVC
compiler, so the Windows builds will have to build the old way. No big deal. 
But the new configure way to build should be much better on the linux/unix
platforms we have to support.  Since these changes are pretty significant, I
went ahead and changed the version number.  The new files are here:
SRPM:
ftp://ftp.mozilla.org/pub/mozilla.org/directory/svrcore/releases/4.0.3/svrcore-4.0.3.01-0.src.rpm
SOURCE:
ftp://ftp.mozilla.org/pub/mozilla.org/directory/svrcore/releases/4.0.3/svrcore-4.0.3.01.tar.gz
SPEC:
ftp://ftp.mozilla.org/pub/mozilla.org/directory/svrcore/releases/4.0.3/svrcore.spec
Comment 30 Jarod Wilson 2006-12-15 14:47:12 EST
Lots of progress here... Re-review notes:

* package meets naming and packaging guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.

The spec file in the src.rpm is dirsec-svrcore.spec, references other dirsec-
bits, I presume this isn't the version aiming to get into Fedora Extras. The
stand-alone svrcore.spec you link to looks correct though.

* dist tag is present.
* build root is correct.
      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
* license field matches the actual license.
* license is open source-compatible.  MPL/GPL/LGPL, text included in package.
* source files match upstream:
      fbb56acf580aa0ebb32df58594458b28  svrcore-4.0.3.01-orig.tar.gz
      fbb56acf580aa0ebb32df58594458b28  svrcore-4.0.3.01.tar.gz
* latest version is being packaged.
* BuildRequires are proper.
* package builds in mock (rawhide/x86_64).
* rpmlint is silent.
    only 1 ignorable warning about no docs in -devel
* final provides and requires are sane:
    svrcore-4.0.3.01-0.fc7.x86_64.rpm
    libsvrcore.so.0()(64bit)  
    svrcore = 4.0.3.01-0.fc7
    =
    /sbin/ldconfig  
    /sbin/ldconfig  
    libnspr4.so()(64bit)  
    libnss3.so()(64bit)  
    libnss3.so(NSS_3.10.2)(64bit)  
    libnss3.so(NSS_3.2)(64bit)  
    libplc4.so()(64bit)  
    libplds4.so()(64bit)  
    libssl3.so()(64bit)  
    libsvrcore.so.0()(64bit)  
    nspr >= 4.6
    nss >= 3.11

    svrcore-debuginfo-4.0.3.01-0.fc7.x86_64.rpm
    libsvrcore.so.0.0.0.debug()(64bit)  
    svrcore-debuginfo = 4.0.3.01-0.fc7
    =
    (none)

    svrcore-devel-4.0.3.01-0.fc7.x86_64.rpm
    svrcore-devel = 4.0.3.01-0.fc7
    =
    libsvrcore.so.0()(64bit)  
    nspr-devel >= 4.6
    nss-devel >= 3.11
    pkgconfig  
    svrcore = 4.0.3.01-0.fc7

* shared libraries are properly split between main and -devel packages.
* package is not relocatable.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* %clean is present.
* %check is N/A
* appropriate minimal ldconfig scriptlets.
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* header in -devel package, installs in /usr/include/.
* pkgconfig files appropriately in -devel.
* no libtool .la droppings.
* not a GUI app.
* not a web app.


Assuming you create a new srpm with the correct spec (the one w/o dirsec-), I'm
gonna say APPROVED, import at will.
Comment 31 Rich Megginson 2006-12-15 15:04:54 EST
Darn it, that's the wrong srpm.  I'm uploading a better one now.
SRPM:
ftp://ftp.mozilla.org/pub/mozilla.org/directory/svrcore/releases/4.0.3/svrcore-4.0.3.01-0.src.rpm
Comment 32 Dennis Gilmore 2007-01-05 10:39:45 EST
Ping please import to cvs and request builds
Comment 33 Rich Megginson 2007-01-05 17:15:36 EST
(In reply to comment #32)
> Ping please import to cvs and request builds

Done.  Sources are in /cvs/extras, branches have been created for FC-5 and FC-6,
and I was successfully able to build on FC-6.

What's left to do?  Just close this bug as NEXTRELEASE?
Comment 34 Rich Megginson 2007-01-05 17:24:46 EST
Finished.

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