Bug 498736 - Review Request: ucommon - Portable C++ runtime for threads and sockets
Summary: Review Request: ucommon - Portable C++ runtime for threads and sockets
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 499137
TreeView+ depends on / blocked
 
Reported: 2009-05-02 16:48 UTC by David Sugar
Modified: 2013-08-13 12:41 UTC (History)
7 users (show)

Fixed In Version: 2.0.5-4.fc10
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-05-15 23:33:27 UTC
Type: ---
Embargoed:
mtasaka: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description David Sugar 2009-05-02 16:48:12 UTC
Spec URL: http://www.gnutelephony.org/specs/ucommon.spec
SRPM URL: http://www.gnutelephony.org/dist/redhat/ucommon-2.0.5-0.src.rpm
Description: uCommon is a lightweight C++ library to facilitate using C++ design patterns even for very deeply embedded applications, such as for systems using uClibc along with POSIX threading support. For this reason, UCommon disables language features that consume memory or introduce runtime overhead. UCommon introduces some design patterns from Objective-C, such as reference counted objects, memory pools, and smart pointers. UCommon introduces some new concepts for handling of thread locking and synchronization.

ADDITIONAL NOTE: This is a new package, it is being introduced as part of a set of what will be in total 4 package submissions, that will include libosip2, libeXosip2, and sipwitch which collectively offers a means to construct private and public secure voip telephone networks.  As this is my first package, I am also looking for a sponsor.

Comment 1 Peter Lemenkov 2009-05-02 17:30:11 UTC
btw, both libeXosip2 and libosip2 are already included into Fedora:

[petro@Sulaco ~]$ yum list *osip*
Loaded plugins: fastestmirror
Available Packages
compat-libosip2.ppc                                                                                                                                              2.2.2-16.fc9                                                                                                                                         fedora
compat-libosip2-devel.ppc                                                                                                                                        2.2.2-16.fc9                                                                                                                                         fedora
libeXosip2.ppc                                                                                                                                                   3.1.0-1.fc9                                                                                                                                          fedora
libeXosip2-devel.ppc                                                                                                                                             3.1.0-1.fc9                                                                                                                                          fedora
libosip2.ppc                                                                                                                                                     3.1.0-1.fc9                                                                                                                                          fedora
libosip2-devel.ppc                                                                                                                                               3.1.0-1.fc9                                                                                                                                          fedora
[petro@Sulaco ~]$

Comment 2 Mamoru TASAKA 2009-05-02 17:40:07 UTC
Some random comments after just glancing at your
spec file:

- First of all please make check that if this is "ucommon"
  or "commoncpp". Note that commoncpp is already in Fedora.
- don't define %version, %release. These macros are defined
  automatically.
- And why do you want to define %uses_stdcpp ?
- Now Fedora suggests to use %global instead of %define:
  https://fedoraproject.org/wiki/Packaging/Guidelines#.25global_preferred_over_.25define
- Duplicating package name in Summary is regarded as
  redundant on Fedora.
- Please use license tag valid on Fedora:
  https://fedoraproject.org/wiki/Packaging/LicensingGuidelines
  https://fedoraproject.org/wiki/Licensing
- BR: gcc-c++ is redundant:
  https://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2
- Please don't use "Packager" item. On Fedora this is automatically
  defined by buildsys.
- Please use tarball name directly as main package name and
  ucommon-devel as development package name. Fedora's naming
  rule is different from debian.
  https://fedoraproject.org/wiki/Packaging/NamingGuidelines#General_Naming
- Please write %changelog at last and actually write something
  on %changelog. ref:
  https://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs
- %setup should be quiet. Please use "%setup -q".
- Please don't use %makeinstall unless avoided:
  https://fedoraproject.org/wiki/Packaging/Guidelines#Why_the_.25makeinstall_macro_should_not_be_used
- Don't strip binaries. Debug information is needed to create
  debuginfo rpm:
  https://fedoraproject.org/wiki/Packaging/Debuginfo
