Bug 711313 - (wicd-kde) Review Request: wicd-kde - a Wicd client built on the KDE Development Platform
Review Request: wicd-kde - a Wicd client built on the KDE Development Platform
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Kevin Kofler
Fedora Extras Quality Assurance
:
Depends On:
Blocks: kde-reviews
  Show dependency treegraph
 
Reported: 2011-06-07 03:10 EDT by Minh Ngo
Modified: 2012-09-01 07:25 EDT (History)
7 users (show)

See Also:
Fixed In Version: wicd-kde-0.3.0-4.fc17
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-11-22 19:58:19 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Minh Ngo 2011-06-07 03:10:12 EDT
Spec URL: http://fpaste.org/tnmQ/
SRPM URL: http://rghost.net/9723221
Description: A Wicd client built on the KDE Development Platform.
Comment 1 Minh Ngo 2011-06-07 04:37:38 EDT
Spec file checked http://fpaste.org/cbsN/
SRPM URL: http://rghost.net/9732771
Comment 2 Minh Ngo 2011-06-07 05:12:08 EDT
I'm sorry. This is the last second fix :)
Spec URL: http://fpaste.org/npzz/
SRPM URL: http://rghost.net/9737101

http://koji.fedoraproject.org/koji/taskinfo?taskID=3115413
Comment 3 Pavel Zhukov 2011-06-08 00:07:33 EDT
I'm using KDE and wicd. But I'm not sponsor. 

Few comments:
Use desktop-file-install for desktop files:
http://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files

Please insert license text to package.

you build package for KDE4 so you need read https://fedoraproject.org/wiki/SIGs/KDE/Packaging/Guidelines. There are some KDE-specific macros. Please use these instead of the standard macros. 

P.S. %{_prefix}/libexec == %{_libexecdir}
Comment 4 Minh Ngo 2011-06-08 02:11:12 EDT
>Use desktop-file-install for desktop files:
>http://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files

Fixed

>Please insert license text to package.
A license text is already in the file COPYING

>you build package for KDE4 so you need read
>https://fedoraproject.org/wiki/SIGs/KDE/Packaging/Guidelines. There are some
>KDE-specific macros. Please use these instead of the standard macros. 

>P.S. %{_prefix}/libexec == %{_libexecdir}

Fixed

Spec URL: http://fpaste.org/VHLr/
SRPM URL: http://koji.fedoraproject.org/koji/getfile?taskID=3118421&name=wicd-kde-0.2.2-1.fc15.src.rpm
Comment 5 Minh Ngo 2011-06-08 03:30:04 EDT
Also this is my first package, and I'm seeking a sponsor.
Comment 6 Minh Ngo 2011-06-08 03:36:01 EDT
Spec file and SRPM package are moved to the permanent repository https://github.com/Ignotus/wicd-kde-fedora
Comment 7 Minh Ngo 2011-06-08 03:39:37 EDT
Spec file and SRPM package are moved to the permanent repository https://github.com/Ignotus/wicd-kde-fedora
Comment 8 Dmitrij S. Kryzhevich 2011-06-08 05:07:16 EDT
Added kde-review flag.

For convenience plese provide direct links for spec and srpm they could be wget-ed.
Desktop-file-install validate .desktop too. There is no need to call desktop-file-validate after it.
Are you sure wicd-kde.desktop couild be used both as application .desktop and as service .desktop?

PS I'm not sponsor either.
Comment 9 Minh Ngo 2011-06-08 10:33:45 EDT
>Added kde-review flag.
thanks

>For convenience plese provide direct links for spec and srpm they could be
>wget-ed.
Spec URL: https://raw.github.com/Ignotus/wicd-kde-fedora/master/wicd-kde.spec
SRPM URL: https://github.com/Ignotus/wicd-kde-fedora/blob/master/wicd-kde-0.2.2-1.fc15.src.rpm?raw=true

>Desktop-file-install validate .desktop too. There is no need to call
>desktop-file-validate after it.

fixed

>Are you sure wicd-kde.desktop couild be used both as application .desktop and
>as service .desktop?
In CMakeList.txt:
install( PROGRAMS wicd-kde.desktop  DESTINATION ${XDG_APPS_INSTALL_DIR} )
install( FILES wicd-kde.desktop  DESTINATION ${AUTOSTART_INSTALL_DIR} )

