Bug 433182 - Review Request: pcmanfm - Extremly fast and lightweight file manager
Review Request: pcmanfm - Extremly fast and lightweight file manager
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Hans de Goede
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-02-17 02:34 EST by Mamoru TASAKA
Modified: 2008-02-18 14:26 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-02-18 14:26:04 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
hdegoede: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
PATCH fixing 64 bit crash (380 bytes, patch)
2008-02-17 14:23 EST, Hans de Goede
no flags Details | Diff

  None (edit)
Description Mamoru TASAKA 2008-02-17 02:34:55 EST
Spec URL: http://mtasaka.fedorapeople.org/Review_request/pcmanfm/pcmanfm.spec
SRPM URL: http://mtasaka.fedorapeople.org/Review_request/pcmanfm/pcmanfm-0.3.5.23-1.fc8.src.rpm
Description: 
PCMan File Manager is an extremly fast and lightweight file manager 
which features tabbed browsing and user-friendly interface.

koji scratch build results:
for dist-f9:
http://koji.fedoraproject.org/koji/taskinfo?taskID=432895
for dist-f8-updates-candidate:
http://koji.fedoraproject.org/koji/taskinfo?taskID=432894
Comment 1 Hans de Goede 2008-02-17 14:21:48 EST
I've done a full review, result below.

<humor>
Unfortunately I cannot approve this package as every package should have atleast
one patch. Luckily I found a 64 bit bug, causing the package to fail to work
(crash) on 64 bit. So I've written a patch fixing this so that we can meet the
every package should have atleast one patch rule.
</humor>

MUST FIX
--------
* Apply the to be attached patch fixing crashing on 64 bit. The patch may look 
  strange for a 64 bit patch, but it really fixes a 64 bit issue:
  -The header in question contains guards to protect it from multiple inclusion
  -These guards where copy pasted from another header without changing the
   define checked and set, therefor this header things its already included
   while its not
  -This causes various prototypes to not be declared, and the compiler to assume
   their return type is int (aka 32 bits)
  -However some return a pointer, which should be 64 bit and now gets truncated 
   to 32 bits.
  -Boom (aka crash)

Should Fix
----------
* Send the 64 bit patch upstream
* Ask upstream to use freedesktop.org icon installation location instead of
  obsolete /usr/share/pixmaps

Although there is a MUST FIX, I trust you to include the patch so I'm going to
set the approved flag, please include the patch before building the first time.
Comment 2 Hans de Goede 2008-02-17 14:22:20 EST
Like I said I'm going to set the approved flag, second try :)
Comment 3 Hans de Goede 2008-02-17 14:23:10 EST
Created attachment 295109 [details]
PATCH fixing 64 bit crash
Comment 4 Mamoru TASAKA 2008-02-18 10:54:27 EST
Thank you for the patch! The upstream developer replied that he
applied the fix on svn.
Also upstream asked me:
--------------------------------------------------------------------------
Some debian users reported that mounting removable devices (usb disk, cdrom)
in pcmanfm 0.3.5.23 from the side pane (with --enable-hal turned on) will
cause crashes (seg. faults).  However, we cannot reproduce the bug on ubuntu
by any means, and from the gdb backtrace, the crash happened in libhal.
We have no idea how could this happens. Would you please get this
tested on Fedora, and I wish that good programmer can help reviewing
this part. It's mainly in src/vfs/vfs-volume-hal.c, and most of the source code
was taken from gnome-mount and exo-mount of xfce.
This is the main blocker of 0.3.6 release. We'll make this stable release
after this potential bug get resolved.
------------------------------------------------------------------------------
This bug seems reproducible to me although for now I am not sure
if this bug is caused really by hal side. For now I disabled to
mount removal devices by pcmanfm by a patch.

http://mtasaka.fedorapeople.org/Review_request/pcmanfm/pcmanfm.spec
http://mtasaka.fedorapeople.org/Review_request/pcmanfm/pcmanfm-0.3.5.23-2.fc8.src.rpm
* Mon Feb 18 2008 Mamoru Tasaka <mtasaka@ioa.s.u-tokyo.ac.jp> - 0.3.5.23-2
- Apply patch to fix crash on 64bits arch as suggested by Hans
  (bug 433182)
- Disable to mount removable devices for now

---------------------------------------------------------------------------
New Package CVS Request
=======================
Package Name:      pcmanfm
Short Description: Extremly fast and lightweight file manager
Owners:            mtasaka
Branches:          F-8 F-7
InitialCC: 
Cvsextras Commits: yes
Comment 5 Kevin Fenzi 2008-02-18 12:47:18 EST
cvs done.
Comment 6 Mamoru TASAKA 2008-02-18 14:26:04 EST
Successfully built on all branches, requested on bodhi, closing.

Thank you for reviewing this package and for CVS procedure!

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