Bug 234612 - Review Request: Ice - The Internet Communications Engine (Object middleware)
Review Request: Ice - The Internet Communications Engine (Object middleware)
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Package Reviews List
:
: 247556 247557 (view as bug list)
Depends On:
Blocks: 247556 247557
  Show dependency treegraph
 
Reported: 2007-03-30 11:55 EDT by Mary Ellen Foster
Modified: 2007-11-30 17:12 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-08-31 02:18:45 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
petersen: fedora‑cvs+


Attachments (Terms of Use)
mock build log of ice 3.2.0-4 on Fedora devel i386 (321.20 KB, text/plain)
2007-06-28 08:21 EDT, Mamoru TASAKA
no flags Details

  None (edit)
Description Mary Ellen Foster 2007-03-30 11:55:09 EDT
This is my first package, and I'm looking for a sponsor.


There are three SRPMS, because there are two noarch packages. These specs are mainly based on the upstream SRPM at http://www.zeroc.com/download/Ice/3.2/ice-3.2.0-1.src.rpm but I've tried to modify them so that they meet the Fedora packaging guidelines.

Some things to note:
- There are three SRPMs because, although most of the packages are arch-specific, there are also two noarch packages which I've packaged separately.
- The two noarch packages need pieces of various arch-specific packages to build properly. In the current version, they both build private versions of the executables they need. For Java, it could have been done with a BuildRequires on ice-java-devel = %{version}; for dotnet, I don't think it could have gone any other way: ice-dotnet BuildRequires ice-csharp-devel, but -- because of the pkgconfig files -- ice-csharp-devel Requires ice-dotnet.
- I've followed the naming convention used by upstream, but I'm open to changing it.
- The Java stuff can be compiled with Java 1.5, but I'm ignoring that because there's no free 1.5 JVM (yet?). Also, there's a quite useful admin tool written in Java that uses jgoodies, which doesn't seem to be packaged for Fedora (again, yet?).
- The documentation and descriptions are lacking, I know, but I wanted advice on packaging technique so I'm submitting these for review now.

Main package (all arch-specific pieces):
Spec URL: http://www6.informatik.tu-muenchen.de/~foster/extras/ice/ice-arch.spec
SRPM URL: http://www6.informatik.tu-muenchen.de/~foster/extras/ice/ice-3.2.0-1.src.rpm
Description:
Ice is a modern alternative to object middleware such as CORBA or
COM/DCOM/COM+.  It is easy to learn, yet provides a powerful network
infrastructure for demanding technical applications. It features an
object-oriented specification language, easy to use C++, C#, Java,
Python, Ruby, PHP, and Visual Basic mappings, a highly efficient
protocol, asynchronous method invocation and dispatch, dynamic
transport plug-ins, TCP/IP and UDP/IP support, SSL-based security, a
firewall solution, and much more.


Java runtime package (noarch)
Spec URL: http://www6.informatik.tu-muenchen.de/~foster/extras/ice/ice-java.spec
SRPM URL: http://www6.informatik.tu-muenchen.de/~foster/extras/ice/ice-java-3.2.0-1.src.rpm
Description:
The Ice runtime for Java.


Dotnet runtime package (noarch)
Spec URL: http://www6.informatik.tu-muenchen.de/~foster/extras/ice/ice-dotnet.spec
SRPM URL: http://www6.informatik.tu-muenchen.de/~foster/extras/ice/ice-dotnet-3.2.0-1.src.rpm
Description:
The Ice runtime for CSharp.
Comment 1 Mary Ellen Foster 2007-04-11 07:44:46 EDT
I've done a bit more testing and uploaded new versions. Changes:
- ice-arch: removed 1.5-ism (assert) from included Java class
- ice-dotnet: added "BuildArch: noarch"
- ice-java: added "BuildArch: noarch" and "BuildRequires: ant-nodeps"

Specs are in the same place as before. Here are the new SRPMS:
http://www6.informatik.tu-muenchen.de/~foster/extras/ice/ice-3.2.0-2.src.rpm
http://www6.informatik.tu-muenchen.de/~foster/extras/ice/ice-java-3.2.0-2.src.rpm
http://www6.informatik.tu-muenchen.de/~foster/extras/ice/ice-dotnet-3.2.0-2.src.rpm
Comment 2 Peter Lemenkov 2007-04-11 08:08:27 EDT
Little notes:

Change something in %files-section from 

/usr/bin/dumpdb
/usr/bin/transformdb
/usr/bin/glacier2router
/usr/bin/icebox
/usr/bin/iceboxadmin
/usr/bin/icecpp
/usr/bin/icepatch2calc
/usr/bin/icepatch2client
/usr/bin/icepatch2server
/usr/bin/icestormadmin
/usr/bin/slice2docbook
/usr/bin/slice2html
/usr/bin/icegridadmin
/usr/bin/icegridnode
/usr/bin/icegridregistry
/usr/bin/iceca
/usr/bin/ImportKey.class

to

%{_bindir}/dumpdb
%{_bindir}/transformdb
%{_bindir}/glacier2router
%{_bindir}/icebox
%{_bindir}/iceboxadmin
%{_bindir}/icecpp
%{_bindir}/icepatch2calc
%{_bindir}/icepatch2client
%{_bindir}/icepatch2server
%{_bindir}/icestormadmin
%{_bindir}/slice2docbook
%{_bindir}/slice2html
%{_bindir}/icegridadmin
%{_bindir}/icegridnode
%{_bindir}/icegridregistry
%{_bindir}/iceca
%{_bindir}/ImportKey.class

and 

/etc/init.d/icegridregistry
/etc/init.d/icegridnode
/etc/init.d/glacier2router
%config(noreplace) /etc/icegridregistry.conf
%config(noreplace) /etc/icegridnode.conf
%config(noreplace) /etc/glacier2router.conf

to 

%{_sysconfdir}/init.d/icegridregistry
%{_sysconfdir}/init.d/icegridnode
%{_sysconfdir}/init.d/glacier2router
%config(noreplace) %{_sysconfdir}/icegridregistry.conf
%config(noreplace) %{_sysconfdir}/icegridnode.conf
%config(noreplace) %{_sysconfdir}/glacier2router.conf

