Bug 1135503 (khelpcenter) - Review Request: khelpcenter - Application to show KDE Application's documentation
Summary: Review Request: khelpcenter - Application to show KDE Application's documenta...
Keywords:
Status: CLOSED RAWHIDE
Alias: khelpcenter
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Rex Dieter
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: plasma5
TreeView+ depends on / blocked
 
Reported: 2014-08-29 13:22 UTC by Daniel Vrátil
Modified: 2015-11-02 01:38 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-01-20 13:34:27 UTC
Type: Bug
Embargoed:
rdieter: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Daniel Vrátil 2014-08-29 13:22:02 UTC
Spec URL: https://dvratil.fedorapeople.org/plasma5/review/khelpcenter.spec
SRPM URL: https://dvratil.fedorapeople.org/plasma5/review/khelpcenter-5.0.1-1.fc20.src.rpm
Description: Application to show KDE Application's documentation.
Fedora Account System Username: dvratil

Comment 1 Christopher Meng 2014-08-30 07:39:32 UTC
1. 

Checking: khelpcenter-5.0.1-1.fc22.i686.rpm
          khelpcenter-5.0.1-1.fc22.src.rpm
khelpcenter.i686: E: invalid-soname /usr/lib/libkdeinit5_khelpcenter.so libkdeinit5_khelpcenter.so
khelpcenter.i686: W: hidden-file-or-dir /usr/share/khelpcenter/plugins/Scrollkeeper/.directory
khelpcenter.i686: W: hidden-file-or-dir /usr/share/khelpcenter/plugins/Manpages/.directory
khelpcenter.i686: W: hidden-file-or-dir /usr/share/khelpcenter/plugins/Applications/.directory
khelpcenter.i686: W: no-manual-page-for-binary khelpcenter
khelpcenter.src: W: strange-permission khelpcenter-5.0.1.tar.xz 0640L
2 packages and 0 specfiles checked; 1 errors, 5 warnings.

i. Are these . files useful?

ii. What about the soname? Is it expected?

2. Just a hint:

%{_datadir}/doc -> %{_docdir}

3. Pick one line:

[ 97%] Building CXX object CMakeFiles/kdeinit_khelpcenter.dir/searchhandler.cpp.o
/usr/bin/c++   -DKCOREADDONS_LIB -DKGUIADDONS_LIB -DQT_CORE_LIB -DQT_DBUS_LIB -DQT_DISABLE_DEPRECATED_BEFORE=0 -DQT_GUI_LIB -DQT_NETWORK_LIB -DQT_NO_DEBUG -DQT_PRINTSUPPORT_LIB -DQT_WIDGETS_LIB -DQT_XML_LIB -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE -D_LARGEFILE64_SOURCE -D_XOPEN_SOURCE=500 -Dkdeinit_khelpcenter_EXPORTS -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches  -m32 -march=i686 -mtune=atom -fasynchronous-unwind-tables  -std=c++0x -fno-exceptions -Wall -Wextra -Wcast-align -Wchar-subscripts -Wformat-security -Wno-long-long -Wpointer-arith -Wundef -Wnon-virtual-dtor -Woverloaded-virtual -Werror=return-type -O3 -DNDEBUG -fPIC -fvisibility=hidden -fvisibility-inlines-hidden -I/builddir/build/BUILD/khelpcenter-5.0.1/i686-redhat-linux-gnu -I/builddir/build/BUILD/khelpcenter-5.0.1 -isystem /usr/include/KF5/KHtml -isystem /usr/include/KF5 -isystem /usr/include/qt5 -isystem /usr/include/qt5/QtGui -isystem /usr/include/qt5/QtCore -isystem /usr/lib/qt5/mkspecs/linux-g++ -isystem /usr/include/KF5/KIOCore -isystem /usr/include/KF5/KCoreAddons -isystem /usr/include/KF5/KService -isystem /usr/include/KF5/KConfigCore -isystem /usr/include/KF5/KI18n -isystem /usr/include/KF5/KParts -I/usr/include/KF5/KIOWidgets -I/usr/include/KF5/KJobWidgets -isystem /usr/include/qt5/QtWidgets -I/usr/include/qt5/QtNetwork -isystem /usr/include/KF5/KCompletion -isystem /usr/include/KF5/KWidgetsAddons -I/usr/include/KF5/KXmlGui -isystem /usr/include/qt5/QtDBus -isystem /usr/include/qt5/QtXml -isystem /usr/include/KF5/KConfigWidgets -I/usr/include/KF5/KCodecs -I/usr/include/KF5/KConfigGui -I/usr/include/KF5/KAuth -isystem /usr/include/KF5/KTextWidgets -I/usr/include/KF5/SonnetUi -isystem /usr/include/KF5/KCMUtils -isystem /usr/include/KF5/KDBusAddons -isystem /usr/include/KF5/KDELibs4Support -isystem /usr/include/KF5/KDELibs4Support/KDE -isystem /usr/include/qt5/QtPrintSupport -isystem /usr/include/KF5/KCrash -isystem /usr/include/KF5/KIOFileWidgets -I/usr/include/KF5/KBookmarks -I/usr/include/KF5/KItemViews -I/usr/include/KF5/Solid -isystem /usr/include/KF5/KNotifications -isystem /usr/include/KF5/KIconThemes -isystem /usr/include/KF5/KWindowSystem -isystem /usr/include/KF5/KGuiAddons -isystem /usr/include/KF5/KUnitConversion    -o CMakeFiles/kdeinit_khelpcenter.dir/searchhandler.cpp.o -c /builddir/build/BUILD/khelpcenter-5.0.1/searchhandler.cpp

