Bug 844721
Summary: | Review request: python-django-flash - A Django extension to provide support for Rails-like flash | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Luis Bazan <bazanluis20> |
Component: | Package Review | Assignee: | Bohuslav "Slavek" Kabrda <bkabrda> |
Status: | CLOSED RAWHIDE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | unspecified | Docs Contact: | |
Priority: | unspecified | ||
Version: | rawhide | CC: | bkabrda, bugs.michael, mrunge, notting, package-review |
Target Milestone: | --- | Flags: | bkabrda:
fedora-review+
gwync: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | Unspecified | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2013-07-03 11:42:54 UTC | Type: | Bug |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 839880 |
Description
Luis Bazan
2012-07-31 13:42:45 UTC
Not related to pytest. I'll take this for a review. - 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" Hi I fix all SPEC:http://lbazan.fedorapeople.org/python-django-flash.spec SRPM:http://lbazan.fedorapeople.org/python-django-flash-1.8-2.fc17.src.rpm Regards! 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...) Hi SPEC: http://lbazan.fedorapeople.org/python-django-flash.spec SRPM: http://lbazan.fedorapeople.org/python-django-flash-1.8-3.fc17.src.rpm have a new corrections. Regards 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? 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. 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. Ping, any progress here? 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! Another ping, what's the status here? And another ping, please continue. I create a patch tomorrow Upload.... Regards! SPEC http://lbazan.fedorapeople.org/python-django-flash.spec SRPM: http://lbazan.fedorapeople.org/python-django-flash-1.8-2.fc17.src.rpm PACTH: http://lbazan.fedorapeople.org/python-django-flash-settings.patch Best Regards! 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? NEW SPEC: http://lbazan.fedorapeople.org/python-django-flash.spec NEW SRPMS: http://lbazan.fedorapeople.org/python-django-flash-1.8-3.fc17.src.rpm NEW PATCH: http://lbazan.fedorapeople.org/python-django-flash-settings.patch Regards! 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 Homepage: Probably it's better to use http://danielfm.github.com/django-flash/ - 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. - 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! I sent the pull request with the fix LINK: https://github.com/danielfm/django-flash/pull/11 Regards! (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 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 Git done (by process-git-requests). 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 python-django-flash-1.8-4.fc18 has been pushed to the Fedora 18 testing repository. python-django-flash-1.8-4.fc18 has been pushed to the Fedora 18 stable repository. 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. https://fedorahosted.org/rel-eng/ticket/5645#comment:1 Now is blocked I can close this BZ? Regards! Not yet. Please read comment 30. Hi Michael I review and solve the problem.... Regards! No more conflicts! I can close this BZ? Regards! 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. 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 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. |