Bug 2010116 - Review Request: vim-surround - Delete/change/add parentheses/quotes/XML-tags/much more with ease
Summary: Review Request: vim-surround - Delete/change/add parentheses/quotes/XML-tags/...
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Carl George 🤠
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-NEEDSPONSOR FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2021-10-03 21:00 UTC by Magnus Morton
Modified: 2022-05-30 00:45 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2022-05-30 00:45:21 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Magnus Morton 2021-10-03 21:00:01 UTC
Spec URL: https://magnus.morton.ai/packaging/vim-surround.spec
SRPM URL: https://magnus.morton.ai/packaging/vim-surround-2.1-1.fc35.src.rpm
Description:
Surround.vim is all about "surroundings": parentheses, brackets, quotes, XML
tags, and more.  The plugin provides mappings to easily delete, change and add
such surroundings in pairs.

This plugin is very powerful for HTML and XML editing, a niche which currently
seems underfilled in Vim land.  (As opposed to HTML/XML *inserting*, for which
many plugins are available).  Adding, changing, and removing pairs of tags
simultaneously is a breeze.

Fedora Account System Username: jaffachief

Koji Build: https://koji.fedoraproject.org/koji/taskinfo?taskID=76651532

This is my first package and I would need sponsored.

Comment 1 Maxwell G 2021-10-14 22:24:57 UTC
I don't have any pointers, but I am hopeful that you find a sponsor and get your package approved soon, because I would really like to have this packaged in Fedora! Unfortunately, I am also a new packager, so I can't sponsor you myself.

-- Maxwell

Comment 2 Robby Callicotte 2021-10-18 22:27:14 UTC
Hello,

I am a new packager, so this is a an unofficial review.

> %install                                                                                                                                                                                     
> mkdir -p %{buildroot}%{vimfiles_root}                                                                                                                                                        
> cp -pr doc plugin %{buildroot}%{vimfiles_root}                                                                                                                                               
>                                                                                                                                                                                              
> # Install AppData.                                                                                                                                                                           
> mkdir -p %{buildroot}%{appdata_dir}                                                                                                                                                          
> install -m 644 %{SOURCE1} %{buildroot}%{appdata_dir}

I believe this section should reference the macros where applicable. (ie %{__mkdir_p}, %{__cp}, %{__install}).  This is a stylistic choice.


> %files                                                                                                                                                                                       
> %doc README.markdown                                                                                                                                                                         
> %{vimfiles_root}/doc/*                                                                                                                                                                       
> %{vimfiles_root}/plugin/*                                                                                                                                                                    
> %{appdata_dir}/vim-surround.metainfo.xml

The contents of the tarball look like this:
vim-surround-2.1
├── doc
│   └── surround.txt
├── plugin
│   └── surround.vim
└── README.markdown

The %files section looks like it would assign ownership of all files under %{vimfiles_root}/doc/ and %{vimfiles_root}/plugin/ to this package.  The spec should directly reference the names of these files.  This will prevent file ownership conflicts.

Try this:
%{vimfiles_root}/doc/surround.txt
%{vimfiles_root}/plugin/surround.vim

--Robby

Comment 3 Carl George 🤠 2022-01-26 05:34:33 UTC
> I believe this section should reference the macros where applicable. (ie %{__mkdir_p}, %{__cp}, %{__install}).  This is a stylistic choice.

Macro forms of system executables SHOULD NOT be used except when there is a need to allow the location of those executables to be configurable.  mkdir, cp, and install are the right choice here.

https://docs.fedoraproject.org/en-US/packaging-guidelines/#_macros


> The spec should directly reference the names of these files.

Agreed.  The Python guidelines state this explicitly, but it's also a good general guideline.  I'm probably going to work on getting a generic form into the main guidelines.

https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_explicit_lists


Here are a few additional things I noticed that should be fixed.


The metainfo file should be installed into %{_metainfodir}.

-%{appdata_dir}/vim-surround.metainfo.xml
+%{_metainfodir}/vim-surround.metainfo.xml

https://docs.fedoraproject.org/en-US/packaging-guidelines/AppData/


It's not directly mentioned in the guidelines, but similar to the patch status guideline, there should be a comment for Source1 linking to where the metainfo file was submitted upstream.

https://docs.fedoraproject.org/en-US/packaging-guidelines/PatchUpstreamStatus/


The %post and %postun scriptlets are no longer necessary on Fedora or EPEL8+ due to file triggers in the main vim package.  If and only if you plan to add this package to EPEL7, you can keep them with a conditional so they only apply on EPEL7.  I suggest following the example of the docfiletriggers conditional in the vim-fugitive spec file.  If you remove them, make sure to also remove the corresponding Requires(post) and Requires(postun).

https://src.fedoraproject.org/rpms/vim/c/d76e3c95ac644b1ab577bf4db79fb055f218724d
https://src.fedoraproject.org/rpms/vim-fugitive/blob/rawhide/f/vim-fugitive.spec


Since the review was submitted, upstream has released version 2.2, so the spec file should be updated to that.

https://github.com/tpope/vim-surround/releases/tag/v2.2

Comment 4 Carl George 🤠 2022-04-29 19:14:34 UTC
> I'm probably going to work on getting a generic form into the main guidelines.

This is now in the main guidelines.

https://docs.fedoraproject.org/en-US/packaging-guidelines/#_explicit_lists

Magnus, have you had a chance to incorporate the fixes suggested here to the spec file yet?

Comment 5 Package Review 2022-05-30 00:45:21 UTC
This is an automatic action taken by review-stats script.

The ticket submitter failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we consider this ticket as DEADREVIEW and proceed to close it.


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