Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 1394789 - Review Request: neovim - Vim-fork focused on extensibility and agility
Summary: Review Request: neovim - Vim-fork focused on extensibility and agility
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jakub Hrozek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 1327160 1327198 1327218 1394785 1394788
Blocks: 1401414
TreeView+ depends on / blocked
 
Reported: 2016-11-14 13:10 UTC by Andreas Schneider
Modified: 2016-12-16 00:25 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-12-15 23:30:52 UTC
Type: ---
Embargoed:
jhrozek: fedora-review+

Attachments (Terms of Use)

Description Andreas Schneider 2016-11-14 13:10:27 UTC 
Spec URL: https://xor.cryptomilk.org/rpm/neovim/neovim.spec
SRPM URL: https://xor.cryptomilk.org/rpm/neovim/neovim-0.1.6-1.fc24.src.rpm

Description:
Neovim is a refactor - and sometimes redactor - in the tradition of
Vim, which itself derives from Stevie. It is not a rewrite, but a
continuation and extension of Vim. Many rewrites, clones, emulators
and imitators exist; some are very clever, but none are Vim. Neovim
strives to be a superset of Vim, notwithstanding some intentionally
removed misfeatures; excepting those few and carefully-considered
excisions, Neovim is Vim. It is built for users who want the good
parts of Vim, without compromise, and more.

Fedora Account System Username: asn
Comment 1 Yunying Sun 2016-11-15 09:31:07 UTC 
An informal review:
1. Group & BuildRoot tag should not be used, according to "Tags and Sections" at http://fedoraproject.org/wiki/Packaging:Guidelines
2. File lists is quite long. If all files under a folder are included, is it enough to include the folder only? that is, use:
%{_datadir}/nvim/runtime
instead of:
%dir %{_datadir}/nvim/runtime
3. Better to have rpmlint result and koji build link here.

I'm having 2 new packages under review, bug 1369708 and 1369720. Would like a review-swap if possible.
Comment 2 Andreas Schneider 2016-11-15 11:28:32 UTC 
Spec URL: https://xor.cryptomilk.org/rpm/neovim/neovim.spec
SRPM URL: https://xor.cryptomilk.org/rpm/neovim/neovim-0.1.6-2.fc24.src.rpm

I've removed Group and BuildRoot. I prefer a complete file list, then I know what is going on. Which files get added and which have been removed.

Koji is not possible, see the dependencies.
Comment 3 Andreas Schneider 2016-11-28 16:25:27 UTC 
We do not need lua-bitop it is a builtin of luajit which neovim uses.
Comment 4 Andreas Schneider 2016-11-28 17:04:08 UTC 
Spec URL: https://xor.cryptomilk.org/rpm/neovim/neovim.spec
SRPM URL: https://xor.cryptomilk.org/rpm/neovim/neovim-0.1.7-1.fc25.src.rpm
Comment 5 fszymanski 2016-12-03 07:42:22 UTC 
Scratch build failed:
http://koji.fedoraproject.org/koji/taskinfo?taskID=16716812
Comment 6 fszymanski 2016-12-03 07:59:24 UTC 
By the way jemalloc-devel build requires is missing.
Comment 7 Andreas Schneider 2016-12-05 08:47:07 UTC 
Spec URL: https://xor.cryptomilk.org/rpm/neovim/neovim.spec
SRPM URL: https://xor.cryptomilk.org/rpm/neovim/neovim-0.1.7-2.fc25.src.rpm
Comment 8 Andreas Schneider 2016-12-05 09:22:07 UTC 
Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=16754838
Comment 9 fszymanski 2016-12-05 09:30:18 UTC 
Use gettext instead of gettext-devel.

Quick question. Do we need luajit-devel (we are using the Lua interpreter)?
Comment 10 fszymanski 2016-12-05 11:21:15 UTC 
One more thing:

BuildRequires: gcc

C and C++ packaging guidelines:
https://fedoraproject.org/wiki/Packaging:C_and_C%2B%2B#BuildRequires_and_Requires
Comment 11 fszymanski 2016-12-05 11:38:56 UTC 
This is simpler

