Spec URL: https://raw.githubusercontent.com/thebeanogamer/hstsparser/master/hstsparser.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/thebeanogamer/hstsparser/fedora-rawhide-x86_64/05970871-hstsparser/hstsparser-1.1.8-3.fc39.src.rpm Description: Parse Firefox and Chrome HSTS databases into Digital Forensics artifacts Fedora Account System Username: thebeanogamer This is a command line tool for which I am the lead maintainer, and it allows you to parse the HSTS databases from Chromium and Firefox into useful digital forensics artifacts for web browser activity (accompanying research is on https://beano.dev/hsts). When running fedora-review on the SRPM locally, the only issue it highlighted was that source MD5 hashes had not been provided. This is because the RPM is coming from the same repo as the source code, and in a proper dist-git repo I would obviously fix this. Whilst I have built RPMs for proprietary tools as part of my job, this is the first RPM I've submitted to Fedora, so it will require sponsorship and likely some changes. I'm more than happy to work on other parts of the Fedora ecosystem to earn that sponsorship if needed. This RPM is currently being built by both COPR (https://copr.fedorainfracloud.org/coprs/thebeanogamer/hstsparser/build/5970871/) and GitHub Actions (https://github.com/thebeanogamer/hstsparser/actions/runs/5104896388/jobs/9176119972).
Congratulations on your first package submission! Here are some comments. General comments ----------------------- Upstream's pyproject.toml has ``` [build-system] requires = ["poetry>=1.3"] build-backend = "poetry.masonry.api" ``` I'd use poetry-core as the build backend: ``` [build-system] requires = ["poetry-core>=1.3"] build-backend = "poetry.core.masonry.api" ``` ---- The maximum version pins in ``` [tool.poetry.dependencies] python = ">=3.9,<3.12" prettytable = ">=0.7.2,<3.8" ``` are also a problem for Fedora integration. You should relax them unless you're absolutely sure a certain version won't work. We only have one version of everything, so packages cannot be so picky or the package will become uninstallable when a dependency is updated or block the dependency update all together. Pinning Python versions are especially problematic; the Fedora Python maintainers are already preparing for the Python 3.12 rebuild. Specfile comments --------------------------- > Group: Applications/Engineering The Group tag should not be used in Fedora. ---- > BuildRequires: python3dist(poetry) > BuildRequires: python3dist(tox-current-env) Please remove these. %generate_buildrequires automatically handles these BuildRequires. ---- You should evenly space specfile sections to make the file more readable. There's two newlines between %prep and %generate_buildrequires, so there should also be two newlines between %build and %install and %install and %check and so on. Two newlines between specfile sections is a general convention. ---- You run `%pyproject_buildrequires -t` and `%tox`, but you don't actually have any tox envs configured in tox.ini. You should either properly configure tox to run your unit tests or entirely remove those invocations. ---- ``` # Fix command name in manpage ln -s hstsparser.py hstsparser # This runs outside the venv, so --version will return the default help2man -N ./hstsparser -o %{buildroot}%{_mandir}/man1/hstsparser.1 --version-string='%{version}' rm -f hstsparser ``` Building of manpages should happen in %build not %install. You can output the file to ./hstsparser.1 in the current directory during %build and then copy that file to %{buildroot}%{_mandir}/man1 during %install. Also, I'm not sure what ``` # This runs outside the venv, so --version will return the default ``` means. Fedora RPM builds do not use venvs. Maxwell's Python Review Checklist ----------------------- Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated - [x] The License tag reflects the package contents and uses the correct identifiers. - [x] The license text is included and marked with %license. - [x] The package builds successfully in mock. - [x] The package is installable (checked by fedora-review). - [x] There are no relevant rpmlint errors. - [x] The package runs tests in %check. The package runs %pyproject_check_import and %tox. Running only %pyproject_check_import is enough to satisfy this guideline, but %tox shouldn't be run if it's NOOP. See the note above. - [x] The latest version is packaged or packaging an earlier version is justified. - [-] Libraries: The package name has a `python3-` prefix and uses the canonical project name - [x] Applications: The package follows the general Naming Guidelines for applications - [x] The pyproject macros are used. - [x] There are no bundled libraries.
Hey Maxwell, Thank you so much for all your feedback. I really appreciate all of it, and hopefully I've just pushed a release which fixes all your concerns: Updated SRPM: https://download.copr.fedorainfracloud.org/results/thebeanogamer/hstsparser/fedora-rawhide-x86_64/06000907-hstsparser/hstsparser-1.1.8-4.fc39.src.rpm * poetry -> poetry-core This was a holdover from when I was working on this project prior to Poetry 1.1 splitting that out. I've switched it all over to poetry-core now. * Version pin for Python This is because pyinstaller (which is used for building the Windows release) has a max Python version set. I've moved that into an environment marker, so the Python version for anything non-Windows is now unconstrained. Is there a way for me to test building against Python 3.12 yet? * Version pin for PrettyTable I think I added this whilst fighting Poetry. Every distro seems to have stuck on prettytable 0.7.2 for a long time, then Fedora 39 is using 3.6.0. I've removed this and it still builds on 38 and 39 so I think it's all fine. * RPM group, tox, & manual BuildRequires I've removed these * Man page building As requested, I've moved the build to %build and now just install the file in %install. * Comment about venv Sorry, re-reading this I understand the confusion. I've updated the comment in the spec file, but to give a bit more detail: hstsparser is using `importlib.metadata` to find the value for `--version` when `help2man` asks for it. However as `hstsparser` is not actually installed in the buildroot, this fails to find and gives a default value. To work around this, I'm manually telling `help2man` what the version is. * formatting I've spent some time tidying this up. Hopefully that covers all your concerns, but if there's anything else please let me know and I'd be delighted to fix it. Thanks again.
You need to post the URLs like this so the fedora-review tool can find them :) Spec URL: https://download.copr.fedorainfracloud.org/results/thebeanogamer/hstsparser/fedora-rawhide-x86_64/06000907-hstsparser/hstsparser.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/thebeanogamer/hstsparser/fedora-rawhide-x86_64/06000907-hstsparser/hstsparser-1.1.8-4.fc39.src.rpm
Thanks for applying my feedback, Daniel! I replied inline: > * poetry -> poetry-core > This was a holdover from when I was working on this project prior to Poetry > 1.1 splitting that out. I've switched it all over to poetry-core now. > * Version pin for Python > This is because pyinstaller (which is used for building the Windows release) > has a max Python version set. I've moved that into an environment marker, so > the Python version for anything non-Windows is now unconstrained. Yeah, using a environment marker makes more sense here. It's too bad that pyinstaller does that. > Is there a way for me to test building against Python 3.12 yet? See the README in https://copr.fedorainfracloud.org/coprs/g/python/python3.12/ about how to preform rpm builds in mock against the Python 3.12 test Copr. If you just want to install the python3.12 beta and develop within a venv, you can install the `python3.12` package from the standard Fedora repositories. > * Version pin for PrettyTable > I think I added this whilst fighting Poetry. Every distro seems to have > stuck on prettytable 0.7.2 for a long time, then Fedora 39 is using 3.6.0. > I've removed this and it still builds on 38 and 39 so I think it's all fine. I see those three changes in the Github repo and they look good. You'll probably want to release a new version including those before you preform the final Fedora import. > * RPM group, tox, & manual BuildRequires > I've removed these Ack. > * Man page building > As requested, I've moved the build to %build and now just install the file > in %install. That looks better now, but you should change ``` install -m 0555 hstsparser.1 %{buildroot}%{_mandir}/man1/hstsparser.1 ``` to ``` install -pm 0644 hstsparser.1 %{buildroot}%{_mandir}/man1/hstsparser.1 ``` See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_timestamps about -p. As for the mode, the file should be writable by root and not executable. This is reflected in the rpmlint output as ``` hstsparser.noarch: W: spurious-executable-perm /usr/share/man/man1/hstsparser.1.gz hstsparser.noarch: E: non-standard-executable-perm /usr/share/man/man1/hstsparser.1.gz 555 2 packages and 0 specfiles checked; 1 errors, 1 warnings, 1 badness; has taken 0.1 s ``` > * Comment about venv > Sorry, re-reading this I understand the confusion. I've updated the comment > in the spec file, but to give a bit more detail: hstsparser is using > `importlib.metadata` to find the value for `--version` when `help2man` asks > for it. However as `hstsparser` is not actually installed in the buildroot, > this fails to find and gives a default value. To work around this, I'm > manually telling `help2man` what the version is. Yeah, I deduced as much after reading the code. The new comment makes much more sense. > * formatting > I've spent some time tidying this up. Looks good to me. > Hopefully that covers all your concerns, but if there's anything else please > let me know and I'd be delighted to fix it. > > Thanks again. Sure! One other thing: ``` %description %{expand: Parse Firefox and Chrome HSTS databases into Digital Forensics artifacts} ``` should be written as ``` %description Parse Firefox and Chrome HSTS databases into Digital Forensics artifacts. ``` There's no need to use %{expand:...} here, and package descriptions end with a period by convention.
Also, don't forget to post a self introduction on the devel list as explained in https://docs.fedoraproject.org/en-US/package-maintainers/Joining_the_Package_Maintainers/#introduce_yourself.
Hey, I've made all the changes you requested (and fixed the fact that my CI wasn't rpmlint-ing properly). Also tested building in the Python 3.12 mock environment, all seemed to build fine. Here's the files for the new release with all the changes in: Updated SRPM: https://download.copr.fedorainfracloud.org/results/thebeanogamer/hstsparser/fedora-rawhide-x86_64/06000981-hstsparser/hstsparser-1.1.9-1.fc39.src.rpm Updated SPEC: https://download.copr.fedorainfracloud.org/results/thebeanogamer/hstsparser/fedora-rawhide-x86_64/06000981-hstsparser/hstsparser.spec As requested, introduction is now on devel (https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/thread/N53EQAGX6FKUYACXAG3TCAIZ6UR6U6SZ/). Please let me know if there's anything else. Thanks.
Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated - [x] The License tag reflects the package contents and uses the correct identifiers. - [x] The license text is included and marked with %license. - [x] The package builds successfully in mock. - [x] The package is installable (checked by fedora-review). - [x] There are no relevant rpmlint errors. - [-] The package runs tests in %check. NOTE: Upstream does not provide tests, but the specfile runs %pyproject_check_import which fulfills this requirement. - [x] The latest version is packaged or packaging an earlier version is justified. - [-] Libraries: The package name has a `python3-` prefix and uses the canonical project name - [x] Applications: The package follows the general Naming Guidelines for applications - [x] The pyproject macros are used. - [x] There are no bundled libraries. - [x] The package complies with the Python and general Packaging Guidelines. Package approved! The recently deployed package review bot should open a ticket for you in the pagure.io/packager-sponsors repository for the next steps.
Hello @thebeanogamer, since this is your first Fedora package, you need to get sponsored by a package sponsor before it can be accepted. A sponsor is an experienced package maintainer who will guide you through the processes that you will follow and the tools that you will use as a future maintainer. A sponsor will also be there to answer your questions related to packaging. You can find all active sponsors here: https://docs.pagure.org/fedora-sponsors/ I created a sponsorship request for you: https://pagure.io/packager-sponsors/issue/572 Please take a look and make sure the information is correct. Thank you, and best of luck on your packaging journey. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service
The Pagure repository was created at https://src.fedoraproject.org/rpms/hstsparser
FEDORA-2023-05a1ff0fe9 has been submitted as an update to Fedora 39. https://bodhi.fedoraproject.org/updates/FEDORA-2023-05a1ff0fe9
FEDORA-2023-05a1ff0fe9 has been pushed to the Fedora 39 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-2023-26a96147c4 has been submitted as an update to Fedora 38. https://bodhi.fedoraproject.org/updates/FEDORA-2023-26a96147c4
FEDORA-2023-26a96147c4 has been pushed to the Fedora 38 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2023-26a96147c4 \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2023-26a96147c4 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2023-26a96147c4 has been pushed to the Fedora 38 stable repository. If problem still persists, please make note of it in this bug report.