Bug 844721 - Review request: python-django-flash - A Django extension to provide support for Rails-like flash
Summary: Review request: python-django-flash - A Django extension to provide support f...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Linux
unspecified
unspecified
Target Milestone: ---
Assignee: Bohuslav "Slavek" Kabrda
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 839880
TreeView+ depends on / blocked
 
Reported: 2012-07-31 13:42 UTC by Luis Bazan
Modified: 2013-07-03 11:42 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-07-03 11:42:54 UTC
Type: Bug
Embargoed:
bkabrda: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Luis Bazan 2012-07-31 13:42:45 UTC
Hi ready to review

http://lbazan.fedorapeople.org/python-django-flash-1.7.2-2.fc17.src.rpm

http://lbazan.fedorapeople.org/python-django-flash.spec

$ rpmlint -v python-django-flash.spec
python-django-flash.spec: I: checking-url http://pypi.python.org/packages/source/d/django-flash/django-flash-1.7.2.tar.gz (timeout 10 seconds)
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

Comment 1 Thomas Moschny 2012-07-31 13:51:54 UTC
Not related to pytest.

Comment 2 Bohuslav "Slavek" Kabrda 2012-08-02 08:32:51 UTC
I'll take this for a review.

Comment 3 Bohuslav "Slavek" Kabrda 2012-08-02 08:45:07 UTC
- There is no need for defining the %python_sitelib, if you only target F18 (which is the target for all the django to python-django renames).
- The upstream archive contains some unit tests, you should run them in the %check section.
- Consider leaving the old changelog of django-flash in place.
- There is no need to specify the %defattr in %files; also, you don't need to do "rm -rf $RPM_BUILD_ROOT"

Comment 5 Bohuslav "Slavek" Kabrda 2012-08-03 06:35:31 UTC
Hi Luis,
there is still one thing that I would like to see in the specfile: running the unittests. It is as simple as running (in the %check section)

%{__python} setup.py test

It seems however that some of the tests fail with new Django (1.4 and above). Could you please investigate that and fix the package/propose changes to upstream?
(BTW I'm going on vacation starting tomorrow, so I hope you won't mind waiting a week before going on with this review...)

Comment 8 Matthias Runge 2012-08-09 06:19:02 UTC
some additions:

- BuildRequirements should read python2-devel and
python-setuptools

- BuildRequires Django resp. python-django. Do you do something there? I can't see, build requires this. You'll probably require django for build time, when running provided checks, but you don't do that.

- requires: python-django-setuptest: I can't see, where setuptest is required in python-django-flash (at least not at runtime), did I miss something?

Comment 9 Luis Bazan 2012-08-09 19:19:55 UTC
Hi

I edit BuildRequirements

python-devel to python2-devel
python-setuptools-devel to python-setuptools

remove Require is not necesary python-django-setuptest

New SPEC version 1.8-4
http://lbazan.fedorapeople.org/python-django-flash.spec

SRPM:
http://lbazan.fedorapeople.org/python-django-flash-1.8-4.fc17.src.rpm

Regards.

Comment 10 Bohuslav "Slavek" Kabrda 2012-08-13 08:23:05 UTC
Luis, please create the %check section and run the tests there, please. I consider this to be a blocker, when tests are available and we don't run them.
Thanks.

Comment 11 Bohuslav "Slavek" Kabrda 2012-08-27 08:30:07 UTC
Ping, any progress here?

Comment 12 Luis Bazan 2012-08-27 23:05:15 UTC
I have a issue in %check because the test have a BUG and the developer no answer my mails :(

let me create a patch to try fix the problem

Best Regards!

Comment 13 Matthias Runge 2012-11-23 11:36:01 UTC
Another ping, what's the status here?

Comment 14 Matthias Runge 2012-12-14 11:56:21 UTC
And another ping, please continue.

Comment 15 Luis Bazan 2013-03-22 20:36:06 UTC
I create a patch tomorrow Upload....

Regards!

Comment 17 Bohuslav "Slavek" Kabrda 2013-03-26 15:56:21 UTC
Hmm, weird. Mockbuild gives me

DEBUG: Patch #0 (python-django-flash-settings.patch):
DEBUG: + /usr/bin/cat /builddir/build/SOURCES/python-django-flash-settings.patch
DEBUG: + /usr/bin/patch -p1 --fuzz=0
DEBUG: can't find file to patch at input line 3
DEBUG: Perhaps you used the wrong -p or --strip option?
DEBUG: The text leading up to this was:
DEBUG: --------------------------
DEBUG: |--- settings.py.orig	2013-03-25 10:31:41.756017345 -0500
DEBUG: |+++ settings.py	2013-03-25 10:32:00.787016418 -0500
DEBUG: --------------------------

Are you sure you posted link to the correct RPM?

Comment 19 Matthias Runge 2013-03-26 22:18:53 UTC
Some smaller drive by comments:

Projects homepage is dead, are you sure the project is still alive? 

Also are you sure, the code is compatible with Django-1.5? (We have Django-1.5 in Rawhide!)

- no buildroot definition required any more
- please use the python macro instead of calling python: %{__python}
- remove the clean section
- defattr in files section not required any more

Comment 20 Matthias Runge 2013-03-26 22:19:35 UTC
Homepage: Probably it's better to use http://danielfm.github.com/django-flash/

Comment 21 Bohuslav "Slavek" Kabrda 2013-03-27 07:33:03 UTC
- As for the old RPM stuff, I think it's ok if Luis wants to also use this spec on epels (that's for buildroot, clean and defattr).
- Django 1.5 compatibility seems to be fine. The spec is running test suite which passes in current Rawhide with Django 1.5.
- "python" calls should really be replaced with "%{__python}"
- I totally agree with using the homepage that Matthias has suggested.
- I appreciate that you sent the pull request with you fix. Could you please put a comment with the pull request url into the specfile? It's good to keep track of these things (so that you can easily see if it was accepted when updating to newer version later).

These are really just cosmetic issues. In general, the package is good and once you fix these, it can be approved.

Comment 22 Luis Bazan 2013-03-27 13:26:16 UTC
- As for the old RPM stuff, I think it's ok if Luis wants to also use this spec on epels (that's for buildroot, clean and defattr).

--> Yes, I'm going to use in epels

- add python macro %{__python}
- add new homepage of the project

NEWSPEC:
http://lbazan.fedorapeople.org/python-django-flash.spec
PATCH:
http://lbazan.fedorapeople.org/python-django-flash-settings.patch
NEWSRPM:
http://lbazan.fedorapeople.org/python-django-flash-1.8-4.fc17.src.rpm

Best Regards!

Comment 23 Luis Bazan 2013-03-27 13:28:01 UTC
I sent the pull request with the fix 

LINK:
https://github.com/danielfm/django-flash/pull/11

Regards!

Comment 24 Bohuslav "Slavek" Kabrda 2013-03-27 13:54:42 UTC
(In reply to comment #23)
> I sent the pull request with the fix 
> 
> LINK:
> https://github.com/danielfm/django-flash/pull/11
> 
> Regards!

What I meant was putting a reference to that pull request inside the specfile, so that it is documented there for whoever reads the spec - but this is just a minor nit, otherwise the package looks good now. (Please don't forget to retire django-flash properly). Thanks :)


APPROVED

Comment 25 Luis Bazan 2013-03-27 15:47:58 UTC
New Package SCM Request
=======================
Package Name: python-django-flash
Short Description: A Django extension to provide support for Rails-like flash
Owners: lbazan
Branches: f17 f18 f19 el6

Comment 26 Gwyn Ciesla 2013-03-27 15:54:37 UTC
Git done (by process-git-requests).

Comment 27 Fedora Update System 2013-03-27 17:18:17 UTC
python-django-flash-1.8-4.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/python-django-flash-1.8-4.fc18

Comment 28 Fedora Update System 2013-03-29 01:32:05 UTC
python-django-flash-1.8-4.fc18 has been pushed to the Fedora 18 testing repository.

Comment 29 Fedora Update System 2013-04-07 00:31:30 UTC
python-django-flash-1.8-4.fc18 has been pushed to the Fedora 18 stable repository.

Comment 30 Michael Schwendt 2013-06-20 11:17:43 UTC
https://fedoraproject.org/wiki/Package_Renaming_Process

| [...] The reviewer of the package MUST [...] check the package
| for the proper Obsoletes and Provides (see the naming guidelines
| for more information.) They MUST document in the review request
| that they have done so. 

http://koji.fedoraproject.org/koji/rpminfo?rpmID=3919899

Obsoletes 	django-flash < 1.7.2-6

=> python-django-flash-1.8-5.fc20.src.rpm
=> python-django-flash-1.8-5.fc20.noarch in fedora-development-i386
  File conflict with: django-flash-1.7.2-6.fc18.noarch

The Obsoletes < 1.7.2-6 is not high enough, because of the .fc18 dist tag.

Comment 31 Luis Bazan 2013-06-21 19:13:40 UTC
https://fedorahosted.org/rel-eng/ticket/5645#comment:1

Now is blocked

I can close this BZ?

Regards!

Comment 32 Michael Schwendt 2013-06-21 19:44:02 UTC
Not yet. Please read comment 30.

Comment 33 Luis Bazan 2013-06-22 17:44:08 UTC
Hi Michael 

I review and solve the problem....

Regards!

Comment 34 Luis Bazan 2013-06-26 16:23:04 UTC
No more conflicts!

I can close this BZ?

Regards!

Comment 35 Michael Schwendt 2013-06-26 18:15:52 UTC
No, you need to increase the version-release in the Obsoletes tag (your %{obs_ver} value to be higher than the last build for Fedora 18), submit a build and updates for the branches where to rename this package.

Comment 36 Michael Schwendt 2013-06-28 21:31:56 UTC
Luis, do you need help? Do you think a longer [more detailed] explanation would be helpful?

In git, you've removed the Obsoletes tag, which is not the right thing to do:
http://pkgs.fedoraproject.org/cgit/python-django-flash.git/commit/?id=55812e2bf42a8e0c09f4793e5a5cfd27334e4f57

Don't forget the upgrade path from Fedora 18. Only with proper Obsoletes tags, the old package will be renamed and replaced.

[...]

django-flash-1.7.2-6.fc18
http://koji.fedoraproject.org/koji/buildinfo?buildID=332780

python-django-flash-1.8-4.fc18.noarch.rpm
http://koji.fedoraproject.org/koji/rpminfo?rpmID=3866674
Obsoletes django-flash < 1.7.2-6

Not high enough either! 6.fc18 is higher than 6
A new update for Fedora 18 is needed, too.

[...]

https://fedoraproject.org/wiki/Package_Renaming_Process
https://fedoraproject.org/wiki/Packaging:Guidelines#Renaming.2FReplacing_Existing_Packages

Comment 37 Michael Schwendt 2013-07-03 11:42:54 UTC
I've committed the necessary fixes to git master, f19 and f18 and will submit bodhi updates, too, once the builds are done. That would have been even easier, if in the f19 branch a simple "git merge master" had been possible, but the branches differed just in a few changelog entries/typos. For f18, the spec file contains many more conflicts with f19, so a simple merge has not been possible.


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