Bug 471829 - Review Request: log4cxx - Log4cxx - a port to C++ of the Log4j project
Review Request: log4cxx - Log4cxx - a port to C++ of the Log4j project
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-11-16 21:11 EST by Hayden James
Modified: 2011-10-24 06:25 EDT (History)
5 users (show)

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


Attachments (Terms of Use)

  None (edit)
Description Hayden James 2008-11-16 21:11:24 EST
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 06:14:06 EST
Hello, anyone there? (*crickets chirping*)
Comment 2 Jason Tibbitts 2008-11-26 11:50:09 EST
Dude, there are seven hundred pending package review tickets.  Please be patient.
Comment 3 Mamoru TASAKA 2008-11-27 02:47:44 EST
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 09:50:04 EST
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 12:06:30 EST
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 14:28:24 EST
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 12:08:08 EST
(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 11:25:57 EST
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 13:30:44 EST
--------------------------------------------------------
   This package (log4cxx) is APPROVED by mtasaka
--------------------------------------------------------
Comment 12 Hayden James 2008-12-01 13:35:01 EST
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 15:45:06 EST
cvs done.
Comment 14 Fedora Update System 2008-12-01 18:32:31 EST
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 18:33:19 EST
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 08:42:26 EST
Okay, thanks.
Comment 17 Fedora Update System 2008-12-02 20:18:26 EST
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-02 20:23:23 EST
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 19:23:14 EDT
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.