Bug 1388294 - Review Request: pyflame - Ptracing Profiler For Python
Review Request: pyflame - Ptracing Profiler For Python
Status: ASSIGNED
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Zbigniew Jędrzejewski-Szmek
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2016-10-24 23:59 EDT by Evan Klitzke
Modified: 2017-02-09 03:56 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
zbyszek: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Evan Klitzke 2016-10-24 23:59:28 EDT
I am the upstream author of https://github.com/uber/pyflame and I am trying to submit it as a Fedora package. This is my first package, and I will need a sponsor. I can maintain this package as part of my job at Uber.

Pyflame is a profiler for Python that uses the ptrace system call to monitor a running Python process and collect stack trace data. This data can then be used to generate Flame Graphs for the process.

I was able to build my package in Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=16191804

My spec file: https://eklitzke.org/pyflame.spec
My SRPM: https://eklitzke.org/pyflame-1.1-1.fc25.src.rpm
Comment 1 Parag AN(पराग) 2016-10-25 00:01:22 EDT
Package reviews should be filed against "Package Review" component.
Comment 2 Miro Hrončok 2016-10-25 04:37:16 EDT
While I'm here, I've noticed the commit hash in the spec. What's the purpose?
Comment 3 Evan Klitzke 2016-10-25 14:39:32 EDT
I was just trying to follow the GitHub packaging guidelines. I actually built the SRPM using the tag v1.1, not the commit hash. What's the recommendation here?
Comment 4 Miro Hrončok 2016-10-25 16:23:29 EDT
The GitHub is there just in case there's no tagged release. In this case, just take that commit thing out of there.
Comment 5 Evan Klitzke 2016-10-29 15:20:03 EDT
Thanks, I've removed the commit hash.
Comment 6 Zbigniew Jędrzejewski-Szmek 2016-10-29 16:11:34 EDT
%description should be augmented: what is good for? Why is better than the competition? Etc. The description you gave on the mailing list would be a good start.

New Python packages SHOULD support/use python3. Does pyflame support python3.5? If yes, you should switch to it. (I'm assuming that one version works equally well to analyze both python2 and python3 scripts... Actually it'd be good to mention that in the %description too: "This package supports Python 2 and Python 3" ?)

You must include the license file in the binary package [https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text].

You have tests. They SHOULD be run in %check.

README.md should be packaged as %doc.


I can sponsor you into the packagers group. In addition to the package that you are submitting, I require two-three real reviews of other packages (see http://fedoraproject.org/PackageReviewStatus/NEW.html). There's plenty of python packages awaiting review, so you should be able to find something interesting without any trouble. In your review, please indicate that you are not a packager yet, hence the review is unofficial. If nobody beats you to it, you'll be able to finalize those reviews after you become a packager (hopefully soon ;)). Running fedora-review and carefully looking at the output is a good start.
Comment 7 fszymanski 2016-10-29 18:01:18 EDT
1) Get rid of this: 
%global gittag0 v1.1

2) Change:
Source0: https://github.com/uber/%{name}/archive/v1.1.tar.gz#/%{name}-v%{version}.tar.gz

to

Source0: https://github.com/uber/%{name}/archive/v%{version}.tar.gz#/%{name}-v%{version}.tar.gz

This way you are changing the version in one place.
Note: Move Source0 little higher.

3) ./autogen.sh and %configure should be in the %build section.
Comment 8 Evan Klitzke 2016-10-29 18:59:58 EDT
Hi Zbigniew and Filip,

I made some changes, and I have a few questions.

Updated spec: https://eklitzke.org/pyflame.spec
Updated SRPM: https://eklitzke.org/pyflame-1.1.1-1.fc25.src.rpm

This addresses all of the feedback except the Py3 stuff, which I have a question about.

Question: currently Pyflame requires building against either Python 2, or Python 3. It actually does *not* link against libpython, but it does use the struct definitions from Python.h to know the struct offsets of various Python fields, and right now you can only build against one or the other. I think I might be able to do some stuff upstream to make it possible to build against both simultaneously, but I'm not sure how much work that will be. What do you think the best option is here?

I will start reviewing some other Python packages. Thanks!
Comment 9 Evan Klitzke 2016-10-29 19:03:44 EDT
One more question: is there a way to specify test dependencies separately? The tests for this package require python-pytest to be installed, but it's not *actually* a build dependency.
Comment 10 Zbigniew Jędrzejewski-Szmek 2016-10-29 19:24:35 EDT
(In reply to Evan Klitzke from comment #8)
> Hi Zbigniew and Filip,
> 
> I made some changes, and I have a few questions.
> 
> Updated spec: https://eklitzke.org/pyflame.spec
> Updated SRPM: https://eklitzke.org/pyflame-1.1.1-1.fc25.src.rpm
> 
> This addresses all of the feedback except the Py3 stuff, which I have a
> question about.
> 
> Question: currently Pyflame requires building against either Python 2, or
> Python 3. It actually does *not* link against libpython, but it does use the
> struct definitions from Python.h to know the struct offsets of various
> Python fields, and right now you can only build against one or the other. I
> think I might be able to do some stuff upstream to make it possible to build
> against both simultaneously, but I'm not sure how much work that will be.
> What do you think the best option is here?

Supporting both simultaneously would of course be best. But for now, the solution would be to build two binary packages to support both versions of python.
https://fedoraproject.org/wiki/Packaging:Python#Naming has some explanations, but unfortunately it also contains a lot of noise. Essentially, you want a layout with a bunch of symlinks like this:

[python3-pyflame]
/usr/bin/pyflame → pyflame-3
/usr/bin/pyflame-3 → pyflame-3.5
/usr/bin/pyflame-3.5

[python2-pyflame]
/usr/bin/pyflame-2 → pyflame-2.7
/usr/bin/pyflame-2.7

(E.g. see pytohn2-pytest and python3-pytest packages for an example.)

So your package would BR: python2-devel, BR: python3-devel, and similarly for other build dependencies.

(In reply to Evan Klitzke from comment #9)
> One more question: is there a way to specify test dependencies separately?
> The tests for this package require python-pytest to be installed, but it's
> not *actually* a build dependency.

I don't know. If the tests require it, and the tests get run during build, then it *is* a build dependency. We don't really optimize build dependencies, one or two small packages are not noticeable.
Comment 11 fszymanski 2016-10-30 03:57:27 EDT
Use macros instead of full paths:

%files
%{_mandir}/man1/pyflame.1*

See: https://fedoraproject.org/wiki/Packaging:Guidelines#Manpages
Comment 12 fszymanski 2016-10-30 04:39:24 EDT
Hey guys one more thing. Looking at the runtests.sh file I noticed that pytest is installed to an isolated Python environment from the PyPI using pip. So maybe instead of python2-pytest we should have python-pip in BuildRequires?

BTW I would make runtest.sh executable:

%prep
chmod +x runtest.sh

%check
./runtests.sh
Comment 13 Zbigniew Jędrzejewski-Szmek 2016-10-30 09:35:13 EDT
Ugh. Thanks for noticing those.

runtests.sh is useless for Fedora.
py.test-%{python2_version} or py.test-%{python3_version} should be used directly.
(And of course packages are not allow to download anything during build, so there's no way that pip would work in koji).
Comment 14 Evan Klitzke 2016-11-01 23:45:30 EDT
I made the changes upstream so that Pyflame will support both Python2 and Python3 at once.

I will work on the test changes, so the test suite runs under both Python releases too, as well as the other packaging changes you've suggested.

Thanks again for your time!
Comment 15 Evan Klitzke 2016-11-02 00:55:11 EDT
I have a pull request upstream that changes runtests.sh so that it will work when building the RPM package: https://github.com/uber/pyflame/pull/43/files . In particular, the change makes it so "./runtest.sh fedora" will run the tests in a way that is suitable for the RPM build. I should be able to get this merged tomorrow, and then I can tag v1.2.1 and update the package again.

Feel free to look at the PR and make suggestions.
Comment 17 Zbigniew Jędrzejewski-Szmek 2016-11-02 08:31:05 EDT
The change to support python versions is impressive! I wish everybody would handle requests for python3 support so incredibly fast...

About runtests.sh: I don't think run_tests_fedora() function is worth the trouble. I certainly does the job, so if you want it this way, it's acceptable, but it a) makes things less transparent, b) hard-codes python versions. For Fedora, for the foreseeable future, we'll be building against python2 and python3, but for example for EPEL, we build against python%{python2_version}, python%{python3_version}, which currently are python2.7 and python3.4, but this might change. So if you wanted to build an EPEL branch, you'd have to fight with runtests.sh to specify python versions. Also, it makes hard to make the build for a certain version conditional.

Without further ado:
- latest version
- package name is OK
- license is acceptable (ASL2)
- license is specified correctly
- macros are used where they should
- %check is present and passes
- provides/requires look OK
- no scriptlets present or necessary

Package is APPROVED. (You will not be able to request formal creation of the package until after you are added to the packagers group, i.e. until the other half of comment #c6 is finished.)
Comment 18 Zbigniew Jędrzejewski-Szmek 2016-11-02 08:32:53 EDT
FTR, http://koji.fedoraproject.org/koji/taskinfo?taskID=16278594 (rawhide),  http://koji.fedoraproject.org/koji/taskinfo?taskID=16278601 (F25).

rpmlint only has bogus spelling corrections.
Comment 19 Evan Klitzke 2016-11-02 16:31:43 EDT
Thanks, I will get to work reviewing some other Python packages.

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