Bug 965007 - Review Request: gedit-trailsave - Gedit plugin who strip trailing whitespace on save
Review Request: gedit-trailsave - Gedit plugin who strip trailing whitespace ...
Status: CLOSED WONTFIX
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Björn 'besser82' Esser
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-20 05:42 EDT by Guillaume Kulakowski
Modified: 2014-03-01 18:24 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-08-21 10:56:02 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
besser82: fedora‑review?


Attachments (Terms of Use)
improved spec-file (3.18 KB, patch)
2013-06-14 08:00 EDT, Björn 'besser82' Esser
no flags Details | Diff

  None (edit)
Description Guillaume Kulakowski 2013-05-20 05:42:04 EDT
Gedit plugin who strip trailing whitespace on save

---

SPEC:
https://projects.llaumgui.com/p/rpmbuild/source/tree/master/SPECS/gedit-trailsave.spec

SRPM:
http://llaumgui.fedorapeople.org/review/gedit-trailsave/gedit-trailsave-3.1.2-1.fc18.src.rpm

rpmlint:
gedit-trailsave.x86_64: W: spelling-error Summary(en_US) whitespace -> white space, white-space, whites pace
gedit-trailsave.x86_64: W: spelling-error %description -l en_US whitespace -> white space, white-space, whites pace
gedit-trailsave.x86_64: E: no-binary
gedit-trailsave.x86_64: W: only-non-binary-in-usr-lib
gedit-trailsave.x86_64: E: script-without-shebang /usr/lib64/gedit/plugins/trailsave.py
gedit-trailsave.x86_64: E: script-without-shebang /usr/lib64/gedit/plugins/trailsave.plugin
gedit-trailsave-debuginfo.x86_64: E: empty-debuginfo-package
gedit-trailsave.src: W: spelling-error Summary(en_US) whitespace -> white space, white-space, whites pace
gedit-trailsave.src: W: spelling-error %description -l en_US whitespace -> white space, white-space, whites pace
gedit-trailsave.src: W: invalid-url Source0: https://github.com/jonleighton/gedit-trailsave/archive/a9b65a6358c07e41ea835bcd22cf1c99a8df470a/gedit-trailsave-3.1.2-a9b65a6.tar.gz The read operation timed out
3 packages and 0 specfiles checked; 4 errors, 6 warnings.
Comment 1 Germán Racca 2013-05-30 11:12:18 EDT
Hi Gillaume!

Some comments:

- This is a "noarch" package because you don't build anything (rpmlint complaints E: no-binary), so you must put "BuildArch: noarch" inside the spec file.

- As this is a noarch plugin, you shouldn't use %{_libdir} as the install dir; instead you should use %{_datadir}.

- You should create a directory called "trailsave" and install the plugin in it: %{_datadir}/trailsave/

- You should remove (unless you intend to maintain this package for EPEL5):
  - "BuildRoot" tag
  - "rm -rf %{buildroot}" from install section
  - the "%clean" section
  - "%defattr(-,root,root,-)"

- There is no a separate license file in the source package, so you should ask upstream to include it.

HTH somewhat,
Germán.
Comment 2 Germán Racca 2013-05-30 11:18:17 EDT
(In reply to Germán Racca from comment #1)

> - You should create a directory called "trailsave" and install the plugin in
> it: %{_datadir}/trailsave/

Sorry, install dir should be: %{_datadir}/gedit/plugins/trailsave/
Comment 3 Germán Racca 2013-05-30 12:05:55 EDT
> - There is no a separate license file in the source package, so you should ask > upstream to include it.

I think that the license text inside README.md is ok in this case!
Comment 4 Björn 'besser82' Esser 2013-06-11 16:37:50 EDT
I'll review during this week...
Comment 5 Björn 'besser82' Esser 2013-06-11 16:43:49 EDT
Spec-Url returns HTML-Document:

  INFO: Downloading .spec and .srpm files
  error: line 1: Unknown tag: <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML...
  ERROR: Exception down the road...

Please fix, e.g. upload spec to fedorapeople.org as well.
Comment 6 Guillaume Kulakowski 2013-06-11 17:08:38 EDT
> I'll review during this week...
I must apply before the german's recommandation.

> Spec-Url returns HTML-Document:
Yes, it's a forge. Raw files is here : https://projects.llaumgui.com/p/rpmbuild/source/file/master/SPECS/gedit-trailsave.spec
Comment 7 Björn 'besser82' Esser 2013-06-11 17:17:52 EDT
(In reply to Guillaume Kulakowski from comment #6)
> > I'll review during this week...
> I must apply before the german's recommandation.

Yes! Stereotypes must be maintained. ;P

> > Spec-Url returns HTML-Document:
> Yes, it's a forge. Raw files is here :
> https://projects.llaumgui.com/p/rpmbuild/source/file/master/SPECS/gedit-
> trailsave.spec

fedora-review likes it this way ( More German's stereotypes ahead XD ):

Spec URL: https://projects.llaumgui.com/p/rpmbuild/source/file/master/SPECS/gedit-trailsave.spec
SRPM URL: http://llaumgui.fedorapeople.org/review/gedit-trailsave/gedit-trailsave-3.1.2-1.fc18.src.rpm
Comment 8 Björn 'besser82' Esser 2013-06-12 06:58:55 EDT
As far as I can see in spec, it's basically unchanged since you reported for review.  Please add the changes proposed by Germán in comment #1 and comment #2.  I'll start full review then.
Comment 9 Germán Racca 2013-06-12 09:10:02 EDT
I'm glad you guys are following my stereotypes :)
Comment 10 Guillaume Kulakowski 2013-06-12 14:38:02 EDT
I look other Gedit plugin and no plugin is noarch because gedit is not noarch. Gedit take .plugin in /usr/lib64/gedit/plugins and can take other file in /usr/share/gedit plugin.

Are you sure for the Germán's recommandation about noarch ?
Comment 11 Björn 'besser82' Esser 2013-06-14 08:00:43 EDT
Created attachment 761229 [details]
improved spec-file

(In reply to Guillaume Kulakowski from comment #10)
> I look other Gedit plugin and no plugin is noarch because gedit is not
> noarch.

That's not a reason python-based plugins must be arched, too. In case of gedit-plugins arched-pkg is needed because of files have to be (unfortunately) placed inside %{_libdir}/gedit/plugins/

> Gedit take .plugin in /usr/lib64/gedit/plugins and can take other
> file in /usr/share/gedit plugin.

Would not make things better, anyways and will not work for sure.

> Are you sure for the Germán's recommandation about noarch ?

Not really, so I looked around the web and found this page:
https://live.gnome.org/Gedit/PythonPluginHowTo

So Germán was horribly wrong...

There are some issues about the spec-file I want to discuss with you:

-%global commit a9b65a6358c07e41ea835bcd22cf1c99a8df470a
-%global shortcommit %(c=%{commit}; echo ${c:0:7})

Is not needed, see below.

+# gedit-plugins unfortunately need to be arched because they are installed
+# inside %%{_libdir}.  This plugin is written in Python and so cannot
+# provide any debuginfo.
+%global debug_package %{nil}

You wimust switch off generation of debuginfo-pkg in such a case as here (having just noarched stuff in arched-pkg).

+# gedit requires python(abi) = 3.X, so let's bytecompile with the proper
+# python-interpreter providing this.
+%global __python %{__python3}

You need to byte-compile against the same python-abi which gedit uses. Byte-compiling against other abi-version is pointless and has no use.

-Summary:        Gedit plugin who strip trailing whitespace on save
+Summary:        Gedit plugin stripping trailing whitespaces before saving

The summary is written bad english, take my proposal or correct otherwise. Change the bug, too, please.
 
-Group:          Applications/Editors

Group-Tag was obsoleted around F12 somewhen and actually is need for el5, only.

-URL:            http://github.com/jonleighton/gedit-trailsave
+URL:            http://github.com/jonleighton/%{name}

Use macros more frequently, please. In this case they will reduce errors caused through typos.

-Source0:        https://github.com/jonleighton/gedit-trailsave/archive/%{commit}/%{name}-%{version}-%{shortcommit}.tar.gz
+Source0:        https://github.com/jonleighton/%{name}/archive/v%{version}.tar.gz#/%{name}-{version}.tar.gz

same goes here for macros.  Changing the Source-Url from %commit to versioned one will make updates easier, actually.
 
-BuildRoot:      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

deprecated, only needed for el5

-BuildRequires:  python
+BuildRequires:  python3-devel

We actually need python3 here, because of gedit uses python3. Packages Python is involved in, are mandatory to have BR: pythonX-devel by guidelines.
 
-Requires:       gedit >= 3

Every activly maintained version of Fedora ships gedit v3.X, so requiring an explict version is obsolete and frowned upon by guidelines.

-Requires:       python
+# python3 is pulled from gedit dependencies, so no need to install explicitly
+Requires:       gedit%{?_isa}

Proper version of Python is pulled-in by gedit already. Since the plugin ends-up in an arched-dir, the correctly arched-version of gedit is mandatory, here.

 %description
-Gedit plugin who automatically strip all trailing whitespace before saving.
+%{name} is a plugin for gedit which automatically strips all trailing
+whitespaces before saving the document to disk.

%description is written in horrible english, take my proposal or correct otherwise, please. 

 %prep
-%setup -qn %{name}-%{commit}
+%setup -q

There's no need for %commit, anymore. See above...

 %build
+# noop

Comment clearly there's no real build-process needed.

 %install
-rm -rf %{buildroot}

deleting buildroot is obsoleted and only needed for el5.


-install -d %{buildroot}%{_libdir}/gedit/plugins/
+mkdir -p %{buildroot}%{_libdir}/gedit/plugins/%{name}

Plugins go in %{buildroot}%{_libdir}/gedit/plugins/%{name}

-install trailsave.plugin %{buildroot}%{_libdir}/gedit/plugins/
-install trailsave.py %{buildroot}%{_libdir}/gedit/plugins/
+install -pm 0644 trailsave.plugin %{buildroot}%{_libdir}/gedit/plugins/
+install -pm 0644 trailsave.py %{buildroot}%{_libdir}/gedit/plugins/%{name}/

You should preserve timestamps of file being installed.

-%clean
-rm -rf %{buildroot}

Having a %clean-target is only needed for el5.

 %files
-%defattr(-,root,root,-)

Obsoleted, el5 only.

-%{_libdir}/gedit/plugins/trailsave.py
-%{_libdir}/gedit/plugins/trailsave.pyo
-%{_libdir}/gedit/plugins/trailsave.pyc
-%{_libdir}/gedit/plugins/trailsave.plugin
+%{_libdir}/gedit/plugins/*

Using wild-glob is actually shorter and less error prone in case of files beeing added/removed or renamed and will make sure subdir are owned by rpm properly.

 %changelog
-* Mon May 20 2013 Guillaume Kulakowski <guillaume DOT kulakowski AT fedoraproject DOT org> - 3.1.2-1
+* Mon May 20 2013 Guillaume Kulakowski <guillaume.kulakowski@fedoraproject.org> - 3.1.2-1

Don't never-ever cloak your email-adress in spec-file...

#####

Please fix your spec-file, e.g. using attached patch and build/upload new srpm/spec. I'll take full review afterwards.
Comment 12 Germán Racca 2013-06-14 10:04:06 EDT
(In reply to Björn Esser from comment #11)

> Not really, so I looked around the web and found this page:
> https://live.gnome.org/Gedit/PythonPluginHowTo
> 
> So Germán was horribly wrong...

That "horribly" didn't sound really good :(

Anyway, thanks for the info. I'm about to package another gedit plugin and will take advantage of any discussion that happens here :)
Comment 13 Guillaume Kulakowski 2013-06-14 14:11:40 EDT
Hi,

some question :
 - Why modify github Source0 ? I have followed the guideline : http://fedoraproject.org/wiki/Packaging:SourceURL#Github

 - %global __python %{__python3}.
OK, but with condition, because Gedit plugins on F18 use Python2.

 - http://github.com/jonleighton/%{name}
Is compatible with readability guideline ? OK to use variable on Source0, but I think that use variable on url and description harm to readability

> Don't never-ever cloak your email-adress in spec-file...
Why ?
Comment 14 Björn 'besser82' Esser 2013-06-20 14:17:40 EDT
(In reply to Guillaume Kulakowski from comment #13)

Hi Guillaume! Sorry for the little delay. :)

> some question :
>  - Why modify github Source0 ? I have followed the guideline :
> http://fedoraproject.org/wiki/Packaging:SourceURL#Github

It was just meant a proposal, nothing mandatory. Both are fine with the guideline. My proposal makes things easier when it comes to updates; you won't have to look for new commit-sha, just adjust version-tag...
 
>  - %global __python %{__python3}.
> OK, but with condition, because Gedit plugins on F18 use Python2.

OK. Then just add the conditional. I'm fine with that.

>  - http://github.com/jonleighton/%{name}
> Is compatible with readability guideline ? OK to use variable on Source0,
> but I think that use variable on url and description harm to readability

This goes fine with readability guidelines. I have it this way in my packages as well and nobody complained about it. I think having ONE macro in url-tag is ok, since it still makes sense when reading it --> url ends on pkg-name.

> > Don't never-ever cloak your email-adress in spec-file...
> Why ?

Forget about it. Guidelines say it's OK. But on bugzilla your email-address is public viewable either...

Just let me know, when I can start over with new spec/srpm...

Cheers,
  Björn
Comment 15 Michael Schwendt 2013-06-21 13:34:04 EDT
Comment on attachment 761229 [details]
improved spec-file

some good findings in there
Comment 16 Guillaume Kulakowski 2013-06-26 01:50:07 EDT
Hi,

my initial review request was made on F18. With F19 ans Python3, this plugin doesn't work fine. I report bug upstream.
Comment 17 Guillaume Kulakowski 2013-06-26 01:52:03 EDT
Know issue: https://github.com/jonleighton/gedit-trailsave/issues/10
Comment 18 Björn 'besser82' Esser 2013-08-17 07:32:16 EDT
Any news here, Guillaume?
Comment 19 Guillaume Kulakowski 2013-08-21 10:17:17 EDT
The bug is not fixed and the author don't want to update his extension. I think it's a dead project I think we can close this bug ?
Comment 20 Björn 'besser82' Esser 2013-08-21 10:56:02 EDT
I thinks so, too.

cu :)
Comment 21 Stephen 2014-03-01 18:24:15 EST
Per https://github.com/jonleighton/gedit-trailsave/issues/10 this was ported to Python 3 three months ago - any interest in reviving this bug?

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