Bug 1979081 - Review Request: rubygem-gist - Gist uploader command line utility for GitHub
Summary: Review Request: rubygem-gist - Gist uploader command line utility for GitHub
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Jarek Prokop
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2021-07-04 19:30 UTC by Georg Sauthoff
Modified: 2021-08-23 01:31 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2021-08-23 01:03:31 UTC
Type: Bug
Embargoed:
jprokop: fedora-review+


Attachments (Terms of Use)

Description Georg Sauthoff 2021-07-04 19:30:49 UTC
I've created a .spec file for the 'gist' rubygem:

https://raw.githubusercontent.com/gsauthof/copr-fedora/f53b8548d0529df1c01bd541d82ddb09f92198c8/rubygem-gist/rubygem-gist.spec

Source RPM:

https://download.copr.fedorainfracloud.org/results/gsauthof/fedora/fedora-rawhide-x86_64/02313970-rubygem-gist/rubygem-gist-6.0.0-1.fc35.src.rpm


The corresponding copr build:
https://copr.fedorainfracloud.org/coprs/gsauthof/fedora/build/2313970/


I started from a spec file generated by gem2rpm, but modified some fields/sections where necessary.

AFAICS, there isn't a gist-upload command included in Fedora, currently. IIRC, some releases ago there was one, which was written in Python, but that one was abandoned and orphaned.


I think that it's a useful command, thus I packaged it and now I'm looking for a reviewer.


My Fedora (FAS) username is: gsauthof

Comment 1 Jarek Prokop 2021-08-08 10:40:07 UTC
Taking this for a review.

Comment 2 Jarek Prokop 2021-08-08 17:41:07 UTC
if both packages provide the `ronn` binary, it is better to use `BuildRequires: %{_bindir}/ronn`:
This minimal patch should work
~~~
-%if 0%{?fedora} < 34
-BuildRequires: rubygem(ronn)
-%else
-BuildRequires: rubygem(ronn-ng)
-%endif
+BuildRequires: %{_bindir}/ronn
~~~

  JFTR upstream provides man pages which could be used as additional source instead like this: `Source1: https://github.com/defunkt/%{gem_name}/blob/v%{version}/build/%{gem_name}.1`
  I recommend querying upstream if they could include it in the built gem on RubyGems.

* The Summary and Descriptions seem to differ from the upstream, let's keep it consistent with upstream.

* Vendored library `vendor/json.rb`
  - While upstream has a use-case for this we do not as it is for homebrew users (AFAICT from the repo), the file should be deleted for distribution in Fedora Linux.
  - There should probably be added a license header on that file and an addition to the package's `License:` field if the file is kept in the resulting RPM, since it looks like it was copied from merging 2 files from upstream Ruby: https://github.com/flori/json/tree/master/lib/json/pure

And a small nit: 
* The `BuildRequires: rubygem(rspec) > 3` restricting condition is probably not needed as Fedora in general ships rubygem-rspec > 3.

Comment 3 Georg Sauthoff 2021-08-08 21:47:45 UTC
Hm, the summary is pretty much what upstream provides:

rpm: Upload code to gist.github.com 
upstream: upload code to https://gist.github.com

So this is from the upstream readme/man page.

Whereas the upstream help starts with another tag line:

> Gist (v#{Gist::VERSION}) lets you upload to https://gist.github.com/

However, I don't mind adding the https:// prefix to the RPM summary.


The RPM description basically summarizes the README.md. I think it is a better summary than just copying the Synopsis from the README/man page because it describes additional key points while still keeping it very short.



I'll remove the vendored json and the superfluous rspec dependency.

FWIW, the rspec depedency comes from the gem2rpm template.

Comment 4 Jarek Prokop 2021-08-08 22:35:21 UTC
(In reply to Georg Sauthoff from comment #3)
> Hm, the summary is pretty much what upstream provides:
> 
> rpm: Upload code to gist.github.com 
> upstream: upload code to https://gist.github.com
> 
> So this is from the upstream readme/man page.
> 
> Whereas the upstream help starts with another tag line:
> 
> > Gist (v#{Gist::VERSION}) lets you upload to https://gist.github.com/
> 
> However, I don't mind adding the https:// prefix to the RPM summary.

Hmm, fresh run of gem2rpm gives me:
~~~
Summary: Just allows you to upload gists
~~~
Which to me seems good enough.

and description:
~~~
%description
Provides a single function (Gist.gist) that uploads a gist.
~~~

JFTR the fresh specfile: https://gist.github.com/jackorp/3e88a6abe5c5e57f933d6dc6130a1df2

IIRC both these values are taken from the gem's gemspec, which should be our source of truth for these things (most of the time anyways).

> 
> The RPM description basically summarizes the README.md. I think it is a
> better summary than just copying the Synopsis from the README/man page
> because it describes additional key points while still keeping it very short.
> 
> 
> 
> I'll remove the vendored json and the superfluous rspec dependency.
> 
> FWIW, the rspec depedency comes from the gem2rpm template.

Those are BuildRequires taken from gemspec, the maintainer should then decide what makes sense to be included and what does not ;)

>  I recommend querying upstream if they could include it in the built gem on RubyGems.

Just found out it was removed on purpose.

https://github.com/defunkt/gist/commit/b58ff3eb9dd8403af3f801be0eafcb2eb6f4ab2a

But I think we can still take the generated manpage from upstream (if they are current that is), which eases up on build dependencies and we don't have to generate it ourselves.
(Writing it again, as I forgot that it should be a raw textfile) `Source1: https://raw.githubusercontent.com/defunkt/gist/v6.0.0/build/gist.1`

Comment 5 Georg Sauthoff 2021-08-08 23:37:19 UTC
> Those are BuildRequires taken from gemspec, the maintainer should then decide what makes sense to be included and what does not ;)

Sure, but if a requirement is indeed superfluous on fedora, then gem2rpm could filter it, i.e. when using a fedora template.

However,

> This minimal patch should work
> ~~~
> -%if 0%{?fedora} < 34
> -BuildRequires: rubygem(ronn)
> -%else
> -BuildRequires: rubygem(ronn-ng)
> -%endif
> +BuildRequires: %{_bindir}/ronn
> ~~~

No, it doesn't work. With that change the build fails on Copr:

+ rspec spec
/var/tmp/rpm-tmp.UCjJBV: line 33: rspec: command not found

https://copr.fedorainfracloud.org/coprs/gsauthof/fedora/build/2454151/

Comment 6 Georg Sauthoff 2021-08-08 23:51:25 UTC
As I wrote in my initial posting:

> I started from a spec file generated by gem2rpm, but modified some fields/sections where necessary.

I revisited it again, the rspec build-requires comes from my modified gem2rpm fedora template. So, I take this back:

> Sure, but if a requirement is indeed superfluous on fedora, then gem2rpm could filter it, i.e. when using a fedora template.

(since there is nothing to filter for gem2rpm)

This is also the reason why you see the different summary/description.

I still think that my summary/description is an improvement over the default ones.

> ~~~
> Summary: Just allows you to upload gists
> ~~~
> Which to me seems good enough.

I disagree. If you don't already know what 'gists' are and that rubygem-gist uploads to github, then it doesn't tell you much.

Comment 7 Jarek Prokop 2021-08-09 07:40:49 UTC
> I revisited it again, the rspec build-requires comes from my modified gem2rpm fedora template. So, I take this back:
no, that one should be there since that's what the test suite needs to run (at according to gemspec as well). And that is the reason for the build failure; the rubygem-rspec package is not present.
I was talking about the conditional `> 3`, since Fedora does not provide rubygem-rspec <= 2, it is superfluous.

> I still think that my summary/description is an improvement over the default ones.
It does descript the library better. Then I have only one small nit regarding Summary:

`Upload code to gist.github.com`

`Upload text to gist.github.com`

Because it does not need to be code to upload it.

Comment 8 Georg Sauthoff 2021-08-10 20:36:03 UTC
>> I revisited it again, the rspec build-requires comes from my modified gem2rpm fedora template. So, I take this back:
> no, that one should be there since that's what the test suite needs to run (at according to gemspec as well). And that is the reason for the build failure; the rubygem-rspec package is not present.

Yes, this is what I thought, and what I observed a few weeks ago, when I created the package.

> I was talking about the conditional `> 3`, since Fedora does not provide rubygem-rspec <= 2, it is superfluous.

I see, I misread that part.

> > `Upload text to gist.github.com`
>
> Because it does not need to be code to upload it.

Fair enough, what about 'content' then?


I've updated the .spec file:

https://raw.githubusercontent.com/gsauthof/copr-fedora/e7e3fe19107bb6de63c83c7e0527f035d3257af9/rubygem-gist/rubygem-gist.spec

Corresponding Copr build (all green on rawhide/f33/f34):

https://copr.fedorainfracloud.org/coprs/gsauthof/fedora/build/2504491/

Comment 9 Jarek Prokop 2021-08-11 22:06:32 UTC
Package approved.

Thanks!

Comment 10 Georg Sauthoff 2021-08-13 16:28:44 UTC
Thank you for reviewing the package!

Comment 11 Gwyn Ciesla 2021-08-13 19:32:45 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/rubygem-gist

Comment 12 Fedora Update System 2021-08-14 10:46:07 UTC
FEDORA-2021-7ebe4ed80e has been submitted as an update to Fedora 33. https://bodhi.fedoraproject.org/updates/FEDORA-2021-7ebe4ed80e

Comment 13 Fedora Update System 2021-08-14 10:46:10 UTC
FEDORA-2021-7e20a5478d has been submitted as an update to Fedora 34. https://bodhi.fedoraproject.org/updates/FEDORA-2021-7e20a5478d

Comment 14 Fedora Update System 2021-08-15 01:16:59 UTC
FEDORA-2021-7ebe4ed80e has been pushed to the Fedora 33 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --advisory=FEDORA-2021-7ebe4ed80e \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2021-7ebe4ed80e

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 15 Fedora Update System 2021-08-15 02:01:33 UTC
FEDORA-2021-7e20a5478d has been pushed to the Fedora 34 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --advisory=FEDORA-2021-7e20a5478d \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2021-7e20a5478d

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 16 Fedora Update System 2021-08-23 01:03:31 UTC
FEDORA-2021-7ebe4ed80e has been pushed to the Fedora 33 stable repository.
If problem still persists, please make note of it in this bug report.

Comment 17 Fedora Update System 2021-08-23 01:31:46 UTC
FEDORA-2021-7e20a5478d has been pushed to the Fedora 34 stable repository.
If problem still persists, please make note of it in this bug report.


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