Bug 1245604 - Review Request: vim-go - Go development plugin for Vim
Summary: Review Request: vim-go - Go development plugin for Vim
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Zbigniew Jędrzejewski-Szmek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-07-22 11:22 UTC by Nikola Forró
Modified: 2015-08-11 02:12 UTC (History)
5 users (show)

Fixed In Version: vim-go-1.1-1.fc24
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-07-29 11:05:26 UTC
Type: ---
Embargoed:
zbyszek: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Nikola Forró 2015-07-22 11:22:06 UTC
Spec URL: https://nforro.fedorapeople.org/vim-go.spec
SRPM URL: https://nforro.fedorapeople.org/vim-go-1.0.5-1.fc22.src.rpm

Koji scratchbuild URL: http://koji.fedoraproject.org/koji/taskinfo?taskID=10435031

Description:
Go (golang) support for Vim. It comes with predefined sensible settings (like auto gofmt on save), has auto-complete, snippet support, improved syntax highlighting, go tool-chain commands, etc... If needed vim-go installs all necessary binaries for providing seamless Vim integration with current commands. It's highly customizable and each individual feature can be disabled/enabled easily.

Fedora Account System Username: nforro

rpmlint output:
$ rpmlint vim-go.spec SRPMS/vim-go-1.0.5-1.fc22.src.rpm 
vim-go.src: W: spelling-error %description -l en_US golang -> Angolan, Golan, Angola
vim-go.src: W: spelling-error %description -l en_US gofmt -> Goff
vim-go.src: W: spelling-error %description -l en_US customizable -> customization
1 packages and 1 specfiles checked; 0 errors, 3 warnings.

Comment 1 Nikola Forró 2015-07-23 07:43:46 UTC
Update: removed unneeded rm command in %postun section

Spec URL: https://nforro.fedorapeople.org/vim-go.spec
SRPM URL: https://nforro.fedorapeople.org/vim-go-1.0.5-2.fc22.src.rpm

Comment 2 Nikola Forró 2015-07-23 12:24:40 UTC
Update: require golang
        added command to %postun to clear vim tags

Spec URL: https://nforro.fedorapeople.org/vim-go.spec
SRPM URL: https://nforro.fedorapeople.org/vim-go-1.0.5-3.fc22.src.rpm

Comment 3 Zbigniew Jędrzejewski-Szmek 2015-07-23 14:28:58 UTC
Looks good.

- Name matches https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Addon_Packages_.28General.29
- license is OK

License file is not installed. Add %license LICENSE
[https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text].

%post/%postun scriplets should be suffixed with || : so that they cannot fail.

I'm not a vim expert, but comparing the scriptlets with vim-taglist I see that vim-taglist removes the file, but you truncate it... Is this intentional?

I don't think use should mark %{vimfiles_root}/doc/* with %doc. Packages are not allowed to use stuff marked with %doc at runtime, but this help is used by vim itself, no?

Who owns %{vimfiles_root}/autoload, %{vimfiles_root}/compiler, %{vimfiles_root}/ftdetect, etc? Most likely your package should (co-)own those directories.

