Bug 770174 - Review Request: kde-partitionmanager - GUI for managing disk partitions
Review Request: kde-partitionmanager - GUI for managing disk partitions
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Hans de Goede
Fedora Extras Quality Assurance
:
Depends On:
Blocks: kde-reviews
  Show dependency treegraph
 
Reported: 2011-12-23 17:06 EST by Mattia Verga
Modified: 2012-01-06 15:15 EST (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-01-06 15:15:37 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
hdegoede: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Mattia Verga 2011-12-23 17:06:48 EST
Spec URL: http://www.messafuoco.com/fedora/kde-partitionmanager.spec
SRPM URL: http://www.messafuoco.com/fedora/kde-partitionmanager-1.0.3-4.20111223svn.fc16.src.rpm
Description: (Orphaned package)KDE Partition Manager is a utility program to help you manage the disk devices, partitions and file systems on your computer. It allows you to easily create, copy, move, delete, resize without losing data, backup and restore partitions.
Comment 1 Mattia Verga 2011-12-23 17:08:18 EST
This is my first package and I need a sponsor also.
Comment 2 Hans de Goede 2011-12-27 09:00:12 EST
Hi,

I'll review this and when the review is done I'll sponsor you, removing needsponsor blocker.

Good:
- rpmlint checks return:
kde-partitionmanager.src: W: spelling-error %description -l en_US resize -> reside, re size, re-size
kde-partitionmanager.src: W: spelling-error %description -l en_US reiserfs -> Dreiser, serfs
kde-partitionmanager.src: W: spelling-error %description -l en_US jfs -> ifs, jiffs
kde-partitionmanager.src: W: spelling-error %description -l en_US xfs -> xis, ifs, xrefs
kde-partitionmanager.src: W: invalid-url Source0: kde-partitionmanager-1.0.3-20111223svn.tar.gz
kde-partitionmanager.x86_64: W: spelling-error %description -l en_US resize -> reside, re size, re-size
kde-partitionmanager.x86_64: W: spelling-error %description -l en_US reiserfs -> Dreiser, serfs
kde-partitionmanager.x86_64: W: spelling-error %description -l en_US jfs -> ifs, jiffs
kde-partitionmanager.x86_64: W: spelling-error %description -l en_US xfs -> xis, ifs, xrefs
kde-partitionmanager.x86_64: E: invalid-soname /usr/lib64/libpartitionmanagerprivate.so libpartitionmanagerprivate.so
kde-partitionmanager.x86_64: E: invalid-soname /usr/lib64/libpartitionmanagerfatlabel.so libpartitionmanagerfatlabel.so
kde-partitionmanager.x86_64: E: script-without-shebang /usr/share/applications/kde4/kde-partitionmanager.desktop
kde-partitionmanager.x86_64: W: no-manual-page-for-binary partitionmanager
kde-partitionmanager.x86_64: W: no-manual-page-for-binary partitionmanager-bin
3 packages and 0 specfiles checked; 3 errors, 11 warnings.

These can all be ignored except for the script-without-shebang warning, see MUST FIX section
- package meets naming guidelines
- package meets packaging guidelines
- license (GPLv2+) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- no duplicate files
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- .desktop file properly validated.

MUST FIX:
--------------
- Icons are installed under the freedesktop icon standard icon dir, so you need to add icon cache update
scriplets, see:
http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache
- The .desktop file: /usr/share/applications/kde4/kde-partitionmanager.desktop is installed as being
 an executable file (chmod 755), it should be 644 instead.
- You must add a "Requires: kde-filesystem-4"  for ownership of dirs like /usr/share/kde4/apps ans
 /usr/share/applications/kde4
Comment 3 Mattia Verga 2011-12-27 12:58:54 EST
Thank you Hans, I think I've corrected the issues you pointed out.
I saw that in /usr/share/applications/kde4 there are many .desktop files with permission set to 755, so I thought it was a false positive from rpmlint... at least for KDE apps. Anyway I corrected it to be 644.
You can find the new .src.rpm at
http://www.messafuoco.com/fedora/kde-partitionmanager-1.0.3-5.20111223svn.fc16.src.rpm

(the link to the .spec file it's the same)
Comment 4 Hans de Goede 2011-12-29 10:27:03 EST
(In reply to comment #3)
> Thank you Hans, I think I've corrected the issues you pointed out.

Thanks, I see you addressed all the mentioned issues, still I would like to see one more revision, to address 2 small things with how you fixed things:

1) It is usual for %post[un|trans] scripts to be added to the spec file before %files, so please move all three to above %files

2) The way you fixed the executable .desktop file is a bit non standard, it works, but the usual way to fix things like this would be to add:
chmod -x %{buildroot}%{_kde4_datadir}/applications/kde4/%{name}.desktop
To %install (after the make install)

> I saw that in /usr/share/applications/kde4 there are many .desktop files with
> permission set to 755, so I thought it was a false positive from rpmlint... at
> least for KDE apps

No rpmlint is right to warn, it is not like you can do:
/usr/share/applications/kde4/kde-partitionmanager.desktop
And have it do something, like you can do ie:
/bin/ls
And have it do something, so the file should not be marked executable. If more kde4 packages are doing this then I say BAD kde4 !  :)
Comment 5 Robin Lee 2011-12-30 00:48:07 EST
* For proper Requires:, consider using before the %description section this code instead of Requires kde-filesystem[1]: 