Looks like O3 is being used and overriding optflags, does Fedora allow O3 to be used in KDE?

Others are fine.

Comment 2 Daniel Vrátil 2014-09-01 15:35:03 UTC
The .directory files are being installed explictly in CMakeLists.txt, so they are intentional.

libkdeinit_* are KDE init plugins, they don't have soname.

Can't find anything in khelpcenter or it's deps that would be changing CXX_FLAGS. Could it somehow got in from your environment?

Comment 3 Christopher Meng 2014-09-01 23:01:27 UTC
(In reply to Dan Vrátil from comment #2)
> Can't find anything in khelpcenter or it's deps that would be changing
> CXX_FLAGS. Could it somehow got in from your environment?

No...I think ;)

I will post a unprejudiced koji link:

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

Comment 4 Rex Dieter 2014-12-03 19:42:07 UTC
I'm pretty sure the O3 thing is an issue elsewhere, either in cmake or kf5 or ???, so I wouldn't BLOCK this review on it.  I'll try to dig deeper to find the culprit.

Comment 5 Kevin Kofler 2014-12-04 18:30:39 UTC
The -O3 comes from CMake's default flags for the Release build type. I think KDE 4 overrode those in FindKDE4Internal.cmake, but KF5 keeps CMake's defaults.

IMHO, we should change that default in the cmake package. And yes, ANY package that uses -O3 without a good justification (and "CMake has broken defaults" is not) should not pass review.

I think using RelWithDebInfo instead will make it use -O2, it may or may not have other side effects. (IIRC, we opted against defaulting to RelWithDebInfo because KDE 4 stuff only had -DNDEBUG in Release, or something like that. Not sure how KF5 behaves there.) But I insist, the default for Release should be fixed in CMake packaging.

Comment 6 Daniel Vrátil 2014-12-19 16:28:32 UTC
Spec URL: https://dvratil.fedorapeople.org/plasma5/review/khelpcenter.spec
SRPM URL: https://dvratil.fedorapeople.org/plasma5/review/khelpcenter-5.1.2-2.fc20.src.rpm

Replaced %{_datadir}/doc with %{_docdir}, the remaining issues are explained/dismissed in comments above.

Comment 7 Rex Dieter 2014-12-31 22:01:48 UTC
ping, awaiting feedback from reviewer.

Comment 8 Christopher Meng 2015-01-01 01:28:18 UTC
(In reply to Rex Dieter from comment #7)
> ping, awaiting feedback from reviewer.

Sorry Rex, I'm busy now, I will reassign and let others review this.

Comment 9 Rex Dieter 2015-01-04 16:13:24 UTC
ok, I'll pick it up.

Comment 11 Rex Dieter 2015-01-04 16:29:01 UTC
naming: ok

1. though SHOULD use better project url:
URL: https://projects.kde.org/projects/kde/workspace/khelpcenter

2. SHOULD consider dropping needless scriptlets:
%post -p /sbin/ldconfig
%postun -p /sbin/ldconfig

though I'm not sure if 
%{_kf5_libdir}/libkdeinit5_khelpcenter.so
counts as a shared library in this circumstance or not.

3. SHOULD use
%find_lang ... --with-kde ...
then it should automatically pick up the HTML handbook stuff too

4. SHOULD prefer/use
make install/fast DESTDIR=%{buildroot}
over
%make_install

5. MUST validate application .desktop file
%{_datadir}/applications/Help.desktop
suggest adding 
BuildRequires: desktop-file-utils
and
%check
desktop-file-validate %{buildroot}%{_datadir}/applications/Help.desktop

6. SHOULD consider shipping
%{_kde4_datadir}/kde4/services/khelpcenter.desktop
%{_kde4_datadir}/services/khelpcenter.desktop
to provide services for kde4/kde3 runtimes too (like kde-runtime-4's khelpcenter pkg does)

7. license: NOT ok
there's a mix of GPLv2+ and GPLv2 or GPLv3 so MUST fix license tag, I'd suggest:
License: GPLv2 or GPLv3

Comment 12 Daniel Vrátil 2015-01-06 13:34:44 UTC
Spec URL: https://dvratil.fedorapeople.org/plasma5/review/khelpcenter.spec
SRPM URL: https://dvratil.fedorapeople.org/plasma5/review/khelpcenter-5.1.2-3.fc21.src.rpm

- better URL
- remove unnecessary scriptlets
- validate desktop files
- ship service files for KDE 3 and KDE 4
- fix license

Comment 13 Rex Dieter 2015-01-06 13:46:27 UTC
Looks good, thanks, APPROVED.

Comment 14 Daniel Vrátil 2015-01-06 13:58:37 UTC
New Package SCM Request
=======================
Package Name: khelpcenter
Short Description: Application to show KDE Application's documentation
Upstream URL: https://projects.kde.org/projects/kde/workspace/khelpcenter
Owners: group::kde-sig
Branches:
InitialCC:

Comment 15 Gwyn Ciesla 2015-01-06 14:11:39 UTC
Git done (by process-git-requests).


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