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
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.
Like I said I'm going to set the approved flag, second try :)
Created attachment 295109 [details] PATCH fixing 64 bit crash
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.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
cvs done.
Successfully built on all branches, requested on bodhi, closing. Thank you for reviewing this package and for CVS procedure!