[Bug 2106138] Review Request: pinnwand - Straightforward pastebin software

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



--- Comment #10 from Neil Hanlon <neil@xxxxxxxx> ---
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 :)


-- 
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=2106138

Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202106138%23c10
_______________________________________________
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




[Index of Archives]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite Conditions]     [KDE Users]

  Powered by Linux