What about kcm_wicd.desktop, it's my mistake. I have fixed it.
Comment 10 Dmitrij S. Kryzhevich 2011-06-09 00:50:12 EDT
%make_install defines DESTDIR var itself. Check it.

Speration two desktop-file-install with empty line will add legibility.

You had not mentioned any changes done with spec file in the %changelog section (with Release number update).

And, I think, that is all I can do for this package.
Comment 11 Minh Ngo 2011-06-09 07:14:31 EDT
>%make_install defines DESTDIR var itself. Check it.
>Speration two desktop-file-install with empty line will add legibility.

fixed

>You had not mentioned any changes done with spec file in the %changelog section
>(with Release number update).

this package's not yet in the fedora repository. So it's not necessary to mention any changes in the %changelog section.
Comment 12 Kevin Kofler 2011-06-09 07:26:45 EDT
> this package's not yet in the fedora repository. So it's not necessary to
> mention any changes in the %changelog section.

Actually, you should start maintaining the changelog as soon as you submit the first version for review. That allows reviewers to track what review comments you already addressed and which are still open.
Comment 13 Minh Ngo 2011-06-09 11:27:44 EDT
>Actually, you should start maintaining the changelog as soon as you submit the
>first version for review. That allows reviewers to track what review comments
>you already addressed and which are still open.

Spec URL: https://raw.github.com/Ignotus/wicd-kde-fedora/aa975b605f713d72d97d5b108f17989a044cf322/wicd-kde.spec
SRPM URL: https://github.com/Ignotus/wicd-kde-fedora/blob/aa975b605f713d72d97d5b108f17989a044cf322/wicd-kde-0.2.2-1.fc15.src.rpm?raw=true
Comment 14 Dmitrij S. Kryzhevich 2011-06-09 23:04:10 EDT
And again, Release number update: 0.2.2-1 -> 0.2.2-2.
Comment 16 Gregor Tätzner 2011-10-12 06:08:00 EDT
Could you provide direct download links, please?

-rpmlint wicd-kde-0.2.2-2.fc17.i686.rpm
================================================================================
        wicd-kde.i686: W: non-conffile-in-etc /etc/dbus-1/system.d/org.kde.wicdclient.scripts.conf
        wicd-kde.i686: W: no-manual-page-for-binary wicd-kde
        1 packages and 0 specfiles checked; 0 errors, 2 warnings.

-org.kde.wicdclient.scripts.conf must be marked as a config file.

-Why you don't use the kcm_wicd.desktop in the source file? By the way, there is a new wicd-kde release, consider upgrading your package

-the package contains a so file, you must call ldconfig
Comment 17 Minh Ngo 2011-10-12 10:40:42 EDT
-org.kde.wicdclient.scripts.conf must be marked as a config file.
-the package contains a so file, you must call ldconfig

fixed
SPEC: https://raw.github.com/Ignotus/wicd-kde-fedora/f37c38f037d9f1bcab805467d03dd754f7e52be1/wicd-kde.spec
SRPM: https://github.com/Ignotus/wicd-kde-fedora/blob/f37c38f037d9f1bcab805467d03dd754f7e52be1/wicd-kde-0.2.3-1.fc15.x86_64.rpm?raw=true

-Why you don't use the kcm_wicd.desktop in the source file? By the way, there
is a new wicd-kde release, consider upgrading your package

because:
>> error: key "Exec" is present in group "Desktop Entry", but the type is "Service" while this key is only valid for type "Application"
Comment 18 Kevin Kofler 2011-10-12 10:58:17 EDT
> -the package contains a so file, you must call ldconfig

Wrong. The package does not install any .so file to %{_libdir}, so ldconfig is NOT needed and should NOT be called. ldconfig does not operate on %{_kde4_libdir}/kde4 = %{_libdir}/kde4, which is only for dlopened plugins.

Please remove the ldconfig calls again.

> -Why you don't use the kcm_wicd.desktop in the source file? By the way, there
> is a new wicd-kde release, consider upgrading your package
>
> because:
> >> error: key "Exec" is present in group "Desktop Entry", but the type is
> >> "Service" while this key is only valid for type "Application"