- Please check if document files such as "INSTALL" "BUILDS"
  are needed. 
  I have not read these files (as I am now just watching your spec file)
  but these files are usually for people who want to compile/install
  packages by themselves and not needed for people using rpm.
- Don't ship static archive or libtool .la files unless needed
  by some reason:
  https://fedoraproject.org/wiki/Packaging/Guidelines#Packaging_Static_Libraries
- Package shipping pkgconfig .pc file should have
  "Requires: pkgconfig":
  https://fedoraproject.org/wiki/Packaging/Guidelines#Pkgconfig_Files

Comment 3 David Sugar 2009-05-02 17:57:35 UTC
1.  ucommon is NOT the same as commoncpp.  Both can also co-exist.  ucommon is technically a successor package to commoncpp.

2. I use a macro for that because ucommon can be built in a deeply embedded profile without using libstdc++, so if in theory one uses to spec to cross-compile such a target, one can force libstdc++ off. Incidentally, related to this, unfortunately libtool forces libstdc++ into a build even if you neither use nor want it, even if otherwise --nostdc is specified for example.  To get around this, some special build magic is done to link in libtool under "C" profile.  Is there a better way to do this?

3. I define %version and %release this way because I have a build script to override (autogenerate) values for a rpmbuild through the --define option (also why I have %uses_...).  These can be killed of course to normalize the spec file :).  Hence my goal was to have a single spec I could use internally as well as externally.  But easy enough to redefine by normal conventions :)

4. Remaining items look also easy enough to resolve.  How do I submit an updated spec file?

Comment 4 David Sugar 2009-05-02 18:12:52 UTC
Yes (exosip/osip)...those are also current enough, too!  Then the only other package I will need to submit is sipwitch, keeping track of the issues noted here too :).

