Bug 1388294

Package reviews should be filed against "Package Review" component.

While I'm here, I've noticed the commit hash in the spec. What's the purpose?

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?

The GitHub is there just in case there's no tagged release. In this case, just take that commit thing out of there.

Thanks, I've removed the commit hash.

%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.

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.

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!

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.

(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.

Use macros instead of full paths:

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

See: https://fedoraproject.org/wiki/Packaging:Guidelines#Manpages

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

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).

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!

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.

New spec: https://eklitzke.org/pyflame.spec
New SRPM: https://eklitzke.org/pyflame-1.2.1-1.fc25.src.rpm

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.)

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.

Thanks, I will get to work reviewing some other Python packages.

A lot of time has passed, let's redo the review.

Created attachment 1436628 [details]
Spec file for pyflame.

This is the new current spec file I am using, which is in git at https://github.com/eklitzke/pyflame-rpm

OK, this still looks good. rpmlint has only bogus spelling corrections.

Stalled?

(In reply to Miro Hrončok from comment #23)
> Stalled?

Definitely.

The Pyflame upstream project was recently abandoned.

Unfortunately, I can't find an official end-of-life announcement (that would e.g.
hint the reasons behind this move). But the Pyflame repository now redirects to:

https://github.com/uber-archive/pyflame

which reads:

> This project is deprecated and not maintained.

See also this small sub-thread which mentions some alternatives:

https://news.ycombinator.com/item?id=21695050

As per Comment #24 I'll proceed and close this ticket as DEADREVIEW