Comment 3 Mary Ellen Foster 2007-04-18 05:29:31 EDT
Some more modifications, now that I'm actually trying to use these packages
myself. :)

Changes for ice-java:
- Put files into Java shared directory (/usr/share/java)
- Optionally build IceGrid GUI (depends on jgoodies)
- Optionally use java5

Changes for ice-arch:
- Use RPM macros instead of /etc and /usr/bin (Thanks to Peter Lemenkov)
- Suggestions from ZeroC forum (http://zeroc.com/forums/showthread.php?t=3095):
  - Use Python site-packages directory
  - Create "iceuser" user
  - Split /etc/init.d services into a separate sub-package
- Follow guidelines from Fedora wiki about packaging Ruby
  - Use Ruby site-arch directory
  - Depend on ruby(abi)
- Make sure to compile all Java files with -source 1.4 -target 1.4

The new spec files are in the same place. Here are the new SRPMS:
http://www6.informatik.tu-muenchen.de/~foster/extras/ice/ice-3.2.0-3.src.rpm
http://www6.informatik.tu-muenchen.de/~foster/extras/ice/ice-java-3.2.0-3.src.rpm
Comment 4 Mary Ellen Foster 2007-06-13 08:47:53 EDT
I just realised that I had some crap left in the main spec file, so I've 
updated it again. (The Java and CSharp ones are unchanged.)

http://www6.informatik.tu-muenchen.de/~foster/extras/ice/ice-arch.spec
http://www6.informatik.tu-muenchen.de/~foster/extras/ice/ice-3.2.0-4.fc7.src.rpm

I'm using these packages regularly and they're working well for me ...
Comment 5 Mamoru TASAKA 2007-06-28 08:21:34 EDT
Created attachment 158113 [details]
mock build log of ice 3.2.0-4 on Fedora devel i386

Well, as this is a large srpm, it may take several (or more)
times of fixing before this package is approved...

NOTE: I am also reviewing astyle (bug 207896)

0. Review request itself
  * First of all I suggest to submit a new review request
    for ice-java and ice-dotnet.
    (I want to review only ice package on this bug entry)
  * And rename the spec name to "ice.spec".

A. Description stage
  * Macros
-----------------------------------
%ifarch x86_64
%define icelibdir lib64
%else
%define icelibdir lib
%endif
-----------------------------------
    - This can be replaced with %_lib macro.
    - And ppc64 also uses /usr/lib64 as %_libdir,
      lib64 as %_lib.

  * BuildRequires
    - In -ruby subpackage:
-----------------------------------
Requires: ice = %{version}, ruby(abi) = 1.8
-----------------------------------
      For this, I think that BuildRequires should also have
      BuildRequires: ruby(abi) = 1.8 .

B. %build stage
  * optflags
    - The build does not honor fedora specific optflags
      (check: "Compiler flags" section of
       http://fedoraproject.org/wiki/Packaging/Guidelines).
      The mock build log is attached.

C. %install stage
  * Timestamps
    - To keep timestamps on installing files, please use
      "cp -p" or "install -p" when using cp or install command
      ("Timestamps" section of Guildlines page)

  * Macros, libdir
--------------------------------------
mkdir -p $RPM_BUILD_ROOT/usr
--------------------------------------
    - Use %_prefix
--------------------------------------
mv $RPM_BUILD_ROOT/lib $RPM_BUILD_ROOT/usr/lib
--------------------------------------
    - This should be %{_libdir}, not /usr/lib

  * rc.init scripts
--------------------------------------
    cp $RPM_BUILD_DIR/Ice-rpmbuild-%{version}/$i.%{_vendor}
$RPM_BUILD_ROOT%{_sysconfdir}/init.d/$i
--------------------------------------
    - Please don't rc.init scripts into %_sysconfdir/init.d (/etc/init.d)
      but instead install them under %_initrddir (=/etc/rc.d/init.d)

D. %files entry
   * For main package:
---------------------------------------
%exclude %{_datadir}/Ice-%{version}/convertssl.pyo
%exclude %{_datadir}/Ice-%{version}/upgradeicegrid.pyo
%exclude %{_datadir}/Ice-%{version}/upgradeicestorm.pyo
%exclude %{_datadir}/Ice-%{version}/convertssl.pyc
%exclude %{_datadir}/Ice-%{version}/upgradeicegrid.pyc
%exclude %{_datadir}/Ice-%{version}/upgradeicestorm.pyc
---------------------------------------
    - These should not be excluded.

   * For scriptlets for -server subpackage
    - Use: %_localstatedir for /var, %_sbindir for /usr/sbin.

NOTE: I have not installed the rpms created yet. After the issue above
      are fixed, I will do further review.
Comment 6 Mamoru TASAKA 2007-07-04 10:07:56 EDT
ping?
Comment 7 Mary Ellen Foster 2007-07-09 19:09:25 EDT
Okay, I've updated the main Ice SRPM following the comments. Sorry for the delay ...

I left the Ruby stuff as-is -- it seems from the Ruby packaging guidelines that
that's how it should be (http://fedoraproject.org/wiki/Packaging/Ruby), but I
made the other changes as requested.

Spec: http://www6.informatik.tu-muenchen.de/~foster/extras/ice/ice.spec
SRPM:
http://www6.informatik.tu-muenchen.de/~foster/extras/ice/ice-3.2.0-5.fc7.src.rpm

I'll open the additional review requests shortly and post the links here.
Comment 8 Mary Ellen Foster 2007-07-09 19:19:52 EDT
Here are the additional bugs:

ice-java: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=247556
ice-dotnet: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=247557

I set both bugs to depend on this one -- should the dependency go the other way
around? I'm not sure.
Comment 9 Mamoru TASAKA 2007-07-10 10:56:47 EDT
Again I just checked for packaging.

For 3.2.0-5:

0. Description stage:
* Macros
  - %_sbindir is /usr/sbin, where service (and so on) is
    under /sbin, not under %_sbindir (there is no macro which
    corresponds with /sbin).
    Same for scriptlets.

* php Requirement
  - -php subpackage has "Requires: php >= 5.1.4". Does this mean
    that BuildRequires for php dependency should also have version
    specific dependency, i.e. "BuildRequires: php >= <someversion>"?

1. %build stage
* %optflags
  - %optflags is still not honored completely.
------------------------------------------------------
make[2]: Leaving directory `/builddir/build/BUILD/Ice-3.2.0/src/IceUtil'
making all in icecpp
make[2]: Entering directory `/builddir/build/BUILD/Ice-3.2.0/src/icecpp'
cc -c -I../../include  -O2 -I. -DPREFIX=\"\" cccp.c
cc -c -I../../include  -O2 -I. -DPREFIX=\"\" cexp.c
cc -c -I../../include  -O2 -I. -DPREFIX=\"\" prefix.c
rm -f ../../bin/icecpp
cc -O2 -I. -DPREFIX=\"\" -o ../../bin/icecpp cccp.o cexp.o prefix.o
make[2]: Leaving directory `/builddir/build/BUILD/Ice-3.2.0/src/icecpp'
------------------------------------------------------

2. Scripts
* Home directory
  - It seems that no one owns %_localstatedir/lib/icegrid, or
    doesn't this directory needed?

3. Others
* rpmlint
------------------------------------------------------
W: ice macro-in-%changelog _libdir (etc)
------------------------------------------------------
  - In %changelog, please use %%_prefix, %%_libdir, ...
    so that macros won't be expanded.
Comment 10 Mamoru TASAKA 2007-07-10 13:41:16 EDT
Ah.. I just glanced at the spec file of bug 247556 and
bug 247557 and noticed that both requires
Ice-%{version}.tar.gz to be rebuilt.

IMO the package using the same source should be rebuilt at
once. i.e. IMO both ice-dotnet and ice-java should be
included into ice srpm.
Comment 11 Mary Ellen Foster 2007-07-10 13:48:33 EDT
Unfortunately, I don't think that's possible. They're split out separately
because they're noarch packages (Java and .Net runtime files only), while the
rest of Ice is arch-specific. They both BuildRequire some arch pieces, though,
so that's why I included the tarball in the other SRPMSs. The alternative is for
ice-java to BuildRequire ice-java-devel and similarly for ice-dotnet, but that
seems like it could cause problems on the build system.

Actually, even that won't completely work, because ice-dotnet BuildRequires some
pieces of ice-dotnet-devel, but ice-dotnet-devel Requires some pkg-config files
that are part of ice-dotnet. So that's why I'm doing the bootstrapping thing to
build the noarch packages.

I asked the fedora-packaging list about this issue a few months ago. Here's a
link to the discussion:
    https://www.redhat.com/archives/fedora-packaging/2007-March/msg00134.html
Comment 12 Mamoru TASAKA 2007-07-10 13:55:40 EDT
(In reply to comment #11)
> Unfortunately, I don't think that's possible. They're split out separately
> because they're noarch packages (Java and .Net runtime files only), while the
> rest of Ice is arch-specific. 
When we are to build in one big srpm, this is not a problem.
We can just regard them all as arch-specific.

> Actually, even that won't completely work, because ice-dotnet 
> BuildRequires some
> pieces of ice-dotnet-devel, but ice-dotnet-devel Requires some
>  pkg-config files
> that are part of ice-dotnet. 

Even if you set PKG_CONFIG_PATH?
Comment 13 Mamoru TASAKA 2007-07-24 06:51:11 EDT
ping?
Comment 14 Mary Ellen Foster 2007-07-31 17:04:50 EDT
Okay, I've now put everything into one big SRPM. I'm not sure this is the way to
go -- non-noarch Java and C# packages are sort of weird -- but this does make
things neater.

http://www6.informatik.tu-muenchen.de/~foster/extras/ice/ice.spec
http://www6.informatik.tu-muenchen.de/~foster/extras/ice/ice-3.2.0-6.fc7.lcg.src.rpm
Comment 15 Mamoru TASAKA 2007-08-01 02:07:09 EDT
(In reply to comment #14)
> http://www6.informatik.tu-muenchen.de/~foster/extras/ice/ice.spec
>
http://www6.informatik.tu-muenchen.de/~foster/extras/ice/ice-3.2.0-6.fc7.lcg.src.rpm

Well, I just tried to rebuild and have not done anything else:
The results are: (you can browse/download the rpms, build log else
                  from the following URL)

* on i386 and ppc, the rebuild is okay (again, I just rebuilt)
  http://koji.fedoraproject.org/koji/taskinfo?taskID=84407
  http://koji.fedoraproject.org/koji/taskinfo?taskID=84425

* rebuild fails on x86_64
  http://koji.fedoraproject.org/koji/taskinfo?taskID=84409
  Build log says that file movement regarding to %_lib is wrong

* Also rebuild fails on ppc64
  http://koji.fedoraproject.org/koji/taskinfo?taskID=84403
  root log says that mono-core and db4-java are not available
  on ppc64. You have to disable the support of some components
  on ppc64.
Comment 16 Mamoru TASAKA 2007-08-01 02:09:55 EDT
By the way, ice-dotnet is still seperated?
Comment 17 Mamoru TASAKA 2007-08-01 02:11:16 EDT
*** Bug 247556 has been marked as a duplicate of this bug. ***
Comment 18 Mary Ellen Foster 2007-08-01 05:06:16 EDT
*** Bug 247557 has been marked as a duplicate of this bug. ***
Comment 19 Mary Ellen Foster 2007-08-01 11:37:16 EDT
Okay, here's a new version. Changelog:
- Fixed arch-specific issues:
  - %ifnarch ppc64 in a lot of places; it doesn't have db4-java or mono-core, so 
    no Java or CSharp packages
  - Replaced one literal "lib" with %{_lib}
- Added IceGrid registry patch from ZeroC forum
- Don't build "test" or "demo" subdirectories
- Use "/sbin/ldconfig" instead of %{_sbindir} because that's /usr/sbin
- Removed useless "dotnetversion" define (it's the same as "version")
- Remove executable bit on all "*.ice" files (it gets set somehow on a few)

Here are the new versions:
http://www6.informatik.tu-muenchen.de/~foster/extras/ice/ice.spec
http://www6.informatik.tu-muenchen.de/~foster/extras/ice/ice-3.2.0-6.fc7.lcg.src.rpm

NB: I can only test building on i386, so I can't guarantee that the
arch-specific issues are truly gone.
Comment 20 Mary Ellen Foster 2007-08-01 11:40:42 EDT
Whoops, that's still the old SRPM; here's the new one:
http://www6.informatik.tu-muenchen.de/~foster/extras/ice/ice-3.2.0-7.fc7.lcg.src.rpm
Comment 21 Mamoru TASAKA 2007-08-01 13:54:13 EDT
Well again just rebuilt 
(I will check the rebuild failure later, however as I am also
 reviewing other persons' requests, I hope that you will fix the
 build failure before I try :) )

* i386, ppc okay
  http://koji.fedoraproject.org/koji/taskinfo?taskID=84848
  http://koji.fedoraproject.org/koji/taskinfo?taskID=84868

* x86_64 not okay, however some progress on build log.
  http://koji.fedoraproject.org/koji/taskinfo?taskID=84850

* ppc64 not okay. According to the build log, you may want to
  disable ppc64 support completely?
  http://koji.fedoraproject.org/koji/taskinfo?taskID=84824
Comment 22 Mamoru TASAKA 2007-08-13 10:57:42 EDT
ping?
Comment 23 Mary Ellen Foster 2007-08-16 11:35:02 EDT
Argh, dammit, I made those changes right after receiving your last message, but
I never actually uploaded them. *sigh* As a bonus, ZeroC has released version
3.2.1 in the meantime, so here's a new srpm and spec. (The website I normally
put these on is having trouble, so they're on a different server.)

http://homepages.inf.ed.ac.uk/mef/extras/ice-3.2.1-1.fc7.src.rpm
http://homepages.inf.ed.ac.uk/mef/extras/ice.spec
Comment 24 Mamoru TASAKA 2007-08-17 04:15:11 EDT
Currently I meet a strange buildroot failure on koji so I cannot
rebuild your new srpm and I am asking fedora-devel mailing list
why...
Comment 25 Mamoru TASAKA 2007-08-18 08:30:41 EDT
Still almost the same as comment 21:
* okay on i386, ppc:
  http://koji.fedoraproject.org/koji/taskinfo?taskID=108304
  http://koji.fedoraproject.org/koji/taskinfo?taskID=108308

* not okay on x86_64
  A little more fix seems to be needed
  http://koji.fedoraproject.org/koji/taskinfo?taskID=108306

* not okay on ppc64
  Maybe you want to disable ppc64 completely...
  http://koji.fedoraproject.org/koji/taskinfo?taskID=108319
Comment 26 Mary Ellen Foster 2007-08-20 04:30:28 EDT
Oh, argh, head slap -- the version I sent didn't actually have my ExcludeArch
fixes in it. Here you go:

http://homepages.inf.ed.ac.uk/mef/extras/ice-3.2.1-2.fc7.src.rpm
http://homepages.inf.ed.ac.uk/mef/extras/ice.spec
Comment 27 Mary Ellen Foster 2007-08-20 05:57:58 EDT
p.s. -- Is there any way for me to get my own Koji account so that I can catch
brainos like that myself without needing you to do a new build? Or do I need to
wait until I'm sponsored ...
Comment 28 Mamoru TASAKA 2007-08-20 07:25:46 EDT
Well, this time rebuild passed (ppc64 explicitly excluded)
http://koji.fedoraproject.org/koji/taskinfo?taskID=110075

I will check the result package later. As this package is large,
it may take several time...

(In reply to comment #27)
> p.s. -- Is there any way for me to get my own Koji account so that I can catch
> brainos like that myself without needing you to do a new build? Or do I need to
> wait until I'm sponsored ...
You have to wait...

Comment 29 Mamoru TASAKA 2007-08-20 07:27:48 EDT
Meantime you can check if the result binary rpms are as you expect.
The result binary rpms and build logs can be downloaded from:

http://koji.fedoraproject.org/scratch/mtasaka/task_110075/
Comment 30 Mamoru TASAKA 2007-08-20 11:14:24 EDT
For 3.2.1-2:

1. First, for spec file:
1-1: Description stage
A: ruby-libs dependency
   - This package has "BuildRequires: ruby-libs" and
     -ruby subpackage has "Requires: ruby(abi) = 1.8"

     For consistency (i.e. to avoid that this package is
     rebuild against ruby 1.9), IMO "BuildRequires: ruby-libs"
     should be replaced with "ruby(abi) = 1.8".

B. Release number specific dependency
   - Usually the dependency between main package and subpackages
     MUST be EVR (epoch-version-release) specific.
     i.e. for example -server subpackage must have:
--------------------------------------------------
Requires: ice = %{version}-%{release}
--------------------------------------------------

C. Dependency between subpackages
   - Please check if this is correct. For example,
     is the following correct?
     - -java subpackage does not require main package
     - -java-devel subpackage does not require -java subpackage
     - -csharp subpackage does not require main package

D. naming
   - Usually foo-devel package should have the corresponding
     package named foo.
     IMO -cxx-devel subpackage should just be named as
     "ice-devel".

E. pkgconfig dependency
   - Packages which contains pkgconfig .pc files must have
     "Requires: pkgconfig"

1-2: %files and scriptlets
A. User/group registry
   - Please refer to:
     http://fedoraproject.org/wiki/Packaging/UsersAndGroups

B. ldconfig
   - It does not seem that -c++-devel subpackage should call ldconfig
     on %post and %postun

C. Directory ownership issue
   - The following directories are not owned by any packages
-----------------------------------------------------
%{python_sitelib}/Ice/*/
-----------------------------------------------------

2. rpmlint complaint
A. script-without-shebang
-----------------------------------------------------
E: ice script-without-shebang /usr/share/Ice-3.2.1/slice/Freeze/Connection.ice
E: ice script-without-shebang /usr/share/Ice-3.2.1/slice/IceGrid/FileParser.ice
E: ice script-without-shebang /usr/share/Ice-3.2.1/slice/Freeze/Transaction.ice
E: ice script-without-shebang
/usr/share/Ice-3.2.1/slice/Glacier2/PermissionsVerifierF.ice
-----------------------------------------------------
   - Why do these scripts have executable permission?

B. non-standard-group
-----------------------------------------------------
W: ice-csharp non-standard-group System Environment/Libaries
------------------------------------------------------
   - Please fix typo

C. License tags
------------------------------------------------------
W: ice invalid-license GPL
------------------------------------------------------
   - License tag policy changed. Please fix according to:
     http://fedoraproject.org/wiki/Licensing
     http://fedoraproject.org/wiki/Packaging/LicensingGuidelines

D. macro-in-%changelog
-------------------------------------------------------
W: ice macro-in-%changelog ifnarch
W: ice macro-in-%changelog _lib
W: ice macro-in-%changelog _sbindir
W: ice macro-in-%changelog _libdir
W: ice macro-in-%changelog _prefix
--------------------------------------------------------
   - Use %% in the %changelog to prevent macros from being
     expanded. For example:
--------------------------------------------------------
* Wed Aug  1 2007 Mary Ellen Foster <mefoster at gmail.com> 3.2.0-7
- Fixed arch-specific issues:
  - %%ifnarch ppc64 in a lot of places; it doesn't have db4-java or mono-core, so 
    no Java or CSharp packages
  - Replaced one literal "lib" with %%{_lib}
--------------------------------------------------------

3. build log check
  - Still some compilation does not honor fedora specific compilation
    flags
    (build log also available under
     http://koji.fedoraproject.org/scratch/mtasaka/task_110075/ )
---------------------------------------------------------
   163  make[2]: Leaving directory `/builddir/build/BUILD/Ice-3.2.1/src/IceUtil'
   164  making all in icecpp
   165  make[2]: Entering directory `/builddir/build/BUILD/Ice-3.2.1/src/icecpp'
   166  cc -c -I../../include  -O2 -I. -DPREFIX=\"\" cccp.c
   167  cc -c -I../../include  -O2 -I. -DPREFIX=\"\" cexp.c
   168  cc -c -I../../include  -O2 -I. -DPREFIX=\"\" prefix.c
   169  rm -f ../../bin/icecpp
   170  cc -O2 -I. -DPREFIX=\"\" -o ../../bin/icecpp cccp.o cexp.o prefix.o
   171  make[2]: Leaving directory `/builddir/build/BUILD/Ice-3.2.1/src/icecpp'
----------------------------------------------------------

4. All source codes check (especially license issue check)
  - Well, this is extremely hard.. Actually there are (in total)
    10448 files (the most number in the packages I have reviewed...)

    I may skip this check during review, however if I find any issues
    I will report later.
Comment 31 Mary Ellen Foster 2007-08-22 08:49:46 EDT
(In reply to comment #30)

I'm working on this. I've made most of the changes, but I'd like some advice on
a couple of them.

> A: ruby-libs dependency
>    - This package has "BuildRequires: ruby-libs" and
>      -ruby subpackage has "Requires: ruby(abi) = 1.8"
> 
>      For consistency (i.e. to avoid that this package is
>      rebuild against ruby 1.9), IMO "BuildRequires: ruby-libs"
>      should be replaced with "ruby(abi) = 1.8".

You can't actually "BuildRequire: ruby(abi)" ... what I've got there right now
is consistent with my reading of the Ruby packaging guidelines
(http://fedoraproject.org/wiki/Packaging/Ruby). NB: I don't actually use Ruby ...

> D. naming
>    - Usually foo-devel package should have the corresponding
>      package named foo.
>      IMO -cxx-devel subpackage should just be named as
>      "ice-devel".

The thing is, the main ice package provides a large number of runtime files and
documentation, as well as a set of .so.* libraries (that the runtime files are
linked to). The c++-devel package adds two tools and a set of .h files and .so
links so that you can build Ice packages using c++ -- it is clearly a c++
development package, not an overall "-devel" package. I can see the argument for
calling it "ice-devel", but that's a bit confusing because that sounds like it's
*the* Ice development package when in truth it's really just for C++.

> 4. All source codes check (especially license issue check)
>   - Well, this is extremely hard.. Actually there are (in total)
>     10448 files (the most number in the packages I have reviewed...)
> 
>     I may skip this check during review, however if I find any issues
>     I will report later.

I've done some preliminary looking around. It seems that the majority of source
files are under the "Ice license", which is the GPLv2 with exceptions for
OpenSSL and Orca Robotics components. Here are some numbers (just simple "grep"
for now ...)

- 10448 files
- 6944 have the Ice license
- 13 have GPLv2 "or later"

A lot of the remainder are in the "test" or "demo" directories, which I don't
build at all -- is that an issue? There are also a lot of ".depend" files which
have no license information at all. And then there are the *.png/*.gif files in
the documentation ...

Comment 32 Mary Ellen Foster 2007-08-22 08:52:22 EDT
> - 10448 files
> - 6944 have the Ice license

Okay, whoops, forgot to subtract there: I think something like 3504 have the Ice
license.
Comment 33 Mamoru TASAKA 2007-08-22 10:56:12 EDT
(In reply to comment #31)
> (In reply to comment #30)
> > A: ruby-libs dependency
> >      For consistency (i.e. to avoid that this package is
> >      rebuild against ruby 1.9), IMO "BuildRequires: ruby-libs"
> >      should be replaced with "ruby(abi) = 1.8".
> 
> You can't actually "BuildRequire: ruby(abi)" ... what I've got there right now
> is consistent with my reading of the Ruby packaging guidelines
> (http://fedoraproject.org/wiki/Packaging/Ruby). 

Yes. Currently Fedora ruby guideline does not have
"BuildRequires: ruby(abi) = 1.8", however this is consistent with
python, perl, .... etc and I think this is needed.
Also another reviewer (who reviewed ruby module packages I maintain)
asked me to do so. 

> > D. naming
> >    - Usually foo-devel package should have the corresponding
> >      package named foo.
> >      IMO -cxx-devel subpackage should just be named as
> >      "ice-devel".
> 
> The thing is, the main ice package provides a large number of runtime files and
> documentation, as well as a set of .so.* libraries (that the runtime files are
> linked to). The c++-devel package adds two tools and a set of .h files and .so
> links so that you can build Ice packages using c++ -- it is clearly a c++
> development package, not an overall "-devel" package.

Please read
http://www.redhat.com/archives/rhl-devel-list/2007-August/msg00532.html
Actually
* Most people expects that the development package of ice
  is named as ice-devel
* And some people even think that splitting development packages
  are of no means.
Comment 34 Mary Ellen Foster 2007-08-22 11:59:16 EDT
Okay, I've made a new version with most of your concerns addressed. Here's the
changelog:
- Changed BuildRequires on ruby to ruby(abi) = 1.8
- Fixed all dependencies between subpackages: everything requires the base
  package, and -devel packages should all require their corresponding non-devel
  package now
- Made ice-csharp require pkgconfig
- Modified the user/group creation process based on the wiki
- Removed ldconfig for ice-c++-devel subpackage
- Made the python_sitelib subdirectory owned by ice-python
- Removed executable permission on all files under slice (how did that happen?)
- Fixed typo on ice-csharp group
- Changed license tag to GPLv2
- Removed macros in changelog
- Set CFLAGS as well as CPPFLAGS for make so that building icecpp gets the 
  correct flags too
- Renamed ice-c++-devel to ice-devel
- Added Provides: for ice-c++-devel and ice-dotnet for people moving from the
  ZeroC RPMs
- Also don't build "test" or "demo" for IceCS

http://homepages.inf.ed.ac.uk/mef/extras/ice-3.2.1-3.fc7.src.rpm
http://homepages.inf.ed.ac.uk/mef/extras/ice.spec
Comment 35 Mamoru TASAKA 2007-08-23 00:06:24 EDT
Rebuild failed.

http://koji.fedoraproject.org/koji/taskinfo?taskID=121392

Not checked in detail, however "Requires: ruby(abi) = 1.8" only
pulls ruby-libs and ruby package is not pulled.
Comment 36 Mary Ellen Foster 2007-08-23 07:51:30 EDT
Oh whoops -- guess I should be compiling with mock, not with rpmbuild, even if
it does take longer. *sigh*

Here are some new ones. In addition to the ruby fix, I also made the following
changes:
- Redirected getent output to /dev/null
- Really got rid of the execute bit on those .ice files
- Moved ImportKey.class to /usr/share from /usr/bin

http://homepages.inf.ed.ac.uk/mef/extras/ice-3.2.1-4.fc7.src.rpm
http://homepages.inf.ed.ac.uk/mef/extras/ice.spec

And this package actually *does* build in mock, for both Fedora 7 and Fedora
development (i386). :) The only remaining rpmlint issues are a bunch of
"no-documentation" warnings and the one error (apparently expected with Mono
packages) from ice-csharp-devel about only non-binary files in /usr/lib.
Comment 37 Mamoru TASAKA 2007-08-23 14:23:27 EDT
For -4:

* Redundant Requires
  - For -java-devel subpackage, "Requires: ice = %{version}-%{release}"
    is redundant because -java-devel requires -java subpackage
    and -java subpackage requires ice main package

* scriptlets
--------------------------------------------------------
getent group iceuser > /dev/null || groupadd -r iceuser
--------------------------------------------------------
  - Ah.. "groupadd" must be written in full path
    (%_sbindir/groupadd) otherwise this will cause problem
    when normal user do "su -c 'yum install'" or so.
    Please write groupadd, useradd and so on (i.e.
    commands which are not user normal users' path) in
    full path.
    I also asked Fedora-packaing to change wiki. 

* Directory ownership issue
  - Well, still for python subpackage this is not fixed.
    For example:
--------------------------------------------------------
[tasaka1@localhost ~]$ LANG=C rpm -qf
/usr/lib/python2.5/site-packages/Ice/Glacier2/__init__.py
ice-python-3.2.1-4.fc8
[tasaka1@localhost ~]$ LANG=C rpm -qf /usr/lib/python2.5/site-packages/Ice/Glacier2/
file /usr/lib/python2.5/site-packages/Ice/Glacier2 is not owned by any package
---------------------------------------------------------
    Please check the line
---------------------------------------------------------
%{python_sitelib}/Ice/*/*.py*
---------------------------------------------------------

    Note: When we write
----------------------------------------------------------
%files
foo/
----------------------------------------------------------
    (here foo/ is a directory), this contains the directory foo
    itself and all files/directories/etc under foo/, where
    when we write
-----------------------------------------------------------
%files
%dir foo/
------------------------------------------------------------
    this contains only foo/ itself.

    So, for example
------------------------------------------------------------
%files ruby
%defattr(644,root,root,755)
%dir %{ruby_sitearch}/Ice
%{ruby_sitearch}/Ice/*
------------------------------------------------------------
    is the same as
------------------------------------------------------------
%files ruby
%defattr(644,root,root,755)
%{ruby_sitearch}/Ice/
------------------------------------------------------------
    (it is my habit to add slash at last when the entry is
     a directory)

? /var/lib/icegrid
  - Well actually I don't know how to use ice at all, however
    anyway I tried:
-------------------------------------------------------------
[root@localhost ~]# service icegridregistry start
Starting icegridregistry: runuser: warning: cannot change directory to
/var/lib/icegrid: No such file or directory
/usr/bin/icegridregistry: failure occurred in daemon
                                                           [FAILED]
-------------------------------------------------------------
    While I don't think the failure is the issue (perhaps some
    configuration is needed beforehand), does this mean that
    %_localstatedir/lib/icegrid must be owned by some package?
Comment 38 Mamoru TASAKA 2007-08-24 02:40:01 EDT
(In reply to comment #37)
> For -4:

> * scriptlets
> --------------------------------------------------------
> getent group iceuser > /dev/null || groupadd -r iceuser
> --------------------------------------------------------
>   - Ah.. "groupadd" must be written in full path
>     (%_sbindir/groupadd) otherwise this will cause problem
>     when normal user do "su -c 'yum install'" or so.
>     Please write groupadd, useradd and so on (i.e.
>     commands which are not user normal users' path) in
>     full path.
>     I also asked Fedora-packaing to change wiki. 

Well, I asked on Fedora packaging and I was told that
it is okay that groupadd and so on can be only basename, not
full path.
Comment 39 Mary Ellen Foster 2007-08-24 10:58:50 EDT
Okay, here's another new version:

- Clean up packaging of icegridgui: it's a gui app, so we should treat it as
  such (NB: building this package is still disabled by default because it needs
  jgoodies)
- Actually create the working directory for the Ice services
- Remove redundant requires on java-devel and csharp-devel packages
- Fix file list for python package to own directories too
- Modified the README to accurately reflect what's in the Fedora package

Note that, with the python %files changes, I get a spew of "file listed twice"
errors because python_sitelib and python_sitearch are the same directory on my
i386 machine, but this doesn't stop anything from building. Note also that
building icegrid-gui is disabled at the moment because it depends on some
additional Java packages that aren't there yet. I've got packages that I'm
intending to submit soon, but for now I'd like to concentrate on getting the
guts of Ice in.

http://homepages.inf.ed.ac.uk/mef/extras/ice-3.2.1-5.fc7.src.rpm
http://homepages.inf.ed.ac.uk/mef/extras/ice.spec
Comment 40 Mary Ellen Foster 2007-08-24 11:01:54 EDT
Whoops, just noticed that I wasn't actually using python_sitearch anywhere, so I
just removed all references to it in the spec file. No more duplicate files.
SRPM and spec are in the same place.
Comment 41 Mamoru TASAKA 2007-08-24 13:19:50 EDT
Well, I have not checked -5 in detail, however:

From diff of -4 and -5:
-------------------------------------------------------
@@ -327,10 +350,8 @@
 done
 mkdir -p ${RPM_BUILD_ROOT}%{ruby_sitearch}/Ice
 mv $RPM_BUILD_ROOT/ruby/* ${RPM_BUILD_ROOT}%{ruby_sitearch}/Ice
-mkdir -p ${RPM_BUILD_ROOT}%{python_sitearch}/Ice
-mv ${RPM_BUILD_ROOT}/python/IcePy.so* ${RPM_BUILD_ROOT}%{python_sitearch}/Ice
 mkdir -p ${RPM_BUILD_ROOT}%{python_sitelib}/Ice
-cp -p %SOURCE7 $RPM_BUILD_ROOT%{python_sitelib}
+cp -p %{SOURCE7} $RPM_BUILD_ROOT%{python_sitelib}
 mv ${RPM_BUILD_ROOT}/python/* ${RPM_BUILD_ROOT}%{python_sitelib}/Ice
 
 mkdir -p ${RPM_BUILD_ROOT}%{_datadir}/Ice-%{version}
-----------------------------------------------------------------
Well, actually IcePy.so is arch-dependent binary and this
must be put under python_sitearch, not python_sitelib.

(In reply to comment #39)
> Note that, with the python %files changes, I get a spew of "file listed twice"
> errors because python_sitelib and python_sitearch are the same directory on my
> i386 machine,

Yes, this has always been a issue when both sitelib and sitearch are
needed. Perhaps the following will work.
-----------------------------------------------------
%files python
%defattr(644,root,root,755)
%ifarch x86_64 ppc64
%{python_sitearch}/Ice/
%endif
%{python_sitelib}/Ice/
%{python_sitelib}/%{name}.pth
------------------------------------------------------
Comment 42 Mary Ellen Foster 2007-08-27 11:45:04 EDT
Okay, I fixed the Python stuff to put IcePy.so* into sitearch and the rest into
sitelib, and added the ifarch to the %files list.

I also modified the CSharp build process so that it actually uses gacutil to
install instead of faking it. The net result is the same, but it's neater.

The Mono packaging spec on the Wiki is ambiguous about whether dlls should go
into %{_libdir} or %{_prefix}/lib. In this rpm, they go into %{_libdir} for now
... NB: I've asked about this on fedora-packaging.

http://homepages.inf.ed.ac.uk/mef/extras/ice-3.2.1-6.fc7.src.rpm
http://homepages.inf.ed.ac.uk/mef/extras/ice.spec
Comment 43 Mamoru TASAKA 2007-08-27 12:29:53 EDT
Well, this time rebuild failed (at least on x86_64)

http://koji.fedoraproject.org/koji/taskinfo?taskID=131964
Comment 44 Mary Ellen Foster 2007-08-27 12:38:38 EDT
Argh, dammit, that'll fail on the other architectures too. Hopefully this fixes it.

http://homepages.inf.ed.ac.uk/mef/extras/ice-3.2.1-7.fc7.src.rpm
http://homepages.inf.ed.ac.uk/mef/extras/ice.spec
Comment 45 Mamoru TASAKA 2007-08-27 13:18:56 EDT
(In reply to comment #44)

> http://homepages.inf.ed.ac.uk/mef/extras/ice-3.2.1-7.fc7.src.rpm
> http://homepages.inf.ed.ac.uk/mef/extras/ice.spec

Would you check the permission? Accessing this srpm returns 403
----------------------------------------------------
You don't have permission to access /mef/extras/ice-3.2.1-7.fc7.src.rpm on this
server.
---------------------------------------------------
Comment 46 Mary Ellen Foster 2007-08-27 16:08:42 EDT
oops, try now.
Comment 47 Mamoru TASAKA 2007-08-27 21:51:55 EDT
This time rebuild fails at a different point?

http://koji.fedoraproject.org/koji/taskinfo?taskID=132360
Comment 48 Mary Ellen Foster 2007-08-28 04:07:17 EDT
Hmm. That isn't a failure I recognise ... could you try it again? It rebuilds
fine for me in mock on my machine. (Not against development when I just tried,
but that looks more like yum crapping out rather than a build problem.)
Comment 49 Mamoru TASAKA 2007-08-28 05:00:21 EDT
Well, this time rebuild succeeded on x86_64 and ppc, failed on i386
http://koji.fedoraproject.org/koji/taskinfo?taskID=133694
http://koji.fedoraproject.org/koji/taskinfo?taskID=133707
http://koji.fedoraproject.org/koji/taskinfo?taskID=133696

And the previous time, it failed on x86_64
http://koji.fedoraproject.org/koji/taskinfo?taskID=132362

Now I guess this is due to parallel make problem.
Would you try to drop parallel make support and re-upload new srpm?
Comment 50 Mary Ellen Foster 2007-08-28 05:16:12 EDT
Parallel make disabled. Weird, wonder why this hasn't shown up before now ...

http://homepages.inf.ed.ac.uk/mef/extras/ice-3.2.1-8.fc7.src.rpm
http://homepages.inf.ed.ac.uk/mef/extras/ice.spec
Comment 51 Mamoru TASAKA 2007-08-28 08:51:48 EDT
This time rebuild passed.

It is unsure if I can check the results today (in Japan = UTC+09h)
Meantime please check if the results meets what you expect.
Comment 52 Mamoru TASAKA 2007-08-28 08:53:58 EDT
(In reply to comment #51)
> Meantime please check if the results meets what you expect.

Oops.. The results are under:
http://koji.fedoraproject.org/koji/taskinfo?taskID=133932
http://koji.fedoraproject.org/scratch/mtasaka/task_133932/
Comment 53 Mamoru TASAKA 2007-08-29 09:37:48 EDT
==============================================
        LICENSE CHECK
==============================================
For tar files except for "rpmbuild"

Total files number 10434

* ICE_LICENSE                    3504
* Binary .class file (not used)  5612
* jpg/png/gif                     153
* .depend                         166
* .ice                             54
* GPL                              28
-------------------------------------
  Total                          9517
  Left                            917

  I checked the left 917 files manually and they
  are all okay (some of them are demo files or so
  and are never used...)

  * However, as Ice accepts some exceptions for
    linage, please tag the license as
    "GPLv2 with exceptions".

Okay.
==============================================
  This package (ice) is APPROVED by me
==============================================
Please follow the procedure according to:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Get a Fedora Account".
At a point a mail should be sent to sponsor members which notifies
that you need a sponsor (at the stage, please also write on
this bug for confirmation that you requested for sponsorship)
Then I will sponsor you.

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

If you have questions, please ask me.

Comment 54 Mamoru TASAKA 2007-08-29 09:40:56 EDT
(In reply to comment #53)
>   * However, as Ice accepts some exceptions for
>     linage, please tag the license as
>     "GPLv2 with exceptions".

s/linage/linkage/
Comment 55 Mary Ellen Foster 2007-08-29 12:13:52 EDT
(In reply to comment #53)
>   I checked the left 917 files manually and they
>   are all okay (some of them are demo files or so
>   and are never used...)

Wow! I'm impressed ... thanks for that, and for accepting the package.

I made the license change. Meanwhile, I only just now noticed that I'd been
putting the Ruby stuff into a subdirectory where it didn't actually work, so I
fixed that.

On rereading the Ruby packaging guidelines, I also added Provides: Ruby(Ice) =
%{version} to the ruby package. (According to those guidelines, the package
"must" also apparently be called "ruby-ice", not "ice-ruby", but I don't really
want to do that.)

Don't know if you want to look over these changes too ...

http://homepages.inf.ed.ac.uk/mef/extras/ice-3.2.1-9.fc7.src.rpm
http://homepages.inf.ed.ac.uk/mef/extras/ice.spec
Comment 56 Mamoru TASAKA 2007-08-29 12:39:11 EDT
(In reply to comment #55)
 On rereading the Ruby packaging guidelines, I also added Provides: Ruby(Ice) =
> %{version} to the ruby package. (According to those guidelines, the package
> "must" also apparently be called "ruby-ice", not "ice-ruby", but I don't really
> want to do that.)

Well, actually I don't know correctly what this part of Ruby packaging
guidelines tries to say.
IMO we can follow python module naming guideline:
---------------------------------------------------------
Addon Packages (python modules)

Packages of python modules (thus they rely on python as a parent) use a slightly
different naming scheme. They should take into account the upstream name of the
python module. This makes a package name format of python-$NAME. When in doubt,
use the name of the module that you type to import it in a script.
----------------------------------------------------------
For this package, the parent package of ice-ruby is surely
ice, so the name should be ice-ruby, not ruby-ice IMO. Also
I am not sure whether ice-ruby should provide "ruby(Ice) = 1.8"
(although it should be safe that ice-ruby provides it).

Anyway please follow the procedure written on
http://fedoraproject.org/wiki/PackageMaintainers/Join
Comment 57 Mary Ellen Foster 2007-08-30 03:30:49 EDT
New Package CVS Request
=======================
Package Name: ice
Short Description: Object middleware
Owners: mef
Branches: F-7
InitialCC: 
Cvsextras Commits: yes
Comment 58 Mamoru TASAKA 2007-08-30 07:26:32 EDT
(Removing NEEDSPONSOR, I am now sponsoring)
Comment 59 Jens Petersen 2007-08-30 21:39:03 EDT
cvs request done
Comment 60 Mamoru TASAKA 2007-08-31 01:36:18 EDT
One note:
When rebuild is done, you can close this bug as NEXTRELEASE.
Then this review request finishes. After that, as currently
Ice does not support ppc64, please
* create a new bug report with component "Ice" which tells that
  currently Ice does not support ppc64.
* And make the bug block bug 238953.
Comment 61 Mamoru TASAKA 2007-09-05 13:35:58 EDT
For Fedora 7, when you think it is okay, please request to move
from testing to stable using bodhi.

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