Bug 1385856

Summary: Review Request: log4shib - C++ logging library for Shibboleth (OpenSAML)
Product: [Fedora] Fedora Reporter: Raphael Groner <projects.rg>
Component: Package ReviewAssignee: Zbigniew Jędrzejewski-Szmek <zbyszek>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: extras-qa, guido.grazioli, nobody, package-review, projects.rg, raphael.groner, vascom2, walter.pete, zbyszek
Target Milestone: ---Flags: zbyszek: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: 1196920 Environment:
Last Closed: 2017-03-05 18:55:50 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 1196917, 1196918    

Description Raphael Groner 2016-10-17 19:47:59 UTC
+++ This bug was initially created as a clone of Bug #1196920 +++

Spec URL: 
https://guidograzioli.fedorapeople.org/packages/log4shib/log4shib.spec

SRPM URL: 
https://guidograzioli.fedorapeople.org/packages/log4shib/log4shib-1.0.9-1.fc23.src.rpm

Description: 
log4shib is a forked version of log4cpp that has been created for the
Shibboleth project to ensure a consistent, working snapshot that builds
reliably on the necessary platforms.

Fedora Account System Username:guidograzioli

Additional info:
opensaml build is currently (2.4.3) falling back to using log4cpp; for updating to opensaml 2.5.3 (xmltooling 1.5.3) this package becomes necessary.

--- Additional comment from Michael Schwendt on 2015-03-11 12:55:50 CET ---

> Name:           log4shib
> Group:          Development/Libraries

Runtime library base packages have been in group "System Environment/Libraries" for many years. The group "Development/Libraries" is for the separate build-time -devel packages.

Nowadays, the Group tag is optional, however:
https://fedoraproject.org/wiki/Packaging:Guidelines#Group_tag

Reusing a Fedora spec file for EL is not always a good idea, since differences in updates and rebuilds introduce a lot of irrelevant crap in the %changelog.


> BuildRequires:  gcc-c++

JFYI, it is not necessary yet to add this.
https://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2


> %package doc
> Summary:        Development documentation for %{name}
> Group:          Development/Libraries

If at all, it would be group "Documentation".


> Requires:       %{name}%{?_isa} = %{version}-%{release}

Although the guidelines don't cover it [yet], documentation packages ought to stay free of such unnecessary dependencies. It should be possible to install documentation packages without pulling in unneeded runtime libs and other dependencies. That is especially true, if the documentation does not need the base package to be displayed/viewed by the user.


> # Packages are not supposed to add %optflags or %__global_ldflags to *.pcs
> sed -i -e "s|%{optflags}||;s|%{__global_ldflags}||" %{name}.pc

This will require a second look as I've only skimmed over the spec file. It would be good if the .pc did not add any other unnecessary stuff to CFLAGS/LDFLAGS.


> %files
> %defattr(-, root, root, 0755)

Superfluous. %defattr is not necessary anymore for a long time, and a change from the default (-,root,root,-) would need to be explained:
https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions


> %doc ChangeLog 

There are a couple of other plain text doc files. Why not include them?


> %{_bindir}/log4shib-config

This will need to be checked for multilib conflicts as well as for inserted CFLAGS/LDFLAGS just like the .pc file.  In case there is a conflict, a common work-around is to have the script query pkg-config instead of redefining paths/values itself.

--- Additional comment from Guido Grazioli on 2015-03-11 14:12:00 CET ---

