https://bugzilla.redhat.com/show_bug.cgi?id=2210561 --- Comment #4 from Maxwell G <maxwell@xxxxxxx> --- 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. -- You are receiving this mail because: You are always notified about changes to this product and component You are on the CC list for the bug. https://bugzilla.redhat.com/show_bug.cgi?id=2210561 _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