%{?_kde4_macros_api:Requires: kde4-macros(api) = %{_kde4_macros_api} } 


[1] http://fedoraproject.org/wiki/SIGs/KDE#Best_Practices
Comment 6 Mattia Verga 2011-12-31 04:18:56 EST
Thank you Hans & Robin.

I've uploaded the new .spec and .src
http://www.messafuoco.com/fedora/kde-partitionmanager.spec

http://www.messafuoco.com/fedora/kde-partitionmanager-1.0.3-5.20111223svn.fc16.src.rpm

(same version as previous)
Comment 7 Kevin Kofler 2011-12-31 11:44:15 EST
So does this support parted 3 now?
Comment 8 Mattia Verga 2012-01-01 04:40:33 EST
(In reply to comment #7)
> So does this support parted 3 now?

Yes, it successfully compile in F16 now (tried with a scratch build in koji with 'f16-candidate' as target).
It should fix #757661. I applied a new review request because it's an orphaned package in database.
Comment 9 Kevin Kofler 2012-01-01 12:17:06 EST
> No rpmlint is right to warn, it is not like you can do:
> /usr/share/applications/kde4/kde-partitionmanager.desktop
> And have it do something, like you can do ie:
> /bin/ls
> And have it do something, so the file should not be marked executable. If more
> kde4 packages are doing this then I say BAD kde4 !  :)

No, rpmlint is wrong to warn.

For security reasons, KDE requires .desktop files to have the executable bit set in most cases. This prevents e-mails from shipping a .desktop file as an attachment which runs some nasty command, possibly even a self-replicating worm.