(In reply to Michael Schwendt (Fedora Packager Sponsors Group) from comment #1)

aaargh, apologies for having been lazy; I didn't handle a new package for quite some time (ie years) so I'd thought that stealing from the log4cpp spec would have been a nice way to avoid to go thru the guidelines again. I'll address the issues shortly, thanks for your time.

--- Additional comment from Raphael Groner (DAASI International) on 2016-08-08 09:51:07 CEST ---

Any news here?

--- Additional comment from Raphael Groner on 2016-10-03 13:44:46 CEST ---

Friendly reminder. Are you still interested in this package?

--- Additional comment from Guido Grazioli on 2016-10-17 14:44:42 CEST ---

No.

--- Additional comment from Raphael Groner on 2016-10-17 20:45:40 CEST ---

This library is needed to update shibboleth stack, I'll take over here.

Comment 2 Raphael Groner 2016-10-17 20:03:26 UTC
*** Bug 1196920 has been marked as a duplicate of this bug. ***

Comment 3 Raphael Groner 2016-11-13 15:00:01 UTC
Pete, did you have a chance to run fedora-review?

It would be great if we can achieve some progress with the Shibboleth stack in Fedora.

Comment 4 Raphael Groner 2017-01-31 23:47:33 UTC
No response so far. :(

Comment 5 Vasiliy Glazov 2017-02-07 11:51:03 UTC
Please correct rpmlint error and warning:
log4shib.x86_64: E: incorrect-fsf-address /usr/share/licenses/log4shib/COPYING
log4shib.x86_64: W: file-not-utf8 /usr/share/doc/log4shib/THANKS

Comment 6 Zbigniew Jędrzejewski-Szmek 2017-02-07 16:28:42 UTC
I don't get this insistence on substituting all occurrences of the name with %name. For example, if I want to open the URL field in browser, c&p is not enough, I need to manually replace %name. IMHO, such changes are a cargo-cult. Just saying.

Suggestions:
make %{?_smp_mflags} → %make_build
make install DESTDIR=%{buildroot} → %make_install

+ package name is OK
+ license is acceptable (LGPv2+)
+ builds and installs OK
+ provides/requires/buildrequires look correct
+ scriptlets are sane
+ fedora-review doesn't point out any issues

Package is APPROVED.

rpmlint:
log4shib.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblog4shib.so.1.0.9 pthread_key_create
log4shib.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblog4shib.so.1.0.9 pthread_getspecific
log4shib.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblog4shib.so.1.0.9 pthread_key_delete
log4shib.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblog4shib.so.1.0.9 pthread_setspecific
I think that's related to the recent glibc changes, not a problem with this package.

log4shib.x86_64: W: unused-direct-shlib-dependency /usr/lib64/liblog4shib.so.1.0.9 /lib64/libnsl.so.1
log4shib.x86_64: W: unused-direct-shlib-dependency /usr/lib64/liblog4shib.so.1.0.9 /lib64/libm.so.6
Both of those libraries are provided by glibc, so this extra dep is harmless.

log4shib.x86_64: W: file-not-utf8 /usr/share/doc/log4shib/THANKS
log4shib.x86_64: E: incorrect-fsf-address /usr/share/licenses/log4shib/COPYING
Like Vasiliy said, you might want to fix those, or maybe notify upstream.

Comment 7 Zbigniew Jędrzejewski-Szmek 2017-02-07 16:31:25 UTC
NEWS is useless, can be dropped from %doc.

There are some tests in tests/. Would be nice to add a %check section.

Comment 8 Zbigniew Jędrzejewski-Szmek 2017-02-07 16:34:39 UTC
If you still want to do the swap review, #1415331 is nice an simple.

Comment 9 Gwyn Ciesla 2017-02-07 19:44:41 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/log4shib

Comment 10 Raphael Groner 2017-03-05 18:55:50 UTC
Imported into master/rawhide. Please let me know when you need packages for the other branches.

Comment 11 Raphael Groner 2017-03-26 10:01:38 UTC
(In reply to Vasiliy Glazov from comment #5)
> Please correct rpmlint error and warning:
> log4shib.x86_64: E: incorrect-fsf-address
> /usr/share/licenses/log4shib/COPYING
> log4shib.x86_64: W: file-not-utf8 /usr/share/doc/log4shib/THANKS

It's not clear where to report that to. Is this also a bug of log4cpp where log4ship is forked from?