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
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.
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?
(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
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.
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.
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.
ping, awaiting feedback from reviewer.
(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.
ok, I'll pick it up.
using SRPM link: https://dvratil.fedorapeople.org/plasma5/review/khelpcenter-5.1.2-2.fc21.src.rpm
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
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
Looks good, thanks, APPROVED.
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:
Git done (by process-git-requests).