Bug 1422714 (yank) - Review Request: yank - tool for yanking (copying) stdin to clipboard
Summary: Review Request: yank - tool for yanking (copying) stdin to clipboard
Keywords:
Status: CLOSED ERRATA
Alias: yank
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Rex Dieter
QA Contact: Fedora Extras Quality Assurance
URL: https://github.com/mptre/yank
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-02-16 00:28 UTC by Nemanja Milosevic
Modified: 2017-03-24 17:50 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-03-24 17:50:50 UTC
Type: ---
Embargoed:
rdieter: fedora-review+


Attachments (Terms of Use)

Description Nemanja Milosevic 2017-02-16 00:28:50 UTC
Spec URL: https://pagure.io/yank-rpm/blob/master/f/yank.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/nmilosev/yank/fedora-25-x86_64/00512635-yank/yank-0.8.0-1.fc25.src.rpm (for F25)
COPR: https://copr.fedorainfracloud.org/coprs/nmilosev/yank/
Description: This handy tool is very useful as it allows you to copy the output of some command without using a mouse. You usually use it like this: [command] | yank

Fedora Account System Username:nmilosev

This is my first package I'm submitting for a review. Please let me know if I need to do anything else. I've dwelled with packaging before, but this is my first time trying to become sponsored as a packager.

This is my wiki page: https://fedoraproject.org/wiki/User:Nmilosev

I'm also helping package the much bigger .NET Core Framework, working together with the dotnet SIG (https://copr.fedorainfracloud.org/coprs/nmilosev/dotnet-clean/)

Thanks!

Comment 1 Björn 'besser82' Esser 2017-02-16 07:25:29 UTC
Taken!

***

You should use a link to the "raw" spec-file url…  Fixing this quickly for you.

Spec URL: https://pagure.io/yank-rpm/raw/master/f/yank.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/nmilosev/yank/fedora-25-x86_64/00512635-yank/yank-0.8.0-1.fc25.src.rpm

Comment 2 Nemanja Milosevic 2017-02-16 13:13:52 UTC
Ah, sorry about the URL, didn't know it needs to be a direct link.

Please let me know when you review the spec file, and what changes are needed. :)

And thanks for your help.

Cheers,
Nemanja

Comment 3 Nemanja Milosevic 2017-02-16 17:28:13 UTC
After reading a few spec files which are to be reviewed I saw some powerful stuff which I wasn't using. Like using 'install' instead of 'cp', and pulling sources from git automatically (spectool). So added all those to my .spec file:

https://pagure.io/yank-rpm/raw/master/f/yank.spec

Also pushed new build to COPR: 

https://copr.fedorainfracloud.org/coprs/nmilosev/yank/build/512881/

These were all small changes, but I want the spec to be as high quality as possible.

Comment 4 Nemanja Milosevic 2017-02-16 17:30:53 UTC
Only thing I am unsure about in my spec file is about manual installation, please advise if I should use make_install or it isn't mandatory.

Comment 5 Nemanja Milosevic 2017-02-16 17:53:42 UTC
Here is another version: https://pagure.io/yank-rpm/raw/makebuild/f/yank.spec

It's a lot shorter, not sure which is more "clean". Let me know! :)

Comment 6 Ralf Corsepius 2017-02-16 18:25:24 UTC
* Why are you disabling debuginfo?
..
%global debug_package %{nil}
..
This in general is not helpful and therefore is not allowed. Please remove it.

* Renaming binaries breaks debuginfo - Is this why you are disabling debuginfos?

You probably need to rename the program's name to be compiled (ie. to change/patch the Makefile and change the program's name there)


* The package does not use automake
Please remove BuildRequires: automake

* Building doesn't honor RPM_OPT_FLAGS.
Please append CFLAGS=${RPM_OPT_FLAGS} to %make_build


* The SOURCE0-URL is better written this way:
Source0:        %{url}/archive/v%{version}.tar.gz#/%{name}-%{version}.tar.gz

Retrieving the package under this URL then will create a tarball named yank-0.8.0.tar.gz tarball and "v0.8.0.tar.gz"

Comment 7 Nemanja Milosevic 2017-02-16 20:17:54 UTC
(In reply to Ralf Corsepius from comment #6)
> * Why are you disabling debuginfo?
> ..
> %global debug_package %{nil}
> ..
> This in general is not helpful and therefore is not allowed. Please remove
> it.
> 
> * Renaming binaries breaks debuginfo - Is this why you are disabling
> debuginfos?
> 

Yes, that's why I did it. In the Makefile there was a install -s which stripped the information, so I wrote a patch to allow debuginfo to be generated. 

> You probably need to rename the program's name to be compiled (ie. to
> change/patch the Makefile and change the program's name there)
> 

Wrote a patch for this too, spec file is much cleaner now. :)

> 
> * The package does not use automake
> Please remove BuildRequires: automake
> 

Oops, didn't notice that one, thank you!

> * Building doesn't honor RPM_OPT_FLAGS.
> Please append CFLAGS=${RPM_OPT_FLAGS} to %make_build
> 

Sorry, didn't know about this. It's in a separate wiki page on the Packaging guidelines. Added now.

> 
> * The SOURCE0-URL is better written this way:
> Source0:        %{url}/archive/v%{version}.tar.gz#/%{name}-%{version}.tar.gz
> 
> Retrieving the package under this URL then will create a tarball named
> yank-0.8.0.tar.gz tarball and "v0.8.0.tar.gz"

Also added, thank you, didn't know you can do this! :)

New spec file: https://pagure.io/yank-rpm/raw/makebuild/f/yank.spec
New COPR build: https://copr.fedorainfracloud.org/coprs/nmilosev/yank/build/512925/
New SRPM: https://copr-be.cloud.fedoraproject.org/results/nmilosev/yank/fedora-25-x86_64/00512925-yank/yank-0.8.0-1.fc25.src.rpm

Thank you so much for the review!

Cheers,
Nemanja

Comment 8 Nemanja Milosevic 2017-02-17 09:02:24 UTC
Worked with the upstream to integrate both patches for the next release.

Please see:

https://github.com/mptre/yank/issues/35
https://github.com/mptre/yank/pull/36

Cheers,
Nemanja

Comment 10 Rex Dieter 2017-02-18 22:10:05 UTC
setting review flag per comment #1

Comment 12 Rex Dieter 2017-02-22 16:39:52 UTC
I'll continue the review, besser hasn't replied in awhile.

Comment 13 Rex Dieter 2017-02-22 16:47:59 UTC
naming: ok

license: ok

sources: ok
1af010fac3f5d594b9645d282e24d35b  yank-0.8.2.tar.gz

macros: ok

scriplets: ok (n/a)

rpmlint clean


Fairly simple package, nicely done. 


APPROVED.

I've sponsored you, welcome to fedora.


Next steps:
https://fedoraproject.org/wiki/Join_the_package_collection_maintainers?rd=PackageMaintainers/Join#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner

Comment 14 Gwyn Ciesla 2017-02-22 19:15:23 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/yank

Comment 15 Fedora Update System 2017-03-16 13:01:56 UTC
yank-0.8.2-2.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2017-9679fa4df4

Comment 16 Fedora Update System 2017-03-16 23:20:51 UTC
yank-0.8.2-2.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-9679fa4df4

Comment 17 Fedora Update System 2017-03-24 17:50:50 UTC
yank-0.8.2-2.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, 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.