Comment 4 Nikola Forró 2015-07-24 07:03:45 UTC
(In reply to Zbigniew Jędrzejewski-Szmek from comment #3)
> License file is not installed. Add %license LICENSE
> [https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text].
>

There is no license file in the tarball. It was added to upstream master branch later, so it will probably be included in the next release.
 
> %post/%postun scriplets should be suffixed with || : so that they cannot
> fail.
> 

Fixed.

> I'm not a vim expert, but comparing the scriptlets with vim-taglist I see
> that vim-taglist removes the file, but you truncate it... Is this
> intentional?
> 

With rm, rpmlint complains about dangerous command in %postun section.

> I don't think use should mark %{vimfiles_root}/doc/* with %doc. Packages are
> not allowed to use stuff marked with %doc at runtime, but this help is used
> by vim itself, no?
> 

That's right, fixed.

> Who owns %{vimfiles_root}/autoload, %{vimfiles_root}/compiler,
> %{vimfiles_root}/ftdetect, etc? Most likely your package should (co-)own
> those directories.

Those directories are owned by vim-filesystem package, which is "artificial filesystem" package and it also exists in vim-go's dependency chain, so vim-go shouldn't co-own them.

Updated files:
Spec URL: https://nforro.fedorapeople.org/vim-go.spec
SRPM URL: https://nforro.fedorapeople.org/vim-go-1.0.5-4.fc22.src.rpm

Comment 5 Vít Ondruch 2015-07-24 08:39:39 UTC
I would suggest to add .metainfo.xml for Gnome Software [1]. You can find inspiration in my vim-command-t package.



[1] https://fedoraproject.org/wiki/Packaging:AppData#.metainfo.xml_file_creation
[2] http://pkgs.fedoraproject.org/cgit/vim-command-t.git/tree/

Comment 6 Zbigniew Jędrzejewski-Szmek 2015-07-24 14:28:09 UTC
(In reply to Nikola Forró from comment #4)
> (In reply to Zbigniew Jędrzejewski-Szmek from comment #3)
> > License file is not installed. Add %license LICENSE
> > [https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text].
> >
> 
> There is no license file in the tarball. It was added to upstream master
> branch later, so it will probably be included in the next release.
OK.

> > I'm not a vim expert, but comparing the scriptlets with vim-taglist I see
> > that vim-taglist removes the file, but you truncate it... Is this
> > intentional?
> With rm, rpmlint complains about dangerous command in %postun section.
rpmlint is quite often wrong. But if your version works, that is fine.

> > Who owns %{vimfiles_root}/autoload, %{vimfiles_root}/compiler,
> > %{vimfiles_root}/ftdetect, etc? Most likely your package should (co-)own
> > those directories.
> 
> Those directories are owned by vim-filesystem package, which is "artificial
> filesystem" package and it also exists in vim-go's dependency chain, so
> vim-go shouldn't co-own them.
Pff, I checked that with repoquery but must have messed something up.
It is as you say.

(In reply to Vít Ondruch from comment #5)
> I would suggest to add .metainfo.xml for Gnome Software [1]. You can find
> inspiration in my vim-command-t package.
Good point, please add the metadata file.

I now filed bugs against other vim plugins to do the same: https://bugzilla.redhat.com/buglist.cgi?quicksearch=vim%20%22please%20add%20appdata%20metainfo%20file%22 .

> Updated files:
> Spec URL: https://nforro.fedorapeople.org/vim-go.spec
> SRPM URL: https://nforro.fedorapeople.org/vim-go-1.0.5-4.fc22.src.rpm
- license is OK
- name is OK
- description is OK
- license file is not present but will be added in next release
- layout is good and all directories have ownership
- spec file is good
- scriptlets are present
- rpmlint only false positive typos
- requires and provides look OK

Package is APPROVED. Please add the metadata file.

Comment 7 Zbigniew Jędrzejewski-Szmek 2015-07-26 15:36:51 UTC
One more thing: golang is not available everywhere, so your package should not be built for architectures where the dependencies cannot be satisfied anyway:

ExclusiveArch: %{go_arches} noarch

(taken from https://fedoraproject.org/wiki/PackagingDrafts/Go#Libraries_and_Arch)

Comment 8 Nikola Forró 2015-07-28 12:14:25 UTC
(In reply to Vít Ondruch from comment #5)
> I would suggest to add .metainfo.xml for Gnome Software [1]. You can find
> inspiration in my vim-command-t package.
> 
> 
> 
> [1]
> https://fedoraproject.org/wiki/Packaging:AppData#.metainfo.xml_file_creation
> [2] http://pkgs.fedoraproject.org/cgit/vim-command-t.git/tree/
Added.

(In reply to Zbigniew Jędrzejewski-Szmek from comment #7)
> One more thing: golang is not available everywhere, so your package should
> not be built for architectures where the dependencies cannot be satisfied
> anyway:
> 
> ExclusiveArch: %{go_arches} noarch
> 
> (taken from
> https://fedoraproject.org/wiki/PackagingDrafts/Go#Libraries_and_Arch)
Fixed.

Spec URL: https://nforro.fedorapeople.org/vim-go.spec
Metainfo URL: https://nforro.fedorapeople.org/vim-go.metainfo.xml
SRPM URL: https://nforro.fedorapeople.org/vim-go-1.0.5-5.fc22.src.rpm

Comment 9 Nikola Forró 2015-07-28 12:58:51 UTC
New Package SCM Request
=======================
Package Name: vim-go
Short Description: Go development plugin for Vim
Upstream URL: https://github.com/fatih/vim-go
Owners: nforro jchaloup
Branches: f21 f22 f23

Comment 10 Gwyn Ciesla 2015-07-28 13:12:11 UTC
Git done (by process-git-requests).

Comment 11 Fedora Update System 2015-07-29 10:39:55 UTC
vim-go-1.1-1.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/vim-go-1.1-1.fc21

Comment 12 Fedora Update System 2015-07-29 10:42:20 UTC
vim-go-1.1-1.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/vim-go-1.1-1.fc22

Comment 13 Nikola Forró 2015-07-29 11:05:26 UTC
Thanks for the review.

Comment 14 Fedora Update System 2015-08-11 02:07:35 UTC
vim-go-1.1-1.fc21 has been pushed to the Fedora 21 stable repository.

Comment 15 Fedora Update System 2015-08-11 02:12:49 UTC
vim-go-1.1-1.fc22 has been pushed to the Fedora 22 stable repository.


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