Comment 5 Mamoru TASAKA 2009-05-02 18:20:31 UTC
(In reply to comment #3)
> 1.  ucommon is NOT the same as commoncpp.  Both can also co-exist.  ucommon is
> technically a successor package to commoncpp.
- The reason I am asking you about this is that I see the
  term "commoncpp" in URL.
  I guess the upstream URL for this is
  http://www.gnutelephony.org/index.php/UCommon

> 2. I use a macro for that because ucommon can be built in a deeply embedded
> profile without using libstdc++, so if in theory one uses to spec to
> cross-compile such a target, one can force libstdc++ off.
- Well, okay.
 
> 3. I define %version and %release this way because I have a build script to
> override (autogenerate) values for a rpmbuild through the --define option (also
> why I have %uses_...).  
- Please don't do this but directly define version, release
  in Version, Release.

> 4. Remaining items look also easy enough to resolve.  How do I submit an
> updated spec file?  
- Upload the new spec file and new srpm on somewhere.
  spec file can be overwritten on the URL you used on the original
  post, however please make it sure that you change the Release
  number every time you modify your spec file to avoid confusion
  (so the filename of srpm will change every time you modify
   your spec file).

Comment 6 David Sugar 2009-05-02 18:42:25 UTC
Yes, we share the same upstream.  Actually there was also a request to change the package name to ucommon because of some trademark dispute with someone who (much later, as in mid 2006) created an entirely new package they also called "Common C++" (GNU Common C++ has been in continual use since 1999/2000), and the FSF does not wish to fight the issue. Hence all upstreams will likely migrate to the ucommon name at some point...

I have uploaded updates that should cover all initial issues raised:

http://www.gnutelephony.org/specs/ucommon.spec
http://www.gnutelephony.org/specs/ucommon-2.0.5-1.src.rpm

I also put what will be the sipwitch spec I will submit up there to, after covering the issues raised here in that one also in advance:

http://www.gnutelephony.org/specs/sipwitch.spec
http://www.gnutelephony.org/specs/ucommon-2.0.5-1.src.rpm

That one is more complicated, as it has swig and plugins...

Comment 7 David Sugar 2009-05-02 18:43:36 UTC
Opps...wrong url for sipwitch src rpm:

http://www.gnutelephony.org/specs/sipwitch-0.5.4-0.src.rpm

Comment 8 Susi Lehtola 2009-05-03 06:56:47 UTC
- Don't ship static libraries. Try adding --disable-static to %configure. If you can't get static libraries not to build, your development package needs to Provides: %{name}-static = %{version}-%{release}.

- The dependency of the devel package on the main package must be fully versioned: Requires: %{name} = %{version}-%{release}

- Instead of having
 %dir %{_includedir}/ucommon
 %{_includedir}/ucommon/*.h
in the devel file section just put simply
 %{_includedir}/ucommon/
as this will include the directory and all the files in it. You can also leave out the trailing slash but having it makes the statement a bit clearer.

- Do you really need to set
 %attr(0755,root,root) %{_bindir}/ucommon-config
won't a simple
 %{_bindir}/ucommon-config
do?

- You have the Group attribute set twice on the main package.

- %configure automatically sets CXXFLAGS="$RPM_OPT_FLAGS", you don't need that.

Comment 9 David Sugar 2009-05-03 13:53:16 UTC
That is a rather interesting question.  Traditionally I have always shipped static libs as part of the -devel package just in case someone does actually want to build static linked, although I imagine is likely extremely rare to see this happen anymore for anything produced for a mainline distro (embedded on the other hand certainly...) outside I think of perhaps those who are creating proprietary/source secret binaries and I have no interest in that myself.  Shipping static libs as a separate -static package seems acceptable to me, just like the debug package is separate from devel.

The reason I set attr for ucommon-config is that automake/autoconf generated Make kept dropping the execute attribute when installing that file.

All these changes I will add.  I recall that historically %configure only had set CFLAGS with RPM_OPT_FLAGS, and NOT CXXFLAGS (hence C++ never by default built with optimizations).  I gather this has changed...

I like all of these comments and changes...I will also feed them back into the tarball generated spec upstream...

Updated:

http://www.gnutelephony.org/specs/ucommon-2.0.5-2.src.rpm
http://www.gnutelephony.org/specs/ucommon.spec

Comment 10 Mamoru TASAKA 2009-05-03 18:26:46 UTC
Assigning.

Before doing full-review
- Why is %install section empty this time?
- Please consider to use %?dist tag:
  https://fedoraproject.org/wiki/Packaging/DistTag
- Is this the expected behavior on ucommon-config.in?
----------------------------------------------------------------
    16  if [ "`ldd /bin/sh | grep lib64`" = "" ]
    17  then
    18      libdir=${exec_prefix}/lib
    19  else
    20      libdir=${exec_prefix}/lib64
    21  fi
....
....
    56      case "$1" in
    57      --prefix=*)
    58          prefix=$optarg
    59          includedir=$prefix/include
    60          libdir=$prefix/lib
    61          ;;
----------------------------------------------------------------
  Here when --prefix or --exec_prefix is specified, libdir
  will always points to XXXXX/lib even on 64 bits architecture.

- Please follow Fedora's %changelog format:
  https://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs

! For sipwitch please submit a new review request for the
  package and make this review request block the review
  request for sipwitch.

Comment 11 David Sugar 2009-05-03 19:33:34 UTC
Result of too much last minute editing :)....

Added %?dist...and "%{__make} DESTDIR=%{buildroot}/ install" for %install.

Updated %changelog entry to correct format...

I will do the sipwitch one later today (or in the week) as you suggest.

As for ucommon-config.in...hmm...that one I have to think about a bit.  I actually use pkgconfig, so I do not normally think about ucommon-config :), and passing --prefix is perhaps a less common use case.  This may be an issue to resolve for the next upstream patch release...

Updated files:

http://www.gnutelephony.org/specs/ucommon.spec
http://www.gnutelephony.org/specs/ucommon-2.0.5-3.src.rpm

There was a time I used to create a lot of rpm packages, but that was years ago...

Comment 12 David Sugar 2009-05-03 20:26:53 UTC
rpmlint gave me a couple of useful additional things:

ucommon.src: E: no-cleaning-of-buildroot %install
ucommon.src: W: non-standard-group System/Libraries

Comment 13 Mamoru TASAKA 2009-05-04 17:57:35 UTC
For 2.0.5-3 (after actually rebuilding this package)

* Group:
  - For this package: "System Environment/Libraries" is suggested.

* Cleaning buildroot at %install
  - At %install, %buildroot should be cleaned out first
    (example by:
--------------------------------------------------------------
%install
rm -rf %{buildroot}
make install \
	DESTDIR=%{buildroot} INSTALL="install -p" \
	install
--------------------------------------------------------------

* Timestamp
  - Please use
--------------------------------------------------------------
make install DESTDIR=%{buildroot}/ INSTALL="install -p" install
--------------------------------------------------------------
    to keep timestamps on installed files (especially on
    installed header files).
    This method (i.e. INSTALL="install -p") usually works
    for Makefiles generated from recent autotools.

* build error
  - -3 does not build:
    http://koji.fedoraproject.org/koji/taskinfo?taskID=1335547
    You have to remove libtool .la file at %install explicitly.

* Some rpmlint issue
--------------------------------------------------------------
ucommon-doc.i586: E: zero-length /usr/share/doc/ucommon-doc-2.0.5/a00086_f713234fbd42ca1e50a2436c591bfa19_cgraph.map
ucommon-doc.i586: E: zero-length /usr/share/doc/ucommon-doc-2.0.5/a00022_bbc045601d13615d2284212f7b491bc0_cgraph.map
ucommon-doc.i586: E: zero-length /usr/share/doc/ucommon-doc-2.0.5/a00086_1290f048a8f9116ba2aafaafe228e702_cgraph.map
ucommon-doc.i586: E: zero-length /usr/share/doc/ucommon-doc-2.0.5/a00086_d20f9bc12070f452b6b872d21736174a_cgraph.map
.....
.....
--------------------------------------------------------------
  - Are these empty map files needed? Or is it correct that these
    empty files are created?

! Some notes:
(In reply to comment #9)
> That is a rather interesting question.  Traditionally I have always shipped
> static libs as part of the -devel package just in case someone does actually
> want to build static linked, although I imagine is likely extremely rare to see
> this happen anymore for anything produced for a mainline distro (embedded on
> the other hand certainly...) outside I think of perhaps those who are creating
> proprietary/source secret binaries and I have no interest in that myself. 
> Shipping static libs as a separate -static package seems acceptable to me, just
> like the debug package is separate from devel.
  - If you think so, I suggest to remove static archive completely.

> The reason I set attr for ucommon-config is that automake/autoconf generated
> Make kept dropping the execute attribute when installing that file.
  - Personally, I would set permission at %install rather than using %attr 
    like
-----------------------------------------------------------------
%install
....
....
chmod 0755 %{buildroot}%{_bindir}/ucommon-config
-----------------------------------------------------------------
    (except the case where using %attr is unavoidable like
     %attr(2755,root,games))

! Note 2
  - Just noting again that for sipwitch please submit a seperate
    review request for sipwitch.

Comment 14 David Sugar 2009-05-05 08:18:10 UTC
I have removed static libs and I think resolved all other issues here, other than the zero length files doxygen generates over there.  I do not get those when I build ucommon on my box with rpmlint ucommon-doc-2.0.5-4.i386.rpm so I am unsure of that issue and it seems entirely related to how doxygen works.  rpm -qlp only shows me there are a lot of _cgraph.map files produced by it :).  If I were to take a guess, maybe it did not like some specific doxygenated header entry and created an empty callgraph for it.  Hence, it is a bug to be found and resolved in the source distribution, but certainly not a critical one.

I also recall I long ago added 

%debug_package %{nil}
%_unpackaged_files_terminate_build 0
%_missing_doc_files_terminate_build 0

To my own ~/.rpmmacros a long time ago.  This kept me from seeing a few things locally :).  I will work on sipwitch next (submitted under a new bug report) now that this seems to be getting finalized.

Updated Files:

http://www.gnutelephony.org/specs/ucommon.spec
http://www.gnutelephony.org/specs/ucommon-2.0.5-4.src.rpm

Comment 15 Mamoru TASAKA 2009-05-06 18:10:13 UTC
This package itself seems good.
Then when you clean up sipwitch srpm (on bug 499137), I will
approve this package and sponsor you.

Comment 16 Mamoru TASAKA 2009-05-07 16:59:37 UTC
Now I approve this package.
-----------------------------------------------------------
   This package (ucommon) 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/11, 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 David Sugar 2009-05-08 17:22:10 UTC
I created my accounts, certificates, etc...but before I play with anything I will want to look over the existing wiki pages further :).

Comment 18 Mamoru TASAKA 2009-05-08 17:28:07 UTC
Now I am sponsoring you. Please follow "Join" wikin again.

Comment 19 David Sugar 2009-05-10 22:04:49 UTC
New Package CVS Request
=======================
Package Name: ucommon
Short Description: Portable C++ runtime for threads and sockets 
Owners: dyfet
Branches: F-10 F-11
InitialCC: mtasaka

Comment 20 Kevin Fenzi 2009-05-13 05:08:06 UTC
cvs done.

Comment 21 Mamoru TASAKA 2009-05-13 18:13:23 UTC
It seems that you are now seeing that your srpm won't build
on dist-f12. This issue is now being discussed on:

https://www.redhat.com/archives/fedora-devel-list/2009-May/msg01062.html

Comment 22 Fedora Update System 2009-05-13 22:47:34 UTC
ucommon-2.0.5-4.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/ucommon-2.0.5-4.fc10

Comment 23 Fedora Update System 2009-05-13 22:49:40 UTC
ucommon-2.0.5-4.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/ucommon-2.0.5-4.fc11

Comment 24 Fedora Update System 2009-05-15 23:33:21 UTC
ucommon-2.0.5-4.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 25 Fedora Update System 2009-05-15 23:34:49 UTC
ucommon-2.0.5-4.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 26 Mamoru TASAKA 2009-05-16 06:15:23 UTC
Now ucommon should build also on dist-f12, please try.

Comment 27 David Sugar 2009-05-16 11:36:26 UTC
checking now.  If it builds, do I do a "make update" in devel to push it through so I can build sipwitch?  I recall seeing NOT to do so for devel here (http://fedoraproject.org/wiki/PackageMaintainers/Join#Check_out_the_module), but if not, then how is it made available as a dependency for building sipwitch in rawhide?

Comment 28 David Sugar 2009-05-16 12:07:21 UTC
Never mind.  sipwitch built fine too now in rawhide/f12.

Comment 29 Mamoru TASAKA 2009-05-16 13:50:25 UTC
Okay, thank you.

Comment 30 manuel wolfshant 2013-07-29 05:16:16 UTC
Package Change Request
======================
Package Name: ucommon
New Branches: el6
Owners: wolfy

Notes: needed to upgrade libzrtpcpp in order to fix https://bugzilla.redhat.com/show_bug.cgi?id=980894

Comment 31 Gwyn Ciesla 2013-07-29 13:07:40 UTC
Any comment from the Fedora maintainer?

Comment 32 David Sugar 2013-07-29 13:19:28 UTC
I am completely comfortable with him maintaining the epel branch

Comment 33 Gwyn Ciesla 2013-07-30 11:55:47 UTC
Git done (by process-git-requests).

Comment 34 manuel wolfshant 2013-08-12 23:46:33 UTC
Package Change Request
======================
Package Name: ucommon
New Branches: el5
Owners: wolfy

Notes: 
- needed to upgrade libzrtpcpp in order to fix https://bugzilla.redhat.com/show_bug.cgi?id=980894
- the owner already agreed to allow me to maintain the EPEL branch ( #32 ) and helped in patching the code to make it compatible with the older compiler

Comment 35 Gwyn Ciesla 2013-08-13 12:41:49 UTC
Git done (by process-git-requests).


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