Bug 1954478 - Review Request: python3-proton-client - Proton API Python Client
Summary: Review Request: python3-proton-client - Proton API Python Client
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2021-04-28 08:42 UTC by Alexandru Cheltuitor
Modified: 2021-12-10 11:13 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2021-12-10 11:13:06 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Alexandru Cheltuitor 2021-04-28 08:42:39 UTC
Spec URL: https://keybase.pub/calexandru/proton%20fedora/python3-proton-client.spec
SRPM URL: https://keybase.pub/calexandru/proton%20fedora/python3-proton-client-0.4.1-1.src.rpm
Description: First Proton package. This package will be used mainly by our products but it could be used anyone else to reach our API in a secure and authenticated way.
Fedora Account System Username: acrandom

Comment 1 Ben Beasley 2021-05-02 20:55:27 UTC
This is not a review, but just some preliminary notes.

-----

Please read https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/ to understand Python packaging practices in Fedora. It looks like you may not have done so. Please also see https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_sections.

You can use the fedora-review command-line tool and consult https://docs.fedoraproject.org/en-US/packaging-guidelines/ReviewGuidelines/ to see some of the things people will be checking for when reviewing your package.

-----

> %define version 0.4.1
> %define release 1

> Version: %{version}
> Release: %{release}

Do not do this. Do:

> Version: 0.4.1
> Release: 1%{?dist}

Note that the version and release macros are defined for you this way. See https://docs.fedoraproject.org/en-US/packaging-guidelines/DistTag/.

-----

> Prefix: %{_prefix}

Remove this. See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_relocatable_packages.

-----

> Name: python3-proton-client

This violates https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_naming.

The base package should be python-%{unmangled_name}, and you should have

> %package -n python3-%{unmangled_name}

> %files -n python3-%{unmangled_name}

as the built package.

Note that this means you will have to submit a new review ticket with the correct name.

-----

> Requires: python3-requests
> Requires: python3-pyOpenSSL
> Requires: python3-bcrypt
> Requires: python3-gnupg

> %{?python_disable_dependency_generator}

You are *allowed* to opt out of automatic dependency generation (https://github.com/ProtonMail/proton-python-client). But since the upstream setup.py provides the necessary metadata, why would you want to?

-----

> Group: ProtonVPN

> Vendor: Proton Technologies AG <contact>

> BuildRoot: %{_tmppath}/%{unmangled_name}-%{version}-%{release}-buildroot

> %clean
> rm -rf $RPM_BUILD_ROOT

> %defattr(-,root,root)

All of these either MUST NOT or SHOULD NOT be present (https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_sections, https://docs.fedoraproject.org/en-US/packaging-guidelines/#_file_permissions).

-----

> %setup -q -n %{unmangled_name}-%{version} -n %{unmangled_name}-%{version}

should be

> %setup -q -n %{unmangled_name}-%{version}

-----

> %build
> %{python3} setup.py build

> %install
> %{python3} setup.py install --single-version-externally-managed -O1 --root=$RPM_BUILD_ROOT --record=INSTALLED_FILES

Why not just this? When you don’t use the standard macros, it’s hard to tell at a glance whether you’re trying to do something unusual.

> %build
> %py3_build

> %install
> %py3_install

-----

The upstream repository has tests, so you should run them (at least, any tests that do not require network access).

Comment 2 Ben Beasley 2021-05-02 21:03:12 UTC
It also looks like you submitted an email address rather than a Fedora Account System username. An FAS username is required for a successful review to be processed.

Comment 3 Ben Beasley 2021-11-29 14:33:56 UTC
Please consider either preparing an updated submission that more closely follows Fedora packaging guidelines, or closing this as NOTABUG.

Since I set NEEDINFO, this issue should be automatically closed if there is no response in one month plus one week (https://docs.fedoraproject.org/en-US/fesco/Package_review_policy/#submitter_not_responding).

Comment 4 Alexandru Cheltuitor 2021-12-07 12:56:52 UTC
Hey @code thanks for the heads up and for the links to the documentation, it helped a lot in understanding what I did wrong. I've addressed most of your comments though I still have some questions if you don't mind answering. 

> %files -n python3-%{unmangled_name}

1. So does this mean that the line would look like this %files -f INSTALLED_FILES -n python3-%{unmangled_name} or %files just -n python3-%{unmangled_name} ?

2. "It also looks like you submitted an email address rather than a Fedora Account System username. An FAS username is required for a successful review to be processed.", I did create this after logging in with FAS, am I supposed to do anything different ?

3. "Note that this means you will have to submit a new review ticket with the correct name." So this basically means that I will have to close this one and create a new one, correct ?

Comment 5 Ben Beasley 2021-12-08 15:12:58 UTC
> 1. So does this mean that the line would look like this %files -f INSTALLED_FILES -n python3-%{unmangled_name} or %files just -n python3-%{unmangled_name} ?

Yes, just: %files -n python3-%{unmangled_name}

Although you can choose (for now) whether to package under the “new” Python guidelines (https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/) using pyproject-rpm-macros, or the “old” ones (https://docs.fedoraproject.org/en-US/packaging-guidelines/Python_201x/). If you use the new guidelines, which I would recommend, you’ll end up with: %files -n python3-%{unmangled_name} -f %{pyproject_files}

> 2. "It also looks like you submitted an email address rather than a Fedora Account System username. An FAS username is required for a successful review to be processed.", I did create this after logging in with FAS, am I supposed to do anything different ?

I’m referring to the text in the description for this issue: “Fedora Account System Username: acrandom”

That needs to be your FAS username, not an email. For example, I’m https://accounts.fedoraproject.org/user/music/, so I would put “music” there.

> 3. "Note that this means you will have to submit a new review ticket with the correct name." So this basically means that I will have to close this one and create a new one, correct ?

I think so. It might be possible to change the issue title to contain the correct package name (“python-proton-client”), and put the correct FAS username in a “Fedora Account System Username: WHOMEVER” line in a new comment. You would learn if that worked after the package was approved and you tried to request a dist-git repository. Worst case, you would have to close this bug and re-open a new one then, and ask your reviewer to re-approve using the new issue. That happens sometimes.

Still, if it were me, I would go ahead and close this issue and create a new one instead of trying to fix this one.

Comment 6 Ben Beasley 2021-12-08 15:15:33 UTC
I think your FAS username is calexandru2018.

Comment 7 Alexandru Cheltuitor 2021-12-10 11:13:06 UTC
Will create a new package with a new name and all major fixes to the .spec file.


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