Bug 517743

Summary: Review Request: PyPE - Lightweight but powerful graphical editor for developers
Product: [Fedora] Fedora Reporter: Sandro Mathys <sandro>
Component: Package ReviewAssignee: Thomas Spura <tomspur>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: christoph.wickert, fedora-package-review, josiah.carlson, notting, susi.lehtola, tomspur
Target Milestone: ---Flags: tomspur: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: PyPE-2.9.1-3.fc12 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-05-04 06:18:30 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Sandro Mathys 2009-08-16 14:59:16 UTC
Spec URL: http://red.fedorapeople.org/SRPMS/PyPE.spec
SRPM URL: http://red.fedorapeople.org/SRPMS/PyPE-2.8.8-1.fc11.src.rpm
Description:
PyPE (Python Programmers' Editor) was written in order to offer a
lightweight but powerful editor for those of you who think emacs is too
much and idle is too little. Syntax highlighting is included out of the
box, as is multiple open documents via tabs.

$ rpmlint SPECS/PyPE* SRPMS/PyPE* RPMS/noarch/PyPE*
PyPE.noarch: E: non-executable-script /usr/lib/python2.6/site-packages/PyPE/plugins/parsers.py 0644 /usr/bin/python
2 packages and 1 specfiles checked; 1 errors, 0 warnings

Not sure what to do about this error - should I make the script executable or remove the shebang line? I'm pretty sure that script is not executed stand-alone.

By the way, this is my first python-package and I hope I did everything right :)