There is no need to run desktop-file-validate on .desktop files in %{_kde4_datadir}/kde4/services. Those files will only ever be read by kdelibs, so if kdelibs understands them, they're fine.

AIUI, it's normal for KCMs to have an Exec= line which runs kcmshell4 on them, I don't think removing it is a good idea.
Comment 19 Gregor Tätzner 2011-10-28 09:31:42 EDT
Hey Minh Ngo, are you still around? Kevin found some issues.
Comment 20 Minh Ngo 2011-10-28 10:44:07 EDT
>Hey Minh Ngo, are you still around? Kevin found some issues.
Yes. I'm here :)

fixed
https://raw.github.com/Ignotus/wicd-kde-fedora/d95dc76dc9f3dd10e6761be9d2f5cf4c9ac1b875/wicd-kde.spec
Comment 21 Gregor Tätzner 2011-10-28 11:07:16 EDT
great, just a quick look:

1. follow the proper review request layout (like in comment 17, add SPEC: and SRPM:)
2. make a changelog entry in the spec every time you change something
Comment 23 Gregor Tätzner 2011-10-29 15:12:25 EDT
rpmlint wicd-kde-0.2.3-2.fc17.i686.rpm
        ================================================================================
        wicd-kde.i686: W: conffile-without-noreplace-flag /etc/dbus-1/system.d/org.kde.wicdclient.scripts.conf
        wicd-kde.i686: W: no-manual-page-for-binary wicd-kde
        1 packages and 0 specfiles checked; 0 errors, 2 warnings.

-use %config(noreplace) (see packaging guideline).But to be honest: I'm not exactly sure here, maybe the file should be replaced when a upgrade takes place.

wicd-kde.src:73: W: macro-in-%changelog %make_install

-better don't put a %make_install in the changelog