Now, there's an exception for files in /usr and/or owned by root, so for RPMs, it doesn't actually matter whether the +x bit is set or not, but KDE upstream considers that a backwards compatibility hack, and upstream always installs all .desktop files as executable. (As I understand it, the idea is that they should all be +x, we're just not there yet.)

See:
* http://mail.gnome.org/archives/desktop-devel-list/2009-February/msg00132.html (which I think didn't end up getting applied though)
* http://lists.kde.org/?l=kde-core-devel&m=123532436728689&w=4
* http://lists.kde.org/?l=kde-core-devel&m=128595109525156&w=4

We need to get this rpmlint warning dropped, and IMHO we should also make it a SHOULD or even a MUST in our packaging guidelines to have that +x bit set, and eventually start making desktops drop those compatibility hacks and just require +x on all .desktop files.
Comment 10 Kevin Kofler 2012-01-01 12:23:04 EST
Oh, by the way, you can't run a .so either and yet it MUST have the +x bit set or debuginfo extraction doesn't work. So there's a precedent already.
Comment 11 Rex Dieter 2012-01-01 12:45:58 EST
fyi, the rpmlint warning has been dropped in recent builds (rawhide at least, not sure if it's trickled down anywhere else yet), bug #767978
Comment 12 Hans de Goede 2012-01-01 15:13:53 EST
(In reply to comment #9)
> > No rpmlint is right to warn, it is not like you can do:
> > /usr/share/applications/kde4/kde-partitionmanager.desktop
> > And have it do something, like you can do ie:
> > /bin/ls
> > And have it do something, so the file should not be marked executable. If more
> > kde4 packages are doing this then I say BAD kde4 !  :)
> 
> No, rpmlint is wrong to warn.
> 
> For security reasons, KDE requires .desktop files to have the executable bit
> set in most cases. This prevents e-mails from shipping a .desktop file as an
> attachment which runs some nasty command, possibly even a self-replicating
> worm.

Ah, interesting I did not know that live and learn. I stand corrected then :)
Comment 13 Hans de Goede 2012-01-01 15:17:23 EST
Hi,

(In reply to comment #6)
> Thank you Hans & Robin.
> 
> I've uploaded the new .spec and .src
> http://www.messafuoco.com/fedora/kde-partitionmanager.spec

This looks good now -> approved!

If you create a FAS account (if you've not done so already) and let me know your FAS login then I'll sponsor you.

Regards,

Hans

p.s.

I guess you should drop the chmod -x I made you add, given Kevin comments. Please do this either before importing the package into git, or change it in git before building the package.
Comment 14 Mattia Verga 2012-01-02 11:37:59 EST
> 
> This looks good now -> approved!
> 
> If you create a FAS account (if you've not done so already) and let me know
> your FAS login then I'll sponsor you.
> 
> Regards,
> 
> Hans
> 
> p.s.
> 
> I guess you should drop the chmod -x I made you add, given Kevin comments.
> Please do this either before importing the package into git, or change it in
> git before building the package.

Thank you all, I will correct the SRPM before importing it into git.
My FAS login is "mattia" (I really lack fantasy....). After being sponsored I think I must apply for ownership of the package: can I do that directly in the Fedora Package Database web interface or should I use this bug as described in https://fedoraproject.org/wiki/Package_SCM_admin_requests#Package_Change_Requests_for_existing_packages ?
Comment 15 Rex Dieter 2012-01-02 12:04:37 EST
that's correct.
Comment 16 Mattia Verga 2012-01-05 12:12:43 EST
Hans, do I need to do something more before being sponsored or you missed my reply in comment #14 to your request of my FAS login?
Thank you.
Comment 17 Hans de Goede 2012-01-06 07:40:24 EST
(In reply to comment #16)
> Hans, do I need to do something more before being sponsored or you missed my
> reply in comment #14 to your request of my FAS login?

I missed your reply. I've added you to the packager group in FAS and sponsored you now. So you can now continue with the next steps of:
http://fedoraproject.org/wiki/PackageMaintainers/Join
Comment 18 Mattia Verga 2012-01-06 10:06:29 EST
Package Change Request
======================
Package Name: kde-partitionmanager
Owners: [Orphaned]

I would like to own the orphaned package 'kde-partitionmanager'. My FAS login is 'mattia'. Thanks.
Comment 19 Jon Ciesla 2012-01-06 10:16:34 EST
I'm not sure what you need here.  You already own this.
Comment 20 Kevin Kofler 2012-01-06 10:19:37 EST
The procedure to take ownership of packages which are orphaned but not retired is to log in at:
https://admin.fedoraproject.org/pkgdb/acls/name/kde-partitionmanager
and take ownership of the package. I see that either you have already done that or an administrator has done that for you. So no (further) admin action is needed. (I see Jon Ciesla has already cleared the fedora-cvs flag.)

By the way, rereviews are normally not needed for packages which recently got orphaned and are not retired yet. (In this case, you had to get sponsored though. But now that you are sponsored, you can just claim (recently) orphaned packages.)
Comment 21 Mattia Verga 2012-01-06 15:15:37 EST
Thank you Hans, imported and builded into koji. I close the ticket.

In the future, if you need some review, I will be glad to help. ;-)

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