Bug 2106138 - Review Request: pinnwand - Straightforward pastebin software
Summary: Review Request: pinnwand - Straightforward pastebin software
Keywords:
Status: CLOSED DUPLICATE of bug 2229857
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: 2022-07-11 20:21 UTC by Simon de Vlieger
Modified: 2023-08-08 03:27 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2023-08-08 03:27:20 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Simon de Vlieger 2022-07-11 20:21:31 UTC
Spec URL: https://supakeen.fedorapeople.org/for-review/pinnwand.spec
SRPM URL: https://supakeen.fedorapeople.org/for-review/pinnwand-1.3.2-1.fc37.src.rpm
Description: pinnwand is Python pastebin software that tried to keep it simple but got a little more complex
Fedora Account System Username: supakeen

Comment 1 Simon de Vlieger 2022-07-11 20:22:55 UTC
I might need a bit more handholding here, my previous packages were libraries and this one has an executable (service). I've put the service files and example configs in /usr/share so users can create their own and run the executable as they see fit (e.g. copy /usr/share/pinnwand/pinnwand.toml-example somewhere and then run `pinnwand -c path-to-config`).

Would it be normal to add these service files automatically and a default configuration file?

Comment 2 Lyes Saadi 2022-07-26 02:09:25 UTC
Hello!

I'm not really comfortable yet with the new Python packaging guidelines, so not sure about reviewing this, but:

1. Use macros (%{_bindir} / %{_datadir}) for paths:
```
mkdir -p %{buildroot}/usr/share/pinnwand
cp -a pinnwand.service-example /%{buildroot}/usr/share/pinnwand/
cp -a pinnwand.toml-example /%{buildroot}/usr/share/pinnwand/
```
to
```
mkdir -p %{buildroot}%{_datadir}/pinnwand
cp -a pinnwand.service-example %{buildroot}%{_datadir}/pinnwand/
cp -a pinnwand.toml-example %{buildroot}%{_datadir}/pinnwand/
```
(also, removed the unnecessary / before the buildroot)

and

```
%files -f %{pyproject_files}
/usr/bin/pinnwand
/usr/share/pinnwand/pinnwand.service-example
/usr/share/pinnwand/pinnwand.toml-example
```
to
```
%files -f %{pyproject_files}
%{_bindir}/pinnwand
%{_datadir}/pinnwand/pinnwand.service-example
%{_datadir}/pinnwand/pinnwand.toml-example
```

2. Remove the -p1 in %autosetup (did you forget it there?)

3. The examples in share is "fine". I mean, choosing where to place them is usually the job of upstream, but, since, well, you are upstream, that's your choice. Usually, we'd put them with the documentation (with %doc) if upstream decided not to install it themselves, but, putting it in /usr/share makes sense looking at what other upstreams do.

Comment 3 Jonathan Wright 2022-08-12 21:11:20 UTC
Are you still working on this?  If not I'd be happy to package it.

Comment 4 Simon de Vlieger 2022-08-13 19:20:22 UTC
I am still working on this and plan to fix the reviews comments this week!

Comment 5 Felix Kaechele 2023-06-05 03:09:50 UTC
For the Source URL, since this is hosted on GitHub check out this: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_tag_example

As for the examples:
For the packages I've been maintaining I default to installing the configuration examples into the proper configuration location (i.e. /etc aka %{_sysconfdir}). Ideally the software is able to run without any user intervention using some reasonable defaults with that configuration.

In this package you include both a config and a systemd unit as examples.
Technically you should include them at their proper location such that the user can immediately launch the service after they installed it.
See https://docs.fedoraproject.org/en-US/packaging-guidelines/Systemd/#unit_files
In your case this would mean correctly configuring the path to the executable in the systemd unit:

ExecStart=/home/youruser/virtual-environment/bin/pinnwand --configuration-path /home/youruser/pinnwand.toml http

would become something like:

ExecStart=/usr/bin/pinnwand --configuration-path /etc/pinnwand/pinnwand.toml http

and then installing the configuration to %{_sysconfdir}/pinnwand as well as putting it into the files section as

%dir %{_sysconfdir}/%{name}
%config(noreplace) %{_sysconfdir}/%{name}/%{name}.toml

Comment 7 Neil Hanlon 2023-08-04 14:19:43 UTC
Spec URL: https://neil.fedorapeople.org/for-review/pinnwand.spec
SRPM URL: https://neil.fedorapeople.org/for-review/pinnwand-1.4.0-1.fc39.src.rpm

Bumped back to 1.4.0 due to dependencies on docutils/sqlalchemy not-yet-in Fedora.

Addressed comments from above reviewers

Comment 8 Felix Kaechele 2023-08-05 01:47:49 UTC
I'm a bit confused as to who is submitting this package for review at this point? Or are neil and supakeen the same person?

Other than that, here are a few more comments, going through the spec file top to bottom:

1. For the "Release" field use %autorelease: https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_release_tag

2. Did you look at https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_tag_example (as part of https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_using_forges_hosted_revision_control) that I linked earlier? If you find that too complicated you can instead also cheat using anchor links to get a properly named source file:
   Instead of
     Source:         %{url}/archive/refs/tags/v%{version}.tar.gz
   use
     Source:         %{url}/archive/refs/tags/v%{version}.tar.gz#/%{name}-%{version}.tar.gz
   That way you can also drop the "-n" parameter on the `%autosetup` macro.