-also the Buildroot tag seems to be wrong -  if you really want to make it epel compatible:
http://fedoraproject.org/wiki/EPEL/GuidelinesAndPolicies#Distribution_specific_guidelines
Comment 24 Kevin Kofler 2011-10-29 15:54:38 EDT
If you want to refer to macros in the changelog, you have to double the % sign (which escapes the % sign and outputs one % sign to the user-visible changelog).
Comment 26 Gregor Tätzner 2011-11-07 11:35:23 EST
The build root tag is still wrong but for now you should try to find a sponsor. Take part in other reviews, demonstrate your knowledge, as you know already, probably.
Comment 27 Kevin Kofler 2011-11-07 11:59:39 EST
I can be the sponsor. Gregor, do you think the package is fine to be approved now (except for BuildRoot, which is ignored by current RPM anyway, so I don't think this should be a blocker)? If so, I'll have a quick look to make sure everything is right and then approve it.
Comment 28 Gregor Tätzner 2011-11-07 15:11:22 EST
@Kevin Yes, go ahead. Only one question: It appears that wicd doesn't conflict with networkmanager. Can we do something about it? As far as I know you can not use them both together.
Comment 29 Kevin Kofler 2011-11-07 15:18:22 EST
For that, we need to file a bug against the core packages (IMHO against wicd which is the non-default one), can you file one?
Comment 30 Gregor Tätzner 2011-11-07 16:14:27 EST
uh oh...this topic has already been discussed:
https://bugzilla.redhat.com/show_bug.cgi?id=713109

"I wanted to add a Conflict line, but the package reviewers disagreed. 
Technically, the packages can be installed at the same time, so there's not
really a conflict.  You can only run one or the other."

Should I fill out a report nevertheless?
Comment 31 Minh Ngo 2011-11-08 10:18:42 EST
@Gregor @Kevin

Thank you for helping
Comment 32 Kevin Kofler 2011-11-08 11:38:50 EST
Re the Conflicts, I don't care either way. The files don't conflict but the functionality does. But in any case, that is an issue with the wicd core and not related to this review.


Unfortunately, I see some issues remaining with the specfile:

> BuildRoot:      %{_tmppath}/%{name}-%{version}

As Gregor already pointed out, please either remove this line or use one of the lines from:
https://fedoraproject.org/wiki/EPEL/GuidelinesAndPolicies#BuildRoot_tag
(The RPM in current versions of Fedora ignores BuildRoot entirely.)

> BuildRequires:  qt-devel

Better use qt4-devel, which:
* also works on old versions where Qt 3 was the default,
* will also work on future versions where Qt 5 will be the default and
* allows specifying a version without an Epoch, i.e. these will work if some future version of wicd-kde starts requiring Qt 4.8 for whatever reason:
BuildRequires: qt4-devel >= 4.8.0
or:
BuildRequires: qt-devel >= 1:4.8.0
but this will not:
BuildRequires: qt-devel >= 4.8.0
and the Epoch tends to get forgotten in such cases.

> BuildRequires:  kdebase-devel

Are you sure you want kdebase-devel? I think you want kdelibs-devel, or better kdelibs4-devel (same as for Qt above).

> %make_install -C %{_target_platform}

The %make_install macro should not be used in Fedora (except for broken legacy tarballs which do not support DESTDIR nor INSTALL_ROOT, but all CMake-based projects support DESTDIR), please use:
make install/fast DESTDIR="%{buildroot}" -C %{_target_platform}
instead. (For non-CMake projects, use just "install" instead of "install/fast". "install/fast" is a CMake target which skips the checks for whether everything to be installed is already built. We already run "make" in %build, so we can rely on everything already being built by the time we get to %install.)

And you should use either %{buildroot} or ${RPM_BUILD_ROOT} consistently rather than mixing the two.
Comment 33 Kevin Kofler 2011-11-08 11:41:50 EST
Or I guess:
make install/fast DESTDIR=%{buildroot} -C %{_target_platform}
without the quotes is good enough (and consistent with the other uses of %{buildroot}).
Comment 35 Kevin Kofler 2011-11-10 11:35:26 EST
Pedantic comment of the day:

> cp ${RPM_BUILD_ROOT}%{_kde4_datadir}/applications/kde4/%{name}.desktop \
>    ${RPM_BUILD_ROOT}%{_kde4_datadir}/autostart/%{name}.desktop

We prefer cp -p (no clobbering of the timestamp) to plain cp. Though in this case it doesn't matter because the file has already been edited by the desktop-file-install --add-category command before. So this is only a SHOULD.


So it looks like all the issues found have been addressed. Thanks to everyone who did preliminary reviews.

The package is APPROVED.

We can now proceed with the sponsoring process. Have you already signed up for a FAS (Fedora Account System) account? What's your FAS account name?
Comment 36 Minh Ngo 2011-11-10 11:49:01 EST
>What's your FAS account name?

ignotusp
Comment 37 Kevin Kofler 2011-11-10 12:00:25 EST
Congratulations, you are now sponsored!

Please proceed with the remainder of the:
https://fedoraproject.org/wiki/Join_the_package_collection_maintainers
process.

For any future package submissions, follow the:
https://fedoraproject.org/wiki/New_package_process_for_existing_contributors
process instead, as you are now an existing package maintainer.
Comment 38 Minh Ngo 2011-11-10 12:39:13 EST
How to create the git repository for wicd-kde?
Comment 39 Kevin Kofler 2011-11-10 12:50:11 EST
By following yet another process…
https://fedoraproject.org/wiki/Package_SCM_admin_requests
but don't worry, you only have to post a message to this bug complying to the format (careful there, those requests are processed by scripts, so there's little to no tolerance for incorrectly-formatted requests) and set the fedora-cvs flag.
Comment 40 Minh Ngo 2011-11-10 13:01:19 EST
New Package SCM Request
=======================
Package Name: wicd-kde
Short Description: A Wicd client built on the KDE Development Platform
Owners: ignotusp
Branches: f14 f15 f16 el6
InitialCC:
Comment 41 Gwyn Ciesla 2011-11-10 13:41:05 EST
Git done (by process-git-requests).

Not currently accepting new packages for F-14.

Kevin, please take ownership of review BZs.  Thanks!
Comment 42 Kevin Kofler 2011-11-10 13:54:23 EST
> Not currently accepting new packages for F-14.

FYI, Fedora 14 will reach its end of life in less than a month, and no new packages are accepted for Fedora 14. (Updates to existing packages are possible until the end of life. At that point, Fedora 14 will be completely dead as far as the Fedora Project is concerned.)

> Kevin, please take ownership of review BZs.  Thanks!

Whoops, fixed.
Comment 43 Kevin Kofler 2011-11-10 13:56:44 EST
(PS: Minh, next time, when you file a "New Package SCM Request", please set the fedora-cvs flag to '?', not '+'. It gets set to '+' when the request is handled, as was done now with comment #41. I fixed that for you.)
Comment 44 Minh Ngo 2011-11-10 15:20:02 EST
>  3505780 tagBuild (noarch): free
>  3505780 tagBuild (noarch): free -> open (x86-06.phx2.fedoraproject.org)
>  3505780 tagBuild (noarch): open (x86-06.phx2.fedoraproject.org) -> FAILED: >ActionNotAllowed: tag requires admin permission
>  0 free  1 open  3 done  1 failed
>3505695 build (f16, /wicd-kde:8f92e20460e571c7b5a3b177dd7e695d8b7065a0): open >(x86-13.phx2.fedoraproject.org) -> FAILED: ActionNotAllowed: tag requires admin >permission
>  0 free  0 open  3 done  2 failed

>3505695 build (f16, /wicd-kde:8f92e20460e571c7b5a3b177dd7e695d8b7065a0) failed

http://koji.fedoraproject.org/koji/taskinfo?taskID=3505695
Comment 45 Kevin Kofler 2011-11-10 17:37:28 EST
Your fedpkg must be outdated. The build target "f16" hasn't been used since Fedora 16 got branched, and it's even released now. Fedora 16 builds should be built in "f16-candidate", Rawhide builds in "dist-rawhide". Normally, fedpkg gets that right for you, so make sure your fedpkg package is up to date.

For this build, Rex Dieter fixed the tagging mess for you, but please make sure to use an up-to-date fedpkg when submitting more builds.
Comment 46 Fedora Update System 2011-11-11 08:22:02 EST
wicd-kde-0.2.3-3.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/wicd-kde-0.2.3-3.fc16
Comment 47 Fedora Update System 2011-11-11 08:25:26 EST
wicd-kde-0.2.3-3.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/wicd-kde-0.2.3-3.fc15
Comment 48 Fedora Update System 2011-11-11 22:27:57 EST
wicd-kde-0.2.3-3.fc16 has been pushed to the Fedora 16 testing repository.
Comment 49 Fedora Update System 2011-11-22 19:58:19 EST
wicd-kde-0.2.3-3.fc16 has been pushed to the Fedora 16 stable repository.
Comment 50 Fedora Update System 2011-11-22 20:01:25 EST
wicd-kde-0.2.3-3.fc15 has been pushed to the Fedora 15 stable repository.
Comment 51 Fedora Update System 2012-01-10 02:26:20 EST
wicd-kde-0.3.0-1.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/wicd-kde-0.3.0-1.fc16
Comment 52 Fedora Update System 2012-01-10 02:27:34 EST
wicd-kde-0.3.0-1.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/wicd-kde-0.3.0-1.fc15
Comment 53 Fedora Update System 2012-01-18 20:29:21 EST
wicd-kde-0.3.0-1.fc16 has been pushed to the Fedora 16 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 54 Fedora Update System 2012-01-18 20:31:27 EST
wicd-kde-0.3.0-1.fc15 has been pushed to the Fedora 15 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 55 Fedora Update System 2012-03-14 15:12:01 EDT
wicd-kde-0.3.0-3.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/wicd-kde-0.3.0-3.fc17
Comment 56 Fedora Update System 2012-04-04 14:50:22 EDT
wicd-kde-0.3.0-4.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/wicd-kde-0.3.0-4.fc17
Comment 57 Kevin Kofler 2012-04-04 15:32:04 EDT
Please do not reference the review request in all your updates!

The review request is already resolved when the INITIAL "newpackage" update goes through. Further updates should NOT reference it.
Comment 58 Fedora Update System 2012-04-11 23:45:53 EDT
wicd-kde-0.3.0-4.fc17 has been pushed to the Fedora 17 stable repository.  If problems still persist, please make note of it in this bug report.

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