%build
mkdir -p obj

than

%build
if test ! -e "obj"; then
  mkdir obj
fi
Comment 12 Andreas Schneider 2016-12-05 13:20:13 UTC 
Well luajit would be better, but luajit assumes lua 5.1:

LuaJIT 2.0.4 -- Copyright (C) 2005-2015 Mike Pall. http://luajit.org/
JIT: ON CMOV SSE2 SSE3 SSE4.1 fold cse dce fwd dse narrow loop abc sink fuse
> require("lpeg")
stdin:1: module 'lpeg' not found:
        no field package.preload['lpeg']
        no file './lpeg.lua'
        no file '/usr/share/luajit-2.0.4/lpeg.lua'
        no file '/usr/local/share/lua/5.1/lpeg.lua'
        no file '/usr/local/share/lua/5.1/lpeg/init.lua'
        no file '/usr/share/lua/5.1/lpeg.lua'
        no file '/usr/share/lua/5.1/lpeg/init.lua'
        no file './lpeg.so'
        no file '/usr/local/lib/lua/5.1/lpeg.so'
        no file '/usr/lib64/lua/5.1/lpeg.so'
        no file '/usr/local/lib/lua/5.1/loadall.so'


So we need to use lua which is lua 5.3 and luajit seems to be incompatible.
Comment 13 Andreas Schneider 2016-12-05 13:23:43 UTC 
Spec URL: https://xor.cryptomilk.org/rpm/neovim/neovim.spec
SRPM URL: https://xor.cryptomilk.org/rpm/neovim/neovim-0.1.7-3.fc25.src.rpm
Comment 14 fszymanski 2016-12-05 13:30:18 UTC 
Looks good.
Comment 15 fszymanski 2016-12-05 13:59:44 UTC 
I noticed something else. The license file is in %doc and should be in %license:

%files
%license LICENSE
%doc ...
Comment 16 Andreas Schneider 2016-12-05 14:42:00 UTC 
I could swear I've already fixed that before ...
Comment 17 Andreas Schneider 2016-12-05 14:46:51 UTC 
Spec URL: https://xor.cryptomilk.org/rpm/neovim/neovim.spec
SRPM URL: https://xor.cryptomilk.org/rpm/neovim/neovim-0.1.7-4.fc25.src.rpm
Comment 18 Jakub Hrozek 2016-12-06 14:31:00 UTC 
Fedora-review says:

Issues:
=======
- All build dependencies are listed in BuildRequires, except for any that
  are listed in the exceptions section of Packaging Guidelines.
  Note: These BR are not needed: gcc
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2

Remove the gcc dependency and I'll approve :)
Comment 19 fszymanski 2016-12-06 14:42:13 UTC 
"If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. Those packages will include everything that is required to build a standards conforming C or C++ application."

From: https://fedoraproject.org/wiki/Packaging:C_and_C%2B%2B#Packaging
Comment 20 Jakub Hrozek 2016-12-06 14:56:26 UTC 
OK, strange that fedora-review explicitly flags gcc. I don't have any more comments, then.
Comment 21 fszymanski 2016-12-06 15:23:27 UTC 
I was thinking of changing Recommends python{2,3}-neovim to Suggests.
This way we get fewer dependencies (Python packages or only suggested not installed). They are not necessary to run neovim.

What do you think guys?
Comment 22 Gwyn Ciesla 2016-12-06 18:52:33 UTC 
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/neovim
Comment 23 Fedora Update System 2016-12-06 21:35:44 UTC 
neovim-0.1.7-4.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2016-b8f61d4abb
Comment 24 Fedora Update System 2016-12-08 04:55:02 UTC 
neovim-0.1.7-4.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-b8f61d4abb
Comment 25 Fedora Update System 2016-12-15 23:30:52 UTC 
neovim-0.1.7-4.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.
Comment 26 Fedora Update System 2016-12-16 00:25:57 UTC 
neovim-0.1.7-4.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.
Note You need to log in before you can comment on or make changes to this bug.