[Bug 965007] Review Request: gedit-trailsave - Gedit plugin who strip trailing whitespace on save

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=965007

--- Comment #11 from Björn Esser <bjoern.esser@xxxxxxxxx> ---
Created attachment 761229
  --> https://bugzilla.redhat.com/attachment.cgi?id=761229&action=edit
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@xxxxxxxxxxxxxxxxx> - 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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=NfleGBfzNS&a=cc_unsubscribe
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review





[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]