Bug 982352 - Review Request: python-jsonpatch - Applying JSON Patches in Python
Review Request: python-jsonpatch - Applying JSON Patches in Python
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Christopher Meng
Fedora Extras Quality Assurance
:
Depends On: 982351
Blocks: 997850
  Show dependency treegraph
 
Reported: 2013-07-08 14:45 EDT by Dan Prince
Modified: 2014-01-23 15:14 EST (History)
6 users (show)

See Also:
Fixed In Version: python-jsonpatch-1.2-2.fc20
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
: 1019266 (view as bug list)
Environment:
Last Closed: 2013-10-28 23:45:05 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
i: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Dan Prince 2013-07-08 14:45:42 EDT
Spec URL: http://dprince.fedorapeople.org/python-jsonpatch.spec
SRPM URL: http://dprince.fedorapeople.org/python-jsonpatch-0.12-1.fc18.src.rpm
Description: Applying JSON Patches in Python
Fedora Account System Username: dprince

NOTE: This will require python-jsonpointer here: https://bugzilla.redhat.com/show_bug.cgi?id=982351
Comment 1 Pádraig Brady 2013-07-11 06:59:25 EDT
I see v1.1 is available. 0.12 is from 12 months ago
Also needs to s/python-devel/python2-devel/
Comment 2 Christopher Meng 2013-07-13 22:46:35 EDT
1. I can found that in your 2 requests these pkgs are not the latest one.

Please package the latest version.

2. Remove %defattr(-,root,root,-).

3. changelog is invalid IMO, please include your email address.

4. As Pádraig said, please modify the BuildRequires:  python-devel

5. IMO, please package the latest version of python-jsonpointer, and then you don't have to specify ">= 0.6".
Comment 3 Alan Pevec 2013-08-16 16:03:07 EDT
Dan is ok that I take this package, so here are new spec and SRPM, using github tarball:

Spec URL: http://apevec.fedorapeople.org/python-jsonpatch.spec
SRPM URL: http://apevec.fedorapeople.org/python-jsonpatch-1.1-1.fc20.src.rpm
Description: Applying JSON Patches in Python
Fedora Account System Username: apevec
Comment 4 Christopher Meng 2013-09-11 21:39:44 EDT
Reviewing now.
Comment 5 Christopher Meng 2013-09-11 21:56:05 EDT
Executing(%check): /bin/sh -e /var/tmp/rpm-tmp.MbdcHe
+ umask 022
+ cd /builddir/build/BUILD
+ cd python-json-patch-2572aa4d2f5f19b7943a6ea81828559afafc9eed
+ /usr/bin/python tests.py
Traceback (most recent call last):
  File "tests.py", line 8, in <module>
    import jsonpatch
  File "/builddir/build/BUILD/python-json-patch-2572aa4d2f5f19b7943a6ea81828559afafc9eed/jsonpatch.py", line 48, in <module>
    import jsonpointer
ImportError: No module named jsonpointer
RPM build errors:
error: Bad exit status from /var/tmp/rpm-tmp.MbdcHe (%check)
    Bad exit status from /var/tmp/rpm-tmp.MbdcHe (%check)
Child return code was: 1
EXCEPTION: Command failed. See logs for output.
 # ['bash', '--login', '-c', 'rpmbuild -bb --target i686 --nodeps builddir/build/SPECS/python-jsonpatch.spec']
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/mockbuild/trace_decorator.py", line 70, in trace
    result = func(*args, **kw)
  File "/usr/lib/python2.7/site-packages/mockbuild/util.py", line 361, in do
    raise mockbuild.exception.Error, ("Command failed. See logs for output.\n # %s" % (command,), child.returncode)
Error: Command failed. See logs for output.
 # ['bash', '--login', '-c', 'rpmbuild -bb --target i686 --nodeps builddir/build/SPECS/python-jsonpatch.spec']
LEAVE do --> EXCEPTION RAISED


Seems it has problem with tests.

===========
BuildRequires:  python2-devel

missing 

BuildRequires:  python-setuptools
===========
Also just got news from FPC, we should use python2 entirely now.

So all %{__python} should be %{__python2} and %{python_sitelib} should be %{python2_sitelib}
Comment 6 Christopher Meng 2013-09-11 21:56:44 EDT
BTW you should put %check after %install.
Comment 7 Alan Pevec 2013-09-12 10:06:19 EDT
> BTW you should put %check after %install.

Yeah, had that discussion in bug 982351 comment 7
IMHO rpmbuild is wrong and should execute %check afer %build and before %install since tests are executed in builddir.
Comment 8 Christopher Meng 2013-09-12 10:10:36 EDT
(In reply to Alan Pevec from comment #7)
> > BTW you should put %check after %install.
> 
> Yeah, had that discussion in bug 982351 comment 7
> IMHO rpmbuild is wrong and should execute %check afer %build and before
> %install since tests are executed in builddir.

%check section mostly represent "check your pkg to see if it can work well after installation" IMO. If you want to put some tests before install to check(I guess in most cases are used for environ sanity check?), then just write them down in %install.

0.02 cents~


---

Anyway, fix the package and upload a new one, I will run review again.
Comment 9 Michael Schwendt 2013-09-12 10:34:14 EDT
Re: comment 7 (Alan Pevec)

I still disagree (bug 982351 comment 8), but my post to packaging list hasn't received helpful responses yet:
https://lists.fedoraproject.org/pipermail/packaging/2013-September/009491.html
Comment 10 Alan Pevec 2013-09-12 17:45:27 EDT
Re: comment 9

ok, your howto edits make sense, but I'd really like to see an example where %check is testing files in buildrootdir.


Re: comment 5

> Seems it has problem with tests.

Yes, BR on jsonpointer is missing, also that dependency was built only today, I'll update this review tomorrow when jsonpointer hits the Rawhide.

> So all %{__python} should be %{__python2} and %{python_sitelib} should be %{python2_sitelib}

I don't see python2_* on el6 so this needs to be conditional, right?
Comment 11 Christopher Meng 2013-09-12 19:04:17 EDT
(In reply to Alan Pevec from comment #10)
> I don't see python2_* on el6 so this needs to be conditional, right?

Yes of course...
Comment 12 Michael Schwendt 2013-09-13 05:05:45 EDT
> but I'd really like to see an example where %check is testing files
> in buildrootdir.

Test-suites that expect libraries in run-time linker's search path are common. They would not link and run without setting LD_LIBRARY_PATH. And pointing to various local $(pwd)/src/libfoo/.libs builddirs is not really an option.

Or here's a custom test from "compface":

  %check
  export LD_LIBRARY_PATH=$RPM_BUILD_ROOT%{_libdir}:$LD_LIBRARY_PATH
  ./compface %{SOURCE1} | ./uncompface -X > __test.xbm
  cmp %{SOURCE1} __test.xbm

There are also large packages that include many files via '*' wildcards, and in %check you can add file-exists tests for specific files. For example, to verify that important plugins/odules have been built and installed actually if the configure script would disable them without error.

[...]

If there's a good reason not to use %check, perform tests in %build instead. Please don't compile any additional files during %install.
Comment 13 Alan Pevec 2013-09-13 11:52:38 EDT
http://fedoraproject.org/wiki/Packaging:Python is not updated yet to include python2_* macros so I haven't included them, rest of the feedback included:

Spec URL: http://apevec.fedorapeople.org/python-jsonpatch.spec
SRPM URL: http://apevec.fedorapeople.org/python-jsonpatch-1.1-2.fc21.src.rpm
Description: Applying JSON Patches in Python
Fedora Account System Username: apevec
Comment 14 Alan Pevec 2013-09-13 11:53:32 EDT
Scratch Rawhide build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5931599
Comment 15 Alan Pevec 2013-09-13 11:55:28 EDT
Re: comment 12

Thanks for the nice writeup Michael, would be good to put it on fedora wiki!
Comment 16 Christopher Meng 2013-09-23 20:36:51 EDT
1. Homepage should be "https://python-json-patch.readthedocs.org/"

2. Use pypi can reduce size of the spec, you don't need to add commit rev number.

https://pypi.python.org/pypi/jsonpatch

However it's NOT a must, just my suggestion.

3. If you want to push it to el6, you can preserve %{__python}.

------------

PACKAGE APPROVED.
Comment 17 Alan Pevec 2013-09-25 18:18:56 EDT
New Package SCM Request
=======================
Package Name: python-jsonpatch
Short Description: Applying JSON Patches in Python
Owners: apevec dprince
Branches: f19 f20 el6
InitialCC:
Comment 18 Alan Pevec 2013-09-25 18:55:47 EDT
(In reply to Christopher Meng from comment #16)
> 1. Homepage should be "https://python-json-patch.readthedocs.org/"

URL:            https://github.com/stefankoegl/python-json-patch
was taken from egg metadata, so I assume that's what upstream prefers as a homepage?

> 2. Use pypi can reduce size of the spec, you don't need to add commit rev
> number.

I'd like to but see bug 982351 comment 3, license file is missing in the pypi tarball.
I've opened upstream issue about it https://github.com/stefankoegl/python-json-patch/issues/16

 
> 3. If you want to push it to el6, you can preserve %{__python}.

yes, I want el6

> PACKAGE APPROVED.

Thanks!
Comment 19 Christopher Meng 2013-09-25 19:01:20 EDT
(In reply to Alan Pevec from comment #18)
> (In reply to Christopher Meng from comment #16)
> > 1. Homepage should be "https://python-json-patch.readthedocs.org/"
> 
> URL:            https://github.com/stefankoegl/python-json-patch
> was taken from egg metadata, so I assume that's what upstream prefers as a
> homepage?

To me there is no determine line of which one is the closest...

For packagers, using github url will be helpful when they want to check out the new version info.

For devs, using readthedocs will help them find the doc easily.

But I tink you are right, not too many people will have a look on spec and find docs from URL tag. And if upstream create a separate project homepage it would be better to use, whereas currently no such websites available.

So using gihub url as primary URL is right.
Comment 20 Kevin Fenzi 2013-09-27 14:34:32 EDT
Git done (by process-git-requests).
Comment 21 Fedora Update System 2013-10-14 08:20:57 EDT
python-jsonpatch-1.2-1.fc19,python-warlock-1.0.1-1.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/python-jsonpatch-1.2-1.fc19,python-warlock-1.0.1-1.fc19
Comment 22 Fedora Update System 2013-10-14 13:08:51 EDT
python-jsonpatch-1.2-1.fc20,python-warlock-1.0.1-1.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/python-jsonpatch-1.2-1.fc20,python-warlock-1.0.1-1.fc20
Comment 23 Fedora Update System 2013-10-14 15:20:03 EDT
Package python-jsonpatch-1.2-1.fc20, python-warlock-1.0.1-1.fc20:
* should fix your issue,
* was pushed to the Fedora 20 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing python-jsonpatch-1.2-1.fc20 python-warlock-1.0.1-1.fc20'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2013-19078/python-jsonpatch-1.2-1.fc20,python-warlock-1.0.1-1.fc20
then log in and leave karma (feedback).
Comment 24 Fedora Update System 2013-10-28 23:45:05 EDT
python-warlock-1.0.1-1.fc19, python-jsonpatch-1.2-2.fc19 has been pushed to the Fedora 19 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 25 Fedora Update System 2013-11-10 01:14:50 EST
python-warlock-1.0.1-1.fc20, python-jsonpatch-1.2-2.fc20 has been pushed to the Fedora 20 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 26 Fedora Update System 2014-01-23 06:11:45 EST
python-jsonpatch-1.2-2.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/python-jsonpatch-1.2-2.el6
Comment 27 Fedora Update System 2014-01-23 15:14:27 EST
python-jsonpatch-1.2-2.el6 has been pushed to the Fedora EPEL 6 stable repository.  If problems still persist, please make note of it in this bug report.

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