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.
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.
(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/
> - 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!
I'll review during this week...
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.
> 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
(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
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.
I'm glad you guys are following my stereotypes :)
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 ?
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> - 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.
(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 :)
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 ?
(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 on attachment 761229 [details] improved spec-file some good findings in there
Hi, my initial review request was made on F18. With F19 ans Python3, this plugin doesn't work fine. I report bug upstream.
Know issue: https://github.com/jonleighton/gedit-trailsave/issues/10
Any news here, Guillaume?
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 ?
I thinks so, too. cu :)
Per https://github.com/jonleighton/gedit-trailsave/issues/10 this was ported to Python 3 three months ago - any interest in reviving this bug?