Bug 471829 - Review Request: log4cxx - Log4cxx - a port to C++ of the Log4j project
Summary: Review Request: log4cxx - Log4cxx - a port to C++ of the Log4j project
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-11-17 02:11 UTC by Hayden James
Modified: 2011-10-24 10:25 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-12-02 13:42:26 UTC
Type: ---
Embargoed:
mtasaka: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Hayden James 2008-11-17 02:11:24 UTC
Spec URL: http://hayden.doesntexist.com/~hjames/log4cxx.spec
SRPM URL: http://hayden.doesntexist.com/~hjames/log4cxx-0.10.0-1.fc9.src.rpm
Description: Log4cxx is a popular logging package written in C++. One of its distinctive features is the notion of inheritance in loggers. Using a logger hierarchy it is possible to control which log statements are output at arbitrary
granularity. This helps reduce the volume of logged output and minimize the
cost of logging.

Comment 1 Hayden James 2008-11-26 11:14:06 UTC
Hello, anyone there? (*crickets chirping*)

Comment 2 Jason Tibbitts 2008-11-26 16:50:09 UTC
Dude, there are seven hundred pending package review tickets.  Please be patient.

Comment 3 Mamoru TASAKA 2008-11-27 07:47:44 UTC
Fails to build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=954051

By the way as you are already sponsored, you can check if your package
actually builds using koji as below:

$ koji build --scratch <target> <srpm_you_want_to_try>
where <target> can be dist-f11, dist-f{10,9,8}-updates-candidate and so on.
If the build is successful, the built binary rpms and some logs are
saved (about one week) on
http://koji.fedoraproject.org/scratch/<your_FAS_name>/task_<task_id>/ .

Comment 4 Hayden James 2008-11-27 14:50:04 UTC
Thanks for the tip, that helped a lot. Here's the updated files:

http://hayden.doesntexist.com/~hjames/log4cxx.spec
http://hayden.doesntexist.com/~hjames/log4cxx-0.10.0-2.fc9.src.rpm

Comment 5 Mamoru TASAKA 2008-11-27 17:06:30 UTC
Well,

* Summary
  - Usually on Summary beginning with the package name 
   (Log4cxx -) is redundant

* Group
  - Usually this type of package has "System Environment/Libraries"
    Group for main package.

* SourceURL
  - For SourceURL, using %name, %version (especially %version) is
    recommended:
    https://fedoraproject.org/wiki/Packaging/SourceURL#Using_.25.7Bversion.7D

* rpath
  - Using chrpath to remove rpath should be considered as the
    last resort:
    https://fedoraproject.org/wiki/Packaging/Guidelines#Removing_Rpath
    Please don't use chrpath unless avoidable (and usually it
    is avoidable).
    * I usually use
--------------------------------------------------------------
sed -i.libdir_syssearch -e \
	'/sys_lib_dlsearch_path_spec/s|/usr/lib |/usr/lib /usr/lib64 /lib /lib64 |' \
	configure
--------------------------------------------------------------
      and this works for this package

* Timestamp
  - Please consider to use
--------------------------------------------------------------
make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p"
--------------------------------------------------------------
    to keep timestamps on installed files. This method usually works
    for Makefiles generated by recent autotools.

* Installed doxygen html files
  - All these files should be marked as %doc. Moreover, I suggest
    that these html files must be moved under %_datadir/doc/

* Macros
  - Requires: log4cxx = %{version}-%{release},pkgconfig
    I suggest to use %name for consistency

* Requires for -devel subpackage
  - For example %_includedir/%name/helpers/aprinitializer.h contains:
--------------------------------------------------------------
    25  #include <log4cxx/helpers/pool.h>
    26  #include <apr_pools.h>
    27  #include <apr_thread_proc.h>
--------------------------------------------------------------
    This file requires apr-devel.

Comment 6 Hayden James 2008-11-27 19:28:24 UTC
Here's an updated version that fixes most of the above:

http://hayden.doesntexist.com/~hjames/log4cxx.spec
http://hayden.doesntexist.com/~hjames/log4cxx-0.10.0-3.fc9.src.rpm

rpmlint generates an error now with the above sed script to remove the rpaths instead of using chrpath.

Also, I didn't move the doxygen files as of yet, there's no way to do it that I can see without manually 'mv' the files, do you think it should be done anyhow?

Everything else should be fixed.

Comment 7 Mamoru TASAKA 2008-11-29 17:08:08 UTC
(In reply to comment #6)
> rpmlint generates an error now with the above sed script to remove the rpaths
> instead of using chrpath.

  - Please just ignore it.

> Also, I didn't move the doxygen files as of yet, there's no way to do it that I
> can see without manually 'mv' the files, do you think it should be done anyhow?
> 
> Everything else should be fixed.

  - Moving files manually meets the demand.

By the way
* Requires
  - It does not seem that -devel subpackage needs "Requires: apr-util-devel".
    Would you check this?

Comment 9 Mamoru TASAKA 2008-12-01 16:25:57 UTC
Well,
- Now the directory %{_defaultdocdir}/%{name}-%{version}/ is not owned
  by any packages, which must be owned by log4cxx package.

! By the way, instead of moving files to $RPM_BUILD_ROOT%{_defaultdocdir}/%{name}-%{version},
  the following method is simpler.

------------------------------------------------------------------
%install
rm -rf $RPM_BUILD_ROOT
make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p"
mv $RPM_BUILD_ROOT%{_datadir}/%{name}/html .

%files
%defattr(-,root,root,-)
%{_libdir}/liblog4cxx.so.10.0.0
%{_libdir}/liblog4cxx.so.10

%doc NOTICE
%doc LICENSE
%doc KEYS
....

%files devel
%defattr(-,root,root,-)
....
....
%doc html/
------------------------------------------------------------

Comment 11 Mamoru TASAKA 2008-12-01 18:30:44 UTC
--------------------------------------------------------
   This package (log4cxx) is APPROVED by mtasaka
--------------------------------------------------------

Comment 12 Hayden James 2008-12-01 18:35:01 UTC
New Package CVS Request
=======================
Package Name: log4cxx
Short Description: A port to C++ of the Log4j project (logging library)
Owners: hjames
Branches: F-9 F-10
InitialCC: mtasaka

Comment 13 Kevin Fenzi 2008-12-01 20:45:06 UTC
cvs done.

Comment 14 Fedora Update System 2008-12-01 23:32:31 UTC
log4cxx-0.10.0-5.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/log4cxx-0.10.0-5.fc9

Comment 15 Fedora Update System 2008-12-01 23:33:19 UTC
log4cxx-0.10.0-5.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/log4cxx-0.10.0-5.fc10

Comment 16 Mamoru TASAKA 2008-12-02 13:42:26 UTC
Okay, thanks.

Comment 17 Fedora Update System 2008-12-03 01:18:26 UTC
log4cxx-0.10.0-5.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 18 Fedora Update System 2008-12-03 01:23:23 UTC
log4cxx-0.10.0-5.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 19 Jose Pedro Oliveira 2011-08-10 23:23:14 UTC
Hi,

Would it be possible to also branch this package for EPEL-6?

tia,
jpo


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