Comment 1 Susi Lehtola 2009-08-17 09:54:38 UTC
(In reply to comment #0)
> Spec URL: http://red.fedorapeople.org/SRPMS/PyPE.spec
> SRPM URL: http://red.fedorapeople.org/SRPMS/PyPE-2.8.8-1.fc11.src.rpm
> Description:
> PyPE (Python Programmers' Editor) was written in order to offer a
> lightweight but powerful editor for those of you who think emacs is too
> much and idle is too little. Syntax highlighting is included out of the
> box, as is multiple open documents via tabs.
> 
> $ rpmlint SPECS/PyPE* SRPMS/PyPE* RPMS/noarch/PyPE*
> PyPE.noarch: E: non-executable-script
> /usr/lib/python2.6/site-packages/PyPE/plugins/parsers.py 0644 /usr/bin/python
> 2 packages and 1 specfiles checked; 1 errors, 0 warnings
> 
> Not sure what to do about this error - should I make the script executable or
> remove the shebang line? I'm pretty sure that script is not executed
> stand-alone.

Remove the shebang, Python libraries don't need it.

> By the way, this is my first python-package and I hope I did everything right
> :)  

A few things to correct:

- Use the safer, time stamp keeping versions of the conversion tricks from
 http://fedoraproject.org/wiki/PackagingTricks#Convert_encoding_to_UTF-8
and
 http://fedoraproject.org/wiki/PackagingTricks#Remove_DOS_line_endings

- Don't install manually - use setup.py to do the install. Have a look at the sample python spec
 $ rpmdev-newspec python

- You need to own
 %{python_sitelib}/%{name}/
not
 %{python_sitelib}/%{name}/*

Comment 2 Sandro Mathys 2009-08-17 10:05:03 UTC
Hi Jussi, thanks for the initial review!

(In reply to comment #1)
> Remove the shebang, Python libraries don't need it.

Okay :)

> A few things to correct:
> 
> - Use the safer, time stamp keeping versions of the conversion tricks from
>  http://fedoraproject.org/wiki/PackagingTricks#Convert_encoding_to_UTF-8
> and
>  http://fedoraproject.org/wiki/PackagingTricks#Remove_DOS_line_endings

Hmm...what I'm using is actually what I was told to use in another review, but I'll look into this tricks :)

> - Don't install manually - use setup.py to do the install. Have a look at the
> sample python spec
>  $ rpmdev-newspec python

Well, I did use the python spec-template. The thing is: there is no `setup.py install` that does this thing for you and if you try to do it like that, you only get the information that you should just copy the directory to wherever you want to execute it from.

> - You need to own
>  %{python_sitelib}/%{name}/
> not
>  %{python_sitelib}/%{name}/*

Good catch!

I'll implement those changes ASAP, whenever I've got some minutes :)

Comment 3 Fabian Affolter 2009-08-18 13:24:31 UTC
Don't forget the .desktop file since PyPE is a GUI application.

Comment 4 Sandro Mathys 2009-09-22 07:24:15 UTC
Spec URL: http://red.fedorapeople.org/SRPMS/PyPE.spec
SRPM URL: http://red.fedorapeople.org/SRPMS/PyPE-2.8.8-2.fc11.src.rpm

This took longer than I hoped, sorry. All of the above mentioned fixed (except for the setup.py install thingy as I commented before).

Looking for the formal review.

Comment 5 Thomas Spura 2009-11-15 17:45:59 UTC
Review:

OK
- rpmlint is clean
- .desktop file ok for fedora, for RHEL see issues
- no missing BR
- no locales
- owns all dirctories, it should
- no duplicate files
- permissions ok
- %clean ok
- constantly macros
- nothing in %doc for runtime
- no subpackages needed
- latest version packaged
- sources match upstream
  both f286464ad703c3cceec2331a01d88971


Issues:
- .desktop file needs Encoding=UTF-8 if you want to ship this into RHEL.
  (at least desktop-file-validate fails without this, desktop-file-install
  probably too) Just for fedora, this is not needed.

- install: the icons are in the wrong place. When starting pype searchs in
  the python_site_packages_dir. Please install them into:
  /usr/lib/python2.6/site-packages/PyPE/icons/ and adjust the desktop file or
  place a link into the other icons directory.

- License GPLv2 and LGPLv2 and wxWidgets is partly wrong, partly unknown:
  At least plugins/exparse.py is LGPLv2+. The other files (I checked for now)
  contained no license header so you don't know, if it should be (L)GPLv2 ONLY
  or v2+. Please query upstream to add license headers and ask them, if v2 only
  of v2+.

- %doc: changelog is missing

Comment 6 Christoph Wickert 2009-11-15 19:47:39 UTC
(In reply to comment #5)

> Issues:
> - .desktop file needs Encoding=UTF-8 if you want to ship this into RHEL.
>   (at least desktop-file-validate fails without this, desktop-file-install
>   probably too) Just for fedora, this is not needed.

And should not be there in fact. An easy way to achieve this is to have the Encoding in the desktop file and remove it if it's installed in Fedora:

desktop-file-install --dir=$RPM_BUILD_ROOT%{_datadir}/applications \
  %if 0%{?fedora} >= 10
  --remove-key=Encoding \
  %endif
  %{SOURCE1}

BTW: /usr/share/ is hardcoded in the desktop file, you should at least use sed to make sure it is always correct. And please preserve timestamps during cp.

Comment 7 Sandro Mathys 2009-12-09 13:57:03 UTC
Spec URL: http://red.fedorapeople.org/SRPMS/PyPE.spec
SRPM URL: http://red.fedorapeople.org/SRPMS/PyPE-2.8.8-3.fc12.src.rpm

Thanks for the comments so far guys. The new package/spec fixes all that except for the licensing issue. Will contact upstream about it.

Comment 8 Josiah Carlson 2009-12-09 23:28:48 UTC
I've updated the license for all files currently in the subversion repository at http://pype.svn.sourceforge.net/viewvc/pype/ .  If you would prefer to pull from subversion, it wouldn't bother me.  Otherwise, you can wait a little longer for me to cut a release.

Thank you.

Comment 9 Thomas Spura 2009-12-23 15:08:54 UTC
Sandro, I don't mind accepting this now with the old version, if it's clear, which file has which license.

You can choose, between
- adding a comment above the License: field
- refer to a file upstream, where anything is explained
- breack down in the %files section.


See [1] for further information.

[1] https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios

Comment 10 Josiah Carlson 2009-12-23 16:39:14 UTC
FYI, I released a new version with all of the license and code updates yesterday.  :)

Comment 11 Sandro Mathys 2009-12-23 16:44:28 UTC
Just wanted to post the same thing as Josiah - there's a new release with the fixes and I'm going to create a new srpm with it. Might very well get 2010 until I'm able to do so, tho. But maybe I'll find some time to do it earlier...

Comment 12 Sandro Mathys 2009-12-26 16:40:27 UTC
Spec URL: http://red.fedorapeople.org/SRPMS/PyPE.spec
SRPM URL: http://red.fedorapeople.org/SRPMS/PyPE-2.9-1.fc12.src.rpm

Only small changes to the spec file:
- new version
- msvcp90.dll is now removed in %prep
- pype.py needs CRLF -> LF treatment with the new release

Everything else is still the same as in my last version above.

Comment 13 Thomas Spura 2009-12-27 02:18:28 UTC
(In reply to comment #9)
> You can choose, between
> - adding a comment above the License: field
> - refer to a file upstream, where anything is explained
> - breack down in the %files section.
> 
> 
> See [1] for further information.
> 
> [1]
> https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios  

This is a MUST. ;)
If you have multiple licenses, you have to e.g. refer to in this case readme.txt or say in a comment, which files have which license.

wxWidgets is GPL compatible (see https://fedoraproject.org/wiki/Licensing ), so anything under wxWidgets can be relicensed under GPL. BUT in fact, Josiah has already done that:
from readme.txt:
'''
The portions of STCStyleEditor.py included in StyleSetter.py, which is used to

support styles, was released under the wxWindows license [...]
'''

But the whole file itself is GPL -> no wxWidgets everywhere.


What is copyrighted under LGPL?
There is a lgpl.txt, but that's it. Or am I wrong?

So, it stays for me: GPLv2.

@ Josiah: Correct me, if I'm wrong ;)


########################################

Some other issues:
- $RPM_BUILD_ROOT%{_datadir}/%{name} is created, but empty (rest of old icon path) -> not needed anymore.
- sed -i -e "s;PYTHONPATH;%{python_sitelib};g" %{SOURCE1}
  has no effect. There is no pythonpath in the .desktop ( yet? ;) )
- Don't copy *.pyc and *pyw.
  They are created later in the /usr/lib/rpm/brp-python-bytecompile part anyway.
  See https://fedoraproject.org/wiki/Packaging/Guidelines#No_inclusion_of_pre-built_binaries_or_libraries
  MUST: don't install them, and Josiah please don't include them (same for *.dll);)
- changelog format:
  There is '* new version' -> '- new version'

Comment 14 Josiah Carlson 2009-12-27 04:26:56 UTC
The portion that is lgpl licensed is parts of plugins/exparse.py , which describes which parts of itself are lgpl v2 licensed, and includes a link to the original version of the files w/the original license.  :)

Comment 15 Sandro Mathys 2009-12-27 14:33:56 UTC
Spec URL: http://red.fedorapeople.org/SRPMS/PyPE.spec
SRPM URL: http://red.fedorapeople.org/SRPMS/PyPE-2.9-1.fc12.src.rpm

(In reply to comment #13)
> This is a MUST. ;)
> If you have multiple licenses, you have to e.g. refer to in this case
> readme.txt or say in a comment, which files have which license.

Oh, I thought that only applies for the old version where licensing was not clear. But now that I actually read the link it's clear :) Wonder why I never saw this before, I pretty sure have other multi-license packages :/

FIXED (comment above License)

> anything under wxWidgets can be relicensed under GPL. BUT in fact, Josiah has
> already done that
> But the whole file itself is GPL -> no wxWidgets everywhere.

Right, after reading through the license comments again, seems to be sane.

> What is copyrighted under LGPL?
> There is a lgpl.txt, but that's it. Or am I wrong?

Josiah already answered that :) And so did you in comment #5 btw :)

> So, it stays for me: GPLv2.

So it's GPLv2 and LGPLv2. FIXED

> ########################################
> 
> Some other issues:
> - $RPM_BUILD_ROOT%{_datadir}/%{name} is created, but empty (rest of old icon
> path) -> not needed anymore.

Hm...right. FIXED

> - sed -i -e "s;PYTHONPATH;%{python_sitelib};g" %{SOURCE1}
>   has no effect. There is no pythonpath in the .desktop ( yet? ;) )

Uh, good catch! Grabbed the wrong desktop file in the end :/ FIXED

> - Don't copy *.pyc and *pyw.
>   They are created later in the /usr/lib/rpm/brp-python-bytecompile part
> anyway.
>   See
> https://fedoraproject.org/wiki/Packaging/Guidelines#No_inclusion_of_pre-built_binaries_or_libraries
>   MUST: don't install them, and Josiah please don't include them (same for
> *.dll);)

FIXED. Not upstream tho.

> - changelog format:
>   There is '* new version' -> '- new version'  

Awww, right. FIXED

Comment 16 Thomas Spura 2009-12-27 17:16:39 UTC
You use 'sed -i'. Everytime you change the .desktop to contain PYTHONPATH, you destroy it, when you run rpmbuild again.
(And the included .desktop does not contain PYTHONPATH again ;) )

If you upload the corrected one manually (with cvs add... and not with ./common/cvs-import.sh) this will be ok in cvs...

###############
LICENSE

I'm still not sure, if the license is correct.

Josiah copied the LGPLv2 file and made some changes under GPLv2.
("[...] which
describes which parts of itself are lgpl v2 licensed. [...]") and in the file is "adapted from", which means there where changes...
LGPLv2 allows relicensing under GPLv2 (see https://fedoraproject.org/wiki/Licensing#GPL_Compatibility_Matrix ).

-> Anything GPLv2

But I'm no layer.

If Josiah would agree to this, it'll be ok. If not, I'd like the legal team to take a look.


###############y

Anything else fine in the spec.

Comment 17 Josiah Carlson 2009-12-29 06:33:52 UTC
I've updated the license for exparse.py .  It is now GPL v2 licensed.  Originally I included lgpl.txt because it is referenced as part of wxwindows.txt (in addition for exparse.py), and is required by the wxWindows license.

I have relicensed the wxWindows and LGPL v2 stuff as GPL v2 as is allowed by both the wxWindows and LGPL v2 licenses.

I also added a line to get rid of the MS dll for the source distribution.

Because of the non-code changes, I bumped the version number to 2.9.1, and uploaded a source-only release.  sf.net is being painfully slow, so I've not been able to remove the old file quite yet, but the new one is available as of now.

Comment 18 Sandro Mathys 2009-12-29 07:01:36 UTC
That's great Josiah, thanks!

I try to get this into a new pkg ASAP, but I'll get a guest until new year which might result in some inactivity until afterwards. I'll try to it sooner tho.

Comment 19 Sandro Mathys 2009-12-29 08:01:51 UTC
Spec URL: http://red.fedorapeople.org/SRPMS/PyPE.spec
SRPM URL: http://red.fedorapeople.org/SRPMS/PyPE-2.9.1-1.fc12.src.rpm

Guests are late, thus I had the time to pkg the new sources.

Changes:
- new src zip
- changed License to GPLv2 only
- no longer try to remove non-existant *.dll
- defined %global majorminor (only used in Version and Source0)
- worked around sed -i problem for the .desktop file (is that a good way?)

i.e. ALL FIXED

Comment 20 Thomas Spura 2009-12-29 15:03:43 UTC
(In reply to comment #19)
> i.e. ALL FIXED  

Close to finish ;)

You make the pype.py executable and after CRLF -> LF it's *not* executable again.
Make it executable at the end of %prep and then, it'll work as expected.

Unfortunately pype does *not* start anyway...
Don't know why atm......

When executing, there is a File not found. This could be a dos newline, but this is shot down at CRLF -> LF...

Anything else fine now.
(.desktop file is ok this way.)


Josiah, thanks for the quick release.

Comment 21 Thomas Spura 2010-01-14 16:02:42 UTC
If you use:
sed -i -e '/^#!\//, 1d' pype.py
sed -i -e '1d;2i#!/usr/bin/python' pype.py

instead of:
sed -i -e 's|#!/usr/bin/env python|#!/usr/bin/python|g' pype.py

It works.

So there is a dos newline at the end, which is not replaced with the sed command...


--> With dos2unix pype.py, it *SHOULD* work.

But it doesn't, whyever!!

You could use the first 2 sed commands as a fix for now, but where is this problem coming from....?

Comment 22 Sandro Mathys 2010-03-05 14:36:48 UTC
Spec URL: http://red.fedorapeople.org/SRPMS/PyPE.spec
SRPM URL: http://red.fedorapeople.org/SRPMS/PyPE-2.9.1-2.fc12.src.rpm

There seems to be some unprintable chars before #!/usr/bin/env in pype.py, therefore I now do:
sed -i -e 's|^.*#!/usr/bin/env python|#!/usr/bin/python|' pype.py

Everything seems to be sound and working now.

Comment 23 Thomas Spura 2010-04-16 22:39:15 UTC
(In reply to comment #22)
> Spec URL: http://red.fedorapeople.org/SRPMS/PyPE.spec
> SRPM URL: http://red.fedorapeople.org/SRPMS/PyPE-2.9.1-2.fc12.src.rpm
> 
> There seems to be some unprintable chars before #!/usr/bin/env in pype.py,
> therefore I now do:
> sed -i -e 's|^.*#!/usr/bin/env python|#!/usr/bin/python|' pype.py
> 
> Everything seems to be sound and working now.    

A new rpmlint is available since the last run:

$ rpmlint ./PyPE-2.9.1-2.fc13.src.rpm noarch/PyPE-2.9.1-2.fc13.noarch.rpm 
PyPE.src: W: spelling-error %description -l en_US emacs -> Emacs, macs, maces
PyPE.noarch: W: spelling-error %description -l en_US emacs -> Emacs, macs, maces
2 packages and 0 specfiles checked; 0 errors, 2 warnings.

So you should change it to Emacs.

The sed looks a bit hacky, but it seems to be unavoidable...

-> +1

Comment 24 Sandro Mathys 2010-04-17 13:11:57 UTC
Spec URL: http://red.fedorapeople.org/SRPMS/PyPE.spec
SRPM URL: http://red.fedorapeople.org/SRPMS/PyPE-2.9.1-3.fc12.src.rpm

Emacs it is.

Can we get this approved now?

Comment 25 Thomas Spura 2010-04-17 18:36:02 UTC
(In reply to comment #24)
> Spec URL: http://red.fedorapeople.org/SRPMS/PyPE.spec
> SRPM URL: http://red.fedorapeople.org/SRPMS/PyPE-2.9.1-3.fc12.src.rpm
> 
> Emacs it is.
> 
> Can we get this approved now?    

Was it late, when you wrote that? ;-)

Hmm, I don't reed explicit 'APPROVED' above, but I already set the review flag a while ago'


############################################################################

-> 'APPROVED'

Comment 26 Sandro Mathys 2010-04-17 19:28:56 UTC
Ah, I always look for the 'APPROVED' first and only then for the flag...worked well until now ;)

New Package CVS Request
=======================
Package Name: PyPE
Short Description: Lightweight but powerful graphical editor for developers
Owners: red
Branches: F-12 F-13
InitialCC:

Comment 27 Kevin Fenzi 2010-04-18 01:34:54 UTC
CVS done (by process-cvs-requests.py).

Comment 28 Fedora Update System 2010-04-18 13:20:35 UTC
PyPE-2.9.1-3.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/PyPE-2.9.1-3.fc12

Comment 29 Fedora Update System 2010-04-18 13:21:24 UTC
PyPE-2.9.1-3.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/PyPE-2.9.1-3.fc13

Comment 30 Fedora Update System 2010-04-20 13:02:34 UTC
PyPE-2.9.1-3.fc12 has been pushed to the Fedora 12 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update PyPE'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/PyPE-2.9.1-3.fc12

Comment 31 Fedora Update System 2010-04-20 13:35:31 UTC
PyPE-2.9.1-3.fc13 has been pushed to the Fedora 13 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update PyPE'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/PyPE-2.9.1-3.fc13

Comment 32 Fedora Update System 2010-05-04 06:18:23 UTC
PyPE-2.9.1-3.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 33 Fedora Update System 2010-05-04 06:22:47 UTC
PyPE-2.9.1-3.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.