Bug 433182 - Review Request: pcmanfm - Extremly fast and lightweight file manager
Summary: Review Request: pcmanfm - Extremly fast and lightweight file manager
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Hans de Goede
QA Contact: Fedora Extras Quality Assurance
Depends On:
TreeView+ depends on / blocked
Reported: 2008-02-17 07:34 UTC by Mamoru TASAKA
Modified: 2008-02-18 19:26 UTC (History)
2 users (show)

Clone Of:
Last Closed: 2008-02-18 19:26:04 UTC
hdegoede: fedora-review+
kevin: fedora-cvs+

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

Description Mamoru TASAKA 2008-02-17 07:34:55 UTC
Spec URL: http://mtasaka.fedorapeople.org/Review_request/pcmanfm/pcmanfm.spec
SRPM URL: http://mtasaka.fedorapeople.org/Review_request/pcmanfm/pcmanfm-
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:
for dist-f8-updates-candidate:

Comment 1 Hans de Goede 2008-02-17 19:21:48 UTC
I've done a full review, result below.

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.

* 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 19:22:20 UTC
Like I said I'm going to set the approved flag, second try :)

Comment 3 Hans de Goede 2008-02-17 19:23:10 UTC
Created attachment 295109 [details]
PATCH fixing 64 bit crash

Comment 4 Mamoru TASAKA 2008-02-18 15:54:27 UTC
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 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.

* Mon Feb 18 2008 Mamoru Tasaka <mtasaka@ioa.s.u-tokyo.ac.jp> -
- 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
Cvsextras Commits: yes

Comment 5 Kevin Fenzi 2008-02-18 17:47:18 UTC
cvs done.

Comment 6 Mamoru TASAKA 2008-02-18 19:26:04 UTC
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.