Bug 562977 - Review Request: vifm - Lightweight file manager with vi like keybindings
Summary: Review Request: vifm - Lightweight file manager with vi like keybindings
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-02-08 22:03 UTC by Pierre Dorbais
Modified: 2010-04-19 16:55 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-04-19 16:55:34 UTC
mtasaka: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
Patch to fix source code (comment 2) (907 bytes, patch)
2010-04-12 16:56 UTC, Pierre Dorbais
no flags Details | Diff

Description Pierre Dorbais 2010-02-08 22:03:07 UTC
Spec URL: http://chdorblog.free.fr/vifm.spec
SRPM URL: http://chdorblog.free.fr/vifm-0.5-1.fc12.src.rpm
Description: A ncurses based CLI file manager with vi like keybindings

[chdorb@chdorb-desktop rpmbuild]$ rpmlint SRPMS/vifm-0.5-1.fc12.src.rpm \
> RPMS/i686/vifm-0.5-1.fc12.i686.rpm \
> RPMS/i686/vifm-debuginfo-0.5-1.fc12.i686.rpm \
> SPECS/vifm.spec
3 packages and 1 specfiles checked; 0 errors, 0 warnings.

Comment 1 Pierre Dorbais 2010-02-09 21:24:40 UTC
Hi

I packaged up my second package vifm, please review. I also need a sponsor for this and ciso (bug #524423).

Comment 2 Mamoru TASAKA 2010-04-11 18:39:39 UTC
Some notes:

! BuildRoot
  - BuildRoot is no longer needed for Fedora (rpmlint may complain, however
    it can be ignored)
    https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag

* Timestamps
  - Please consider to use
----------------------------------------------------------------------------------
make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p"
----------------------------------------------------------------------------------
    to keep timestamps on installed files as much as possible.
    This method usually works for Makefiles generated by recent
    autotools.

! Documents
  ! Usually INSTALL file is for people trying to compile and install
    software by themselves and is not needed for people using rpm,
    however for this package I will leave it to how you think whether
    INSTALL file should be packaged or not.

* Directory ownership issue
  https://fedoraproject.org/wiki/Packaging/UnownedDirectories#Common_Mistakes
  - The directory %{_datadir}/%{name} itself is not owned by any packages.

* Source codes themselves
  - Some codes hardcode /usr/local for installed paths.
    ! For example from config.c:
---------------------------------------------------------------------------------
    19  #define CP_HELP "cp /usr/local/share/vifm/vifm-help.txt ~/.vifm"
    20  #define CP_RC "cp /usr/local/share/vifm/vifmrc ~/.vifm"
---------------------------------------------------------------------------------
      With this, launching vifm causes warnings like:
---------------------------------------------------------------------------------
 env LANG=C vifm
cp /usr/local/share/vifm/vifm-help.txt ~/.vifmcp: cannot stat `/usr/local/share/vifm/vifm-help.txt': No such file or directory
Unable to find configuration file.
 Using defaults.cp: cannot stat `/usr/local/share/vifm/vifmrc': No such file or directory
---------------------------------------------------------------------------------
     This needs fixing.

  - config.c reads:
---------------------------------------------------------------------------------
   144                  if(chdir(cfg.config_dir))
   145                  {
   146                          if(mkdir(cfg.config_dir, 0777))
   147                                  return;
   148                          if(mkdir(cfg.trash_dir, 0777))
   149                                  return;
   150                          if((f = fopen(help_file, "r")) == NULL)
   151                                  create_help_file();
   152                          if((f = fopen(rc_file, "r")) == NULL)
   153                                  create_rc_file();
   154                  }
---------------------------------------------------------------------------------
    Here "0700" is much safer.

Comment 3 Pierre Dorbais 2010-04-12 16:56:09 UTC
Created attachment 406015 [details]
Patch to fix source code (comment 2)

Comment 4 Pierre Dorbais 2010-04-12 17:00:06 UTC
Thank you.

I've updated spec file and created a patch to fix source code.

Spec URL: http://chdorblog.free.fr/vifm.spec
SRPM URL: http://chdorblog.free.fr/vifm-0.5-2.fc12.src.rpm

Comment 5 Pierre Dorbais 2010-04-12 17:16:33 UTC
[chdorb@chdorb-desktop rpmbuild]$ rpmlint SRPMS/vifm-0.5-2.fc12.src.rpm \
> RPMS/i686/vifm-0.5-2.fc12.i686.rpm \
> RPMS/i686/vifm-debuginfo-0.5-2.fc12.i686.rpm \
> SPECS/vifm.spec
vifm.src: W: spelling-error %description -l en_US ncurses -> nurses, curses, n curses
vifm.src: W: no-buildroot-tag
vifm.i686: W: spelling-error %description -l en_US ncurses -> nurses, curses, n curses
SPECS/vifm.spec: W: no-buildroot-tag
3 packages and 1 specfiles checked; 0 errors, 4 warnings.

Comment 6 Mamoru TASAKA 2010-04-12 18:20:18 UTC
For -2:

One thing

* Duplicate file entry
  - Now build.log shows:
-------------------------------------------------------------
   268  warning: File listed twice: /usr/share/vifm/vifm-help.txt
   269  warning: File listed twice: /usr/share/vifm/vifm.txt
   270  warning: File listed twice: /usr/share/vifm/vifm.vim
-------------------------------------------------------------
    Please make it sure that every file is listed only once.

  ! Note:
    - The following %files entry
-------------------------------------------------------------
%files
%{_datadir}/%{name}
-------------------------------------------------------------
      contains the directory %{_datadir}/%{name} _and_ all
      files/directories/etc under this directory, while
-------------------------------------------------------------
%files
%dir %{_datadir}/%{name}
-------------------------------------------------------------
      contains the directory %{_datadir}/%{name} only.

Comment 7 Mamoru TASAKA 2010-04-12 18:38:18 UTC
One more thing
* %changelog
  - It is better that you put one line between each %changelog
    entry like:
-----------------------------------------------------------
%changelog
* Mon Apr 12 2010 Pierre Dorbais <pierre.dorbais@free.fr> 0.5-2
- Add INSTALL variable to make
- Add patch Patch0
- Add missing path to files macro
- Remove INSTALL doc file
- Remove BuildRoot tag

* Mon Feb 08 2010 Pierre Dorbais <pierre.dorbais@free.fr> 0.5-1
- Initial RPM release
-----------------------------------------------------------
    Especially this is useful on Fedora CVS.

Comment 8 Pierre Dorbais 2010-04-12 20:44:19 UTC
Done

Comment 9 Mamoru TASAKA 2010-04-13 12:28:45 UTC
Would you post your new spec / srpm ?

Comment 10 Pierre Dorbais 2010-04-13 20:43:54 UTC
Ho sorry :s

Spec URL: http://chdorblog.free.fr/vifm.spec
SRPM URL: http://chdorblog.free.fr/vifm-0.5-3.fc12.src.rpm

Comment 11 Mamoru TASAKA 2010-04-14 17:03:19 UTC
Well,

- This package is now in good shape
- Your another review request (bug 524423) is perhaps in good
  shape to some extent at least (I will check it later)

-----------------------------------------------------
  This package (vifm) is APPROVED by mtasaka
------------------------------------------------------

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Install the Client Tools (Koji)".

Now I am sponsoring you.

If you want to import this package into Fedora 11/12/13, you also have
to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.

Removing NEEDSPONSOR.

Comment 12 Pierre Dorbais 2010-04-14 21:27:58 UTC
Thank you very much for help and sponsor me !

Comment 13 Pierre Dorbais 2010-04-14 21:28:45 UTC
New Package CVS Request
=======================
Package Name: vifm
Short Description: Lightweight file manager with vi like key-bindings
Owners: chdorb
Branches: F-11 F-12 F-13
InitialCC:

Comment 14 Kevin Fenzi 2010-04-14 21:48:08 UTC
CVS done (by process-cvs-requests.py).

Comment 15 Fedora Update System 2010-04-15 20:07:57 UTC
vifm-0.5-3.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/vifm-0.5-3.fc11

Comment 16 Mamoru TASAKA 2010-04-16 17:37:59 UTC
Please build also for F-12/13 (and submit push requests on bodhi)

Comment 17 Fedora Update System 2010-04-16 23:49:50 UTC
vifm-0.5-3.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update vifm'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/vifm-0.5-3.fc11

Comment 18 Fedora Update System 2010-04-17 09:53:04 UTC
vifm-0.5-3.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/vifm-0.5-3.fc12

Comment 19 Fedora Update System 2010-04-17 09:54:49 UTC
vifm-0.5-3.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/vifm-0.5-3.fc13

Comment 20 Pierre Dorbais 2010-04-17 09:57:59 UTC
I wanted to view all the process for F-11 to be sure I am right before build and push for F-12/F-13.
It's now also done for F-12 and F-13.

Comment 21 Mamoru TASAKA 2010-04-19 16:55:34 UTC
Closing.


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