3. It's up to you whether you want to carry the changes to the systemd unit file as a patch. I find it more work possibly having to rework the patch if it no longer cleanly applies. Simply carrying the entire systemd unit file as an extra `Source` or using `sed` in the `%prep` section (see an example here: https://src.fedoraproject.org/rpms/borgmatic/blob/3f772e679a94c823337ff22084166e3346cb4020/f/borgmatic.spec#_48) would be acceptable as well.

4. What's the purpose of defining the description via %global _description and then doing %description %_description? %_description is used nowhere else so it may be redundant.

5. The systemd unit is being installed into the wrong location. It needs to go in `%{buildroot}%{_unitdir}/%{name}.service`. Also, using `install -Dpm` instead of `cp -a` allows you to drop the `mkdir` lines. See for example: https://src.fedoraproject.org/rpms/borgmatic/blob/3f772e679a94c823337ff22084166e3346cb4020/f/borgmatic.spec#_49

Other than that it looks pretty good to me. Some other choices you made (like how you sorted the %files section) come down to personal preference/style and they won't be part of the formal review.

Comment 9 Simon de Vlieger 2023-08-07 19:04:16 UTC
Hey @felix. Initially it was me who submitted the package for review. After some back and forth Neil is in a better position to submit this package so they'll be taking over and working through any changes.

However, with all the automated stuff to create the repositories after review+ perhaps it is better to close this one and request a new review under Neil's name. What do you think?

Comment 10 Neil Hanlon 2023-08-07 21:46:28 UTC
Adding my comments from email here, as I thought they'd be appended but don't appear to have been:

(In reply to Felix Kaechele from comment #8)
> I'm a bit confused as to who is submitting this package for review at this
> point? Or are neil and supakeen the same person?

I can add some clarification, sorry for the confusion! We're not the same person, I just happened to think "hey I should upstream my pinnwand package" and found this existing package review, and having worked with supakeen in the past, I thought I'd update the srpm and I was going to request someone else review this and offer to comaintain it with supakeen. My colleague is also submitting a review request for the client component to pinnwand--steck--very soon.

> 
> Other than that, here are a few more comments, going through the spec file
> top to bottom:

Thank you!

> 
> 1. For the "Release" field use %autorelease:
> https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/
> #_release_tag

I'm not personally a fan of using this mechanism right now, and decided to keep the author's choice to use manual release versioning for now.

> 
> 2. Did you look at
> https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/
> #_tag_example (as part of
> https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/
> #_using_forges_hosted_revision_control) that I linked earlier? If you find

I did, yes, however it is my understanding that it is not currently recommended to use forge macros, as there are several outstanding issues with them.


> that too complicated you can instead also cheat using anchor links to get a
> properly named source file:
>    Instead of
>      Source:         %{url}/archive/refs/tags/v%{version}.tar.gz
>    use
>      Source:        
> %{url}/archive/refs/tags/v%{version}.tar.gz#/%{name}-%{version}.tar.gz
>    That way you can also drop the "-n" parameter on the `%autosetup` macro.

Good call, thank you. I'll make this change.

> 
> 3. It's up to you whether you want to carry the changes to the systemd unit
> file as a patch. I find it more work possibly having to rework the patch if
> it no longer cleanly applies. Simply carrying the entire systemd unit file
> as an extra `Source` or using `sed` in the `%prep` section (see an example
> here:
> https://src.fedoraproject.org/rpms/borgmatic/blob/
> 3f772e679a94c823337ff22084166e3346cb4020/f/borgmatic.spec#_48) would be
> acceptable as well.

I'm going to discuss this with supakeen; agreed that it would probably be better to use sed or otherwise put our "own" unit file in. (Indeed, this was Supakeen's preference)


> 
> 4. What's the purpose of defining the description via %global _description
> and then doing %description %_description? %_description is used nowhere
> else so it may be redundant.

I suspect this is carryover from the python spec template, but agree it's not necessary for this project as there are not subpackages (nor candidates to put into subpackages, if I remember correctly).


> 
> 5. The systemd unit is being installed into the wrong location. It needs to
> go in `%{buildroot}%{_unitdir}/%{name}.service`. Also, using `install -Dpm`
> instead of `cp -a` allows you to drop the `mkdir` lines. See for example:
> https://src.fedoraproject.org/rpms/borgmatic/blob/
> 3f772e679a94c823337ff22084166e3346cb4020/f/borgmatic.spec#_49

Thank you for these tips! I'll implement them and some of the above.


> 
> Other than that it looks pretty good to me. Some other choices you made
> (like how you sorted the %files section) come down to personal
> preference/style and they won't be part of the formal review.

I enjoy chaos :)

Comment 11 Felix Kaechele 2023-08-07 23:10:07 UTC
Cool!

Opening a new review and closing this one as a duplicate of the other one would be what I'd do in this situation. The latter is probably not necessary but opening the request under your own account is, as you mentioned, required for the process automation to work.

I'll be happy to do the review.

Comment 12 Neil Hanlon 2023-08-08 03:27:20 UTC

*** This bug has been marked as a duplicate of bug 2229857 ***


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