%description should be wrapped to 72 columns. But please change
the text to something which describes what the package does (history
is not relevant).

Summary should start with a capital letter.

Remove %defattr.

There are some stale comments, remove them too.

No need to run sed in a loop, just pass all the file names to sed at once.
Also, are you sure that you want to encode pypdf version in the file? This package will have to be updated whenever the pypdf package is updated to a new version. Why not remove the pypdf version (s/pypdf == .*/pypdf/) ?

(In reply to Zbigniew Jędrzejewski-Szmek from comment #1)
> %description should be wrapped to 72 columns. But please change
> the text to something which describes what the package does (history
> is not relevant).

I have included a short description but also left a bit of the history as well as the dependencyon PyPDF2. I think this will be helpful for future packagers.

> 
> Summary should start with a capital letter.

done
 
> Remove %defattr.

done
 
> There are some stale comments, remove them too.

I presume that these means the commented-out codes? Done, but for commented out Requires.

> No need to run sed in a loop, just pass all the file names to sed at once.

I am not sure how to do this, please advise if this is a serious issue.

> Also, are you sure that you want to encode pypdf version in the file? This
> package will have to be updated whenever the pypdf package is updated to a
> new version. Why not remove the pypdf version (s/pypdf == .*/pypdf/) ?

pyPdf is dead. The only update will be to PyPDF2. That is also my package which is also under review. So, at this point, this is not particularly an issue?

New source rpms and spec files at:

Spec URL: https://streaming.stat.iastate.edu/~stat580/fedora/pdf-stapler.spec
SRPM URL: https://streaming.stat.iastate.edu/~stat580/fedora/pdf-stapler-0.3.0-1.fc22.src.rpm

(In reply to Globe Trotter from comment #2)
> (In reply to Zbigniew Jędrzejewski-Szmek from comment #1)
> > No need to run sed in a loop, just pass all the file names to sed at once.
> I am not sure how to do this, please advise if this is a serious issue.

for f in setup.py stapler.egg-info/requires.txt ; do
    sed -i 's/pypdf == 1.12/pypdf == .*/g' $f
done

can be changed to

sed -i 's/pypdf == 1.12/pypdf == .*/g' setup.py stapler.egg-info/requires.txt

Nothing serious, just easier to read.
Actually You could just 'rm -rf stapler.egg-info', it would be recreated during installation.

> > Also, are you sure that you want to encode pypdf version in the file? This
> > package will have to be updated whenever the pypdf package is updated to a
> > new version. Why not remove the pypdf version (s/pypdf == .*/pypdf/) ?
> 
> pyPdf is dead. The only update will be to PyPDF2. That is also my package
> which is also under review. So, at this point, this is not particularly an
> issue?
OK.

> Spec URL: https://streaming.stat.iastate.edu/~stat580/fedora/pdf-stapler.spec
> SRPM URL:
> https://streaming.stat.iastate.edu/~stat580/fedora/pdf-stapler-0.3.0-1.fc22.
> src.rpm

Please install the license file using %license [https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text].

Add /usr/lib/python2.7/site-packages/staplelib to %files.

pdf-stapler.noarch: W: incoherent-version-in-changelog tat.maitra ['0.3.0-1.fc23', '0.3.0-1']

Also, change %{__python} to %{__python2} (I'm assuming that python3 is not supported)
[https://fedoraproject.org/wiki/Packaging:Python#Multiple_Python_Runtimes].

Sorry, I am new to this.

> for f in setup.py stapler.egg-info/requires.txt ; do
>     sed -i 's/pypdf == 1.12/pypdf == .*/g' $f
> done
> 
> can be changed to
> 
> sed -i 's/pypdf == 1.12/pypdf == .*/g' setup.py stapler.egg-info/requires.txt
> 
> Nothing serious, just easier to read.
> Actually You could just 'rm -rf stapler.egg-info', it would be recreated
> during installation.

So, instead of the sed, replace with 

rm -rf stapler.egg-info

(I tried this, and it "worked".)

> 
> Please install the license file using %license
> [https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text].

I am sorry but how do I add the file?

> 
> Add /usr/lib/python2.7/site-packages/staplelib to %files.

I did this but I now get warnings, eg:

warning: File listed twice: /usr/lib/python2.7/site-packages/staplelib/__init__.py

....

> 
> pdf-stapler.noarch: W: incoherent-version-in-changelog tat.maitra
> ['0.3.0-1.fc23', '0.3.0-1']

Not sure I understand this point.

> Also, change %{__python} to %{__python2} (I'm assuming that python3 is not
> supported)
> [https://fedoraproject.org/wiki/Packaging:Python#Multiple_Python_Runtimes].

At this point, however, pdf-stapler is moving to python3, in which case I will also move it to python3. (I don't quite see the point of supporting both..)

Thanks again!

(In reply to Globe Trotter from comment #4)
> > Please install the license file using %license
> > [https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text].
> 
> I am sorry but how do I add the file?
> 
> > 
> > Add /usr/lib/python2.7/site-packages/staplelib to %files.
Use something like that for %files:

%files
%{_bindir}/%{name}
/usr/lib/python2.7/site-packages/stapler-%{version}-py2.7.egg-info
%{python2_sitelib}/staplelib
%license LICENSE

(This will include the directory .../staplelib and its contents.)
(It will also install the file LICENSE into %{_pkgdocdir}.)

> I did this but I now get warnings, eg:
> 
> warning: File listed twice:
> /usr/lib/python2.7/site-packages/staplelib/__init__.py
> 
> ....
>
> 
> > 
> > pdf-stapler.noarch: W: incoherent-version-in-changelog tat.maitra
> > ['0.3.0-1.fc23', '0.3.0-1']
> 
> Not sure I understand this point.
You are missing version information in the changelog. The line look should
look something like this:

* Fri May 22 2015 Name Lastname <stat.maitra> - 0.3.0-1

> > Also, change %{__python} to %{__python2} (I'm assuming that python3 is not
> > supported)
> > [https://fedoraproject.org/wiki/Packaging:Python#Multiple_Python_Runtimes].
> 
> At this point, however, pdf-stapler is moving to python3, in which case I
> will also move it to python3. (I don't quite see the point of supporting
> both..)
That's great. The distribution as a whole is trying to move to python3 as much as possible
[https://fedoraproject.org/wiki/Changes/Python_3_as_Default]. So if you can can switch pdf-stapler to use python3 that would be great. But independently of the switch, whatever version is used by the application, it is supposed to be clearly marked. So if it is using python2, then use %{__python2} (which is /usr/bin/python2), otherwise use %{__python3} (which is /usr/bin/python3). This is done in preparation for possibly switching /usr/bin/python to point at python3 instead of python2 at some point.

Fedora Account System (FAS) tells you're not in the packager group yet.

Seems you've missed step 3 from 2.1 https://fedoraproject.org/wiki/Package_Review_Process#Contributor process.

Ooops, I didn't check that. In that case I'm happy to work with you on bringing the package to follow the guidelines, but the final decision about the review will belong to the sponsor.

How does someone block the FE-NEEDSPONSOR tracking bug?

(In reply to Globe Trotter from comment #8)
> How does someone block the FE-NEEDSPONSOR tracking bug?
I already added that. You can see "Blocks: 	FE-NEEDSPONSOR" near the top of the page.

Response to Comment 3:

The author has finally added the License file to upstream.

However, now he has also explicitly required PyPDF2 instead of PyPdf which is also submitted to packaging:

https://bugzilla.redhat.com/show_bug.cgi?id=1262470

How does one proceed to submit a package for review which also depends on another that has also been submitted for review and no one has picked it yet? (The original PyPDF2 was submitted at the same time as this one: https://bugzilla.redhat.com/show_bug.cgi?id=1234208)

Many thanks!

(In reply to Globe Trotter from comment #10)
> Response to Comment 3:
> 
> The author has finally added the License file to upstream.
> 
> However, now he has also explicitly required PyPDF2 instead of PyPdf which
> is also submitted to packaging:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1262470
OK. I'll try to review that.

> How does one proceed to submit a package for review which also depends on
> another that has also been submitted for review and no one has picked it
> yet? (The original PyPDF2 was submitted at the same time as this one:
> https://bugzilla.redhat.com/show_bug.cgi?id=1234208)
By adding the other bug to "Depends On". Both reviews can proceed in parallel, the only caveat is that if this review is finished before the other one, you will not be able to actually build the package officially in koji.

Could you please put the spec file and srpm back? Currently, it's a 404 for me.

Upstream here.

There is a separate setup.py file in the git repository, if the software has to be packaged with the old pyPdf as a dependency. Be aware that it's not as well tested.

If there is anything I can help with, let me know.

(In reply to Matthias Runge from comment #12)
> Could you please put the spec file and srpm back? Currently, it's a 404 for
> me.

New files uploaded here:

SRPM: http://maitra.public.iastate.edu/Fedora/pdf-stapler-0.3.3-1.fc22.src.rpm
SPEC: http://maitra.public.iastate.edu/Fedora/pdf-stapler.spec

Both use python-PyPDF2.

Globe Trotter,

could you please:
- remove code in comments (like in %prep section)
- you should require pyPDF2 like in setup, requiring packages under review is just fine. I added that as depending review.
- there are tests in github repo. it would be great to execute them during package build. It gives you a bit more feedback, if your build works (or not)
- latest version is 0.3.3, please update; I see, you forgot to add a line in changelog
- *I would* separate BuildRequires:  python2-devel,python-setuptools 
into two lines:
BuildRequires:  python2-devel
BuildRequires:  python-setuptools

- P Stark, Globe Trotter, Fedora is moving to be python3 by default. It would be great to provide a python3 (sub) package as well. P Stark, do you support python3 here, too?
- PyPDF2 apparently supports python3
- please remove comments from %files section

Globe Trotter, as a side note, your rhbz account doesn't seem to have anything in common with your FAS account.  The different email account *will* cause problems, you should change one or the other so they are the same.  A real name in both also helps us know who we're working with.

I have a Python 3 porting branch. It's almost done, but it needs some more tests and my time is severely limited currently. I'd say the ETA for Python 3 support is a conservative 2-3 months.

PyPDF2 supports Python 3, but the old pyPdf doesn't. I will be happy to drop support for the old pyPdf completely! :D

Done and new files uploaded at the same site.

> - you should require pyPDF2 like in setup, requiring packages under review
> is just fine. I added that as depending review.

not completely sure how/what to do here. I put this in as a comment, but I thought you did not want that.

> - there are tests in github repo. it would be great to execute them during
> package build. It gives you a bit more feedback, if your build works (or not)

How do I do this? Sorry, I am not a python programmer and this is my first package anyway.

> > - there are tests in github repo. it would be great to execute them during
> > package build. It gives you a bit more feedback, if your build works (or not)
> 
> How do I do this? Sorry, I am not a python programmer and this is my first
> package anyway.

you could add something like:

%check
%{__python} setup.py test

to execute tests.

Be careful: build systems are not connected to the net, thus downloading from pypi doesn't work.

you probably need to sed out requirements:

sed -i 's|"PyPDF2>=1.24"||' setup.py


That has to be done in %prep.

Requirements are handled in rpm anyways, there's no much value in keeping them tied in python.

New files uploaded as per suggested lines. Please check and see if there is anything else to be fixed.

Just to clarify, new files have been uploaded here:

SRPM: http://maitra.public.iastate.edu/Fedora/pdf-stapler-0.3.3-1.fc22.src.rpm
SPEC: http://maitra.public.iastate.edu/Fedora/pdf-stapler.spec

Both use python-PyPDF2.

python-PyPDF2 was approved and imported into repositories

Nobody has reset the fedora-review flag for months. Ticket has not been visible in the needsponsor queue due to that, which is not the only reason for no progress, however.


Btw:

> %{python2_sitelib}/staplelib/*

https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership
https://fedoraproject.org/wiki/Packaging:UnownedDirectories

New files, corrected, have been uploaded here:

SRPM: http://maitra.public.iastate.edu/Fedora/pdf-stapler-0.3.3-1.fc23.src.rpm
SPEC: http://maitra.public.iastate.edu/Fedora/pdf-stapler.spec

Both use python-PyPDF2.

rpmlint only gives some mistaken typographical errors.

Please let me know what additionally is needed to get this through to packaging.

Fedora Account System Username: maitra

maitra's scratch build of pdf-stapler-0.3.3-1.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12027071

(In reply to Ranjan Maitra from comment #25)
> Please let me know what additionally is needed to get this through to
> packaging.

In Fedora new packagers are required to do some extra work to show that they know the packaging rules and various procedures related to packaging. See comment #6.

To get the package into Fedora, you need both things: become a packager, and make the package pass review. This second part is almost done, but to do the first, please see the docs from comment #6. Usually, doing informal reviews of a few packages from http://fedoraproject.org/PackageReviewStatus/NEW.html or http://fedoraproject.org/PackageReviewStatus/REVIEW.html is a welcome step (start with the recent tickets, they are at the bottom in both lists).

maitra's scratch build of pdf-stapler-0.3.3-1.fc23.src.rpm for rawhide completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12027171

New builds uploaded to account for some minor issues:

Documentation now includes README.rst file and PKG-INFO, and License is BSD-style rather than BSD as per upstream.

New files uploaded:

SRPM: http://maitra.public.iastate.edu/Fedora/pdf-stapler-0.3.3-2.fc23.src.rpm
SPEC: http://maitra.public.iastate.edu/Fedora/pdf-stapler.spec

I just checked above spec file link. Here are some suggestions

1) If you are packaging python2 library or tools then your spec file should use %{__python2} macros only. See https://fedoraproject.org/wiki/Packaging:Python#Macros

2) Its always a good practice to increase the release number when you provide updated package here. That will help what has changed since your last package update to new package update.

3) The recent changelog entry looks longer than 80 characters.

4) Good to add comment just above the mv command line in %install that 
# Fedora already have stapler package that is
# why this package is named as pdf-stapler

(In reply to Parag AN(पराग) from comment #31)
> I just checked above spec file link. Here are some suggestions
> 
> 1) If you are packaging python2 library or tools then your spec file should
> use %{__python2} macros only. See
> https://fedoraproject.org/wiki/Packaging:Python#Macros
> 
> 2) Its always a good practice to increase the release number when you
> provide updated package here. That will help what has changed since your
> last package update to new package update.
> 
> 3) The recent changelog entry looks longer than 80 characters.
> 
> 4) Good to add comment just above the mv command line in %install that 
> # Fedora already have stapler package that is
> # why this package is named as pdf-stapler

Done.

New files uploaded:

SRPM: http://maitra.public.iastate.edu/Fedora/pdf-stapler-0.3.3-3.fc23.src.rpm
SPEC: http://maitra.public.iastate.edu/Fedora/pdf-stapler.spec

(

Let's finish the review. It's 95% of the way there ;)

While this review has been in progress, python packaging guidelines have changed (See https://fedoraproject.org/wiki/Packaging:Python#Example_common_spec_file). You should change %build and %install to

%build
%py2_build

%install
py2_install

This should have the exact same effect, but is standard and more concise.


%description is still awkward. If you really want to keep the history part in, at least remove the paragraph about PDFtk. No need to go into detail about an alternative project's downsides.


Please add empty lines between each entry in %changelog.


--


I recently gained the sponsorship privileges and I'd be happy to sponsor you into the packagers group. Please open up a fresh copy of https://fedoraproject.org/wiki/Packaging:Guidelines, fire up fedora-review, and do a two-three reviews from https://fedoraproject.org/PackageReviewStatus/NEW.html, and paste the links here.

(In reply to Zbigniew Jędrzejewski-Szmek from comment #33)
> Let's finish the review. It's 95% of the way there ;)
> 
> While this review has been in progress, python packaging guidelines have
> changed (See
> https://fedoraproject.org/wiki/Packaging:Python#Example_common_spec_file).
> You should change %build and %install to
> 
> %build
> %py2_build
> 
> %install
> py2_install

Thanks, OK with all, but having trouble with this. How do I add in the additional arguments?

I used to have the following in the spec file:

%{__python2} setup.py install -O1 --skip-build --root $RPM_BUILD_ROOT

How do I replace this with %{py2_install} -- sorry, I tried everything I could think of, and the page of guidelines is not completely clear.

Thanks again!

%py2_install already includes all those arguments (try rpmbuild --eval %py2_install), so you use it like in the part you quoted above, literally.

New files posted:


SRPM: http://maitra.public.iastate.edu/Fedora/pdf-stapler-0.3.3-4.fc23.src.rpm
SPEC: http://maitra.public.iastate.edu/Fedora/pdf-stapler.spec

Yep, looks good.

Looking at %description again, the last paragraph is now obsolete, pypdf2 has been packaged.

- license is OK (BSD), license tag matches code
- license file is present, %license is used
- latest version
- package name follows the guidelines
- new python packaging template is used
- builds and install OK
- no scriptlets necessary or present
- provides and requires are OK

rpmlint:
pdf-stapler.noarch: W: spelling-error %description -l en_US opensource -> open source, open-source, outsource
pdf-stapler.noarch: W: spelling-error %description -l en_US commandline -> command line, command-line, commandment
pdf-stapler.noarch: W: spelling-error %description -l en_US pypdf -> PDF
pdf-stapler.noarch: W: spelling-error %description -l en_US refactored -> re factored, re-factored, factored
pdf-stapler.noarch: W: spelling-error %description -l en_US pyPdf -> PDF
pdf-stapler.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/staplelib/stapler.py 644 /usr/bin/env
pdf-stapler.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/staplelib/tests.py 644 /usr/bin/env
pdf-stapler.noarch: W: no-manual-page-for-binary pdf-stapler
pdf-stapler.src: W: spelling-error %description -l en_US opensource -> open source, open-source, outsource
pdf-stapler.src: W: spelling-error %description -l en_US commandline -> command line, command-line, commandment
pdf-stapler.src: W: spelling-error %description -l en_US pypdf -> PDF
pdf-stapler.src: W: spelling-error %description -l en_US refactored -> re factored, re-factored, factored
pdf-stapler.src: W: spelling-error %description -l en_US pyPdf -> PDF
Those can be ignored.

pdf-stapler.src:68: W: macro-in-%changelog %{__python}
pdf-stapler.src:68: W: macro-in-%changelog %{__python_macros}
Please fix those by doubling the macro sign in comments: %%{_python}, etc.
rpm recently warns about this during build and it is very annoying.

The guidelines say that you need to provide python-<modulename> and python2-<modulename>.
See https://fedoraproject.org/wiki/Packaging:Python#Provides. I'm not sure that those are
very meaningful in this case, but it doesn't harm to add:
Provides: python2-staplelib = %{version}-%{release}
%{?python_provide:%python_provide python2-%{srcname}}

Only trivial issues remain, please fix those up when uploading the package.
Package is APPROVED.

We'll also need to finish the sponsorship process, before you are able to upload
the package.

New files posted:


SRPM: http://maitra.public.iastate.edu/Fedora/pdf-stapler-0.3.3-5.fc23.src.rpm
SPEC: http://maitra.public.iastate.edu/Fedora/pdf-stapler.spec

Reviewed: https://bugzilla.redhat.com/show_bug.cgi?id=1291008

Reviewed: https://bugzilla.redhat.com/show_bug.cgi?id=1289760

Reviewing: https://bugzilla.redhat.com/show_bug.cgi?id=1074595

I added a background overlay feature: 
https://github.com/hellerbarde/stapler/pull/23

Ranjan, I'm ready to add you to the packagers group, but I'm not sure about the account details:

17:37 <zbyszek> fasinfo aarem
17:37 <zodbot> User: aarem, Name: Ranjan Maitra, email: itsme_410, Creation: 2012-01-09, 
               IRC Nick: None, Timezone: UTC, Locale: C, GPG key ID: None, Status: active
17:37 <zodbot> Approved Groups: cla_fpca cla_done 
17:37 <zbyszek> fasinfo maitra
17:37 <zodbot> User: maitra, Name: Ranjan Maitra, email: stat.maitra, Creation: 
               2015-06-20, IRC Nick: None, Timezone: UTC, Locale: en, GPG key ID: 038B87FE, 
               Status: active
17:37 <zodbot> Approved Groups: cla_done cla_fpca 

Did you create both? Please note that the e-mail address in bugzilla has to
mount the one in FAS. If both are yours, let me know which one you want to use,
and if the second one, change the e-mail to match bugzilla (or vice versa). Please
terminate the other account (https://fedoraproject.org/wiki/Account_System#Voluntary).

Thanks! Let us use the first account. I was unaware of its existence:-(.

I have deactivated the second account (though I am not 100% sure about it because the instructions were not completely clear to me).

It doesn't seem to have worked, account "maitra" still seems to be active and to contain full name/email information. Please try again and file a ticket if it doesn't work.

I've added you to the packagers group. Welcome!
I'm happy to help with any questions or issues you might have in the future.
Please keep reviewing new packages.

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/pdf-stapler

pdf-stapler-0.3.3-5.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-aa0c4facc9

pdf-stapler-0.3.3-5.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-aa0c4facc9

pdf-stapler-0.3.3-5.fc22 has been pushed to the Fedora 22 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-5ac2c2180a

pdf-stapler-0.3.3-5.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.

pdf-stapler-0.3.3-5.fc22 has been pushed to the Fedora 22 stable repository. If problems still persist, please make note of it in this bug report.