Bug 1099166 - Review Request: screenfetch - Display system information
Review Request: screenfetch - Display system information
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Rex Dieter
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2014-05-19 12:05 EDT by Martín Buenahora
Modified: 2014-11-04 08:34 EST (History)
8 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-11-04 08:34:19 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
rdieter: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Martín Buenahora 2014-05-19 12:05:13 EDT
Spec URL: https://git.fedorahosted.org/cgit/screenfetch.git/tree/screenfetch.spec
SRPM URL: https://git.fedorahosted.org/cgit/screenfetch.git/tree/screenfetch-3.2.2-1.fc20.src.rpm

Description: screenFetch is a "Bash Screenshot Information Tool". 
This handy Bash script can be used to generate one of 
those nifty terminal theme information + ASCII distribution
logos you see in everyone's screenshots nowadays. It will 
auto-detect your distribution and display an ASCII version
of that distribution's logo and some valuable information 
to the right. There are options to specify no ascii art, 
colors, taking a screenshot upon displaying info, and even 
customizing the screenshot command! This script is very easy 
to add to and can easily be extended.

Fedora Account System Username:zironid

This is my first package, so I need an sponsor. (Note: Sponsor will recive a big "Thank you!")

Succseful Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=6863548
Comment 1 Alejandro_Perez 2014-05-19 12:55:06 EDT
Please set the SRPM URL to a proper download site, you can use your home on fedorapeople.org to do so.
Comment 2 Jason Taylor 2014-05-19 13:31:03 EDT
Hi Martin,

Are you planning on building for EPEL? If not, the %clean and rm -rf %{buildroot} items should be removed in accordance with the packaging guidelines (http://fedoraproject.org/wiki/Packaging:Guidelines)

JT
Comment 3 Eduardo Echeverria 2014-05-19 21:02:34 EDT
Hi Martin. 

As said @Jason, some tags are specifically EPEL5's dependents, please don't use it. refer to epel packaging guidelines for further details (It is important to note that epel6 guidelines are the same of fedora) https://fedoraproject.org/wiki/EPEL:Packaging.

This is a snapshot or stable release? if is a snapshot, check this guidelines https://fedoraproject.org/wiki/Packaging:NamingGuidelines?rd=Packaging/NamingGuidelines#Snapshot_packages

If you not have anything to build in %build section, put it a comment as "Nothing to build" there 

I think that screenfetch-dev it is not a good name for an "binary", I recommend rename it.

From my POW, bash not should be a explicit requirement, due to is the default in fedora and other linux distributions. 

For now, I will take this, I'm waiting your fixes soon. :)
Comment 4 Martín Buenahora 2014-05-19 23:23:14 EDT
(In reply to Eduardo Echeverria from comment #3)
> Hi Martin. 
> 
> As said @Jason, some tags are specifically EPEL5's dependents, please don't
> use it. refer to epel packaging guidelines for further details (It is
> important to note that epel6 guidelines are the same of fedora)
> https://fedoraproject.org/wiki/EPEL:Packaging.
> 
> This is a snapshot or stable release? if is a snapshot, check this
> guidelines
> https://fedoraproject.org/wiki/Packaging:NamingGuidelines?rd=Packaging/
> NamingGuidelines#Snapshot_packages
> 
> If you not have anything to build in %build section, put it a comment as
> "Nothing to build" there 
> 
> I think that screenfetch-dev it is not a good name for an "binary", I
> recommend rename it.
> 
> From my POW, bash not should be a explicit requirement, due to is the
> default in fedora and other linux distributions. 
> 
> For now, I will take this, I'm waiting your fixes soon. :)

Ok, thank you.

I think the "screenfetch-dev" file must be linked to "screenfetch". I think that's better than renaming the file itself, because it makes easier to maintain.
Comment 5 Martín Buenahora 2014-05-19 23:26:19 EDT
(In reply to Martín Buenahora from comment #0)
> Spec URL:
> https://git.fedorahosted.org/cgit/screenfetch.git/tree/screenfetch.spec
> SRPM URL:
> https://git.fedorahosted.org/cgit/screenfetch.git/tree/screenfetch-3.2.2-1.
> fc20.src.rpm
> 
> Description: screenFetch is a "Bash Screenshot Information Tool". 
> This handy Bash script can be used to generate one of 
> those nifty terminal theme information + ASCII distribution
> logos you see in everyone's screenshots nowadays. It will 
> auto-detect your distribution and display an ASCII version
> of that distribution's logo and some valuable information 
> to the right. There are options to specify no ascii art, 
> colors, taking a screenshot upon displaying info, and even 
> customizing the screenshot command! This script is very easy 
> to add to and can easily be extended.
> 
> Fedora Account System Username:zironid
> 
> This is my first package, so I need an sponsor. (Note: Sponsor will recive a
> big "Thank you!")
> 
> Succseful Koji build:
> http://koji.fedoraproject.org/koji/taskinfo?taskID=6863548

Right SRPM URL: https://dl-web.dropbox.com/get/Public/screenfetch-3.2.2-1.fc20.src.rpm?_subject_uid=268180465&w=AAC-de8wwaojQ5cjqfAt6Rk145f4vgG3p9KkI1kIm8XgKA
Comment 6 Martín Buenahora 2014-05-19 23:27:15 EDT
(In reply to Alejandro_Perez from comment #1)
> Please set the SRPM URL to a proper download site, you can use your home on
> fedorapeople.org to do so.

Ok, added as a comment
Comment 7 Martín Buenahora 2014-05-20 17:08:54 EDT
Alright, I've fixed the problems.

SPEC URL: https://git.fedorahosted.org/cgit/screenfetch.git/tree/screenfetch.spec
SRPM URL: https://dl-web.dropbox.com/get/Public/screenfetch-3.2.2-2.fc20.src.rpm?_subject_uid=268180465&w=AAAi4bCOmPOg3DPHf9eHgdqhPM9FXKyXmmxHVN3Yn0J_xA
Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=6868427

@Eduardo, I think it's a rellease, because in the .tar.bz download page was marked like that. As you can see, I'm not making the .tar.bz file.
Upstream page:https://github.com/KittyKatt/screenFetch
tar.bz download page: http://git.silverirc.com/cgit.cgi/screenfetch.git/
Comment 8 Martín Buenahora 2014-05-20 17:11:26 EDT
(In reply to Martín Buenahora from comment #7)
> Alright, I've fixed the problems.
> 
> SPEC URL:
> https://git.fedorahosted.org/cgit/screenfetch.git/tree/screenfetch.spec
> SRPM URL:
> https://dl-web.dropbox.com/get/Public/screenfetch-3.2.2-2.fc20.src.
> rpm?_subject_uid=268180465&w=AAAi4bCOmPOg3DPHf9eHgdqhPM9FXKyXmmxHVN3Yn0J_xA
> Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=6868427
> 
> @Eduardo, I think it's a rellease, because in the .tar.bz download page was
> marked like that. As you can see, I'm not making the .tar.bz file.
> Upstream page:https://github.com/KittyKatt/screenFetch
> tar.bz download page: http://git.silverirc.com/cgit.cgi/screenfetch.git/

Whops. It's .tar.gz instead of .tar.bz. Sorry
Comment 9 Martín Buenahora 2014-09-01 12:09:59 EDT
Uhm, sorry, I've been busy these lasts months, so I couldn't talk much. Anyone still around here?
Comment 10 Eduardo Mayorga 2014-09-01 21:10:58 EDT
SRPM URL is broken.

Please paste link to *plain* spec file rather than a cgit tree view. This allows the reviewer to automate a bit the review.
Comment 12 Martín Buenahora 2014-09-02 00:08:45 EDT
I think I should have included the SRPM download link rather than the cgit view: https://git.fedorahosted.org/cgit/screenfetch.git/plain/screenfetch-3.2.2-1.fc20.src.rpm

(Sorry -.- )
Comment 13 Rex Dieter 2014-09-09 12:37:58 EDT
echevemaster,

it's been awhile since your last activity/comment here (almost 5 months), do you still want to continue and finish this review?

(Else, I'll pick it up in a few days)
Comment 14 Martín Buenahora 2014-09-09 17:12:06 EDT
Well, after reading some messages here (and thanks to Eduardo Mayorga ;) ) I realized that I forgot to delet rm -rf %{buildroot} from the install section.

SPEC: https://git.fedorahosted.org/cgit/screenfetch.git/plain/screenfetch.spec
SRPM: https://git.fedorahosted.org/cgit/screenfetch.git/plain/screenfetch-3.2.2-3.fc20.src.rpm

Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=7558594
Comment 15 Martín Buenahora 2014-09-09 21:04:25 EDT
I think I'm going to be in your spam folder soon...

As I haven't checked the upstream page, various major versions has been released since I made my first package, so I made a package of the newest version, and started using snapshot as Version. 

I just have one doubt: ¿Is there any problem in that the name contain a capital letter? In the naming guidelines says I can, but its a bit weird a package named screenFetch. I leaved the F capital because the upstream tarball have it, so it's a bit complicated to change it.

Anyway, here are the new links:

SPEC URL: https://git.fedorahosted.org/cgit/screenfetch.git/plain/screenFetch.spec
SRPM URL: https://git.fedorahosted.org/cgit/screenfetch.git/plain/screenFetch-20140914git-1.fc20.src.rpm
Koju build : http://koji.fedoraproject.org/koji/taskinfo?taskID=7560102

Thanks in advance
Comment 16 Martín Buenahora 2014-09-12 20:18:41 EDT
Hey Rex Dieter.

echevemaster seems to be in a different rhythm that mine, and I don't want to lose my enthusiasm. Could you please review my package?
Comment 17 Raphael Groner 2014-09-13 04:33:49 EDT
This is no formal review. But I have some general comments to your spec file.

> Name:           screenFetch
Try to keep a consistent name and don't mix upper/lower-case: screenfetch

> Version:        20140914git
Do you plan to release always from git with date as version? Maybe we should think about a better versioning scheme to be able to switch to main releases. At least put 0. before: 0.20140914git

> %description
> screenFetch is a "Bash Screenshot Information Tool". 
You can remove that line from %description, because it's / should be the Summary.

> %files
> …
> %{_mandir}/man1/screenfetch.1.gz
That manpage should be with %doc in front:
%doc %{_mandir}/man1/screenfetch.1.gz


> %changelog
> * Tue Sep  9 2014 Martín Buenahora <martin@localhost.localdomain> - 20140914git-1
Please provide an official and working e-mail address. You can use your one from FAS (login: zironid).
Comment 18 Martín Buenahora 2014-09-13 10:51:38 EDT
(In reply to Raphael Groner from comment #17)
> This is no formal review. But I have some general comments to your spec file.
> 
> > Name:           screenFetch
> Try to keep a consistent name and don't mix upper/lower-case: screenfetch
> 
> > Version:        20140914git
> Do you plan to release always from git with date as version? Maybe we should
> think about a better versioning scheme to be able to switch to main
> releases. At least put 0. before: 0.20140914git
> 
> > %description
> > screenFetch is a "Bash Screenshot Information Tool". 
> You can remove that line from %description, because it's / should be the
> Summary.
> 
> > %files
> > …
> > %{_mandir}/man1/screenfetch.1.gz
> That manpage should be with %doc in front:
> %doc %{_mandir}/man1/screenfetch.1.gz
> 
> 
> > %changelog
> > * Tue Sep  9 2014 Martín Buenahora <martin@localhost.localdomain> - 20140914git-1
> Please provide an official and working e-mail address. You can use your one
> from FAS (login: zironid).

Hello Raphael.

I've changed the Summary, the manpages location and the e-mail (I'm using an Emacs that automatically adds entries to the changelog, and uses that as the default e-mail), and changed the F from the name.

I have a doubt about the version. The upstream have releases, as you have seen, but they aren't really defined (I remember one commit that said something like "Is been a while since a new release"), so I don't know if I should come back to the releases, or if I should stay with the git version (I would use "<date>git<short commit>" rather than "<date>git").
Comment 19 Raphael Groner 2014-09-13 11:33:43 EDT
(In reply to Martín Buenahora from comment #18)
> I have a doubt about the version. The upstream have releases, as you have
> seen, but they aren't really defined (I remember one commit that said
> something like "Is been a while since a new release"), so I don't know if I
> should come back to the releases, or if I should stay with the git version
> (I would use "<date>git<short commit>" rather than "<date>git").

You could also "do the AUR way" and use a suffix, that means 'screenfetch-git' as a (temporary) package name, till there would be any main release at upstream. So there could be two packages (git and "official" release). Just as an idea, you're of course free to name your package as you prefer if the guidelines comply.

See also
https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Non-Numeric_Version_in_Release
Comment 20 Martín Buenahora 2014-09-13 11:42:18 EDT
(In reply to Raphael Groner from comment #19)
> You could also "do the AUR way" and use a suffix, that means
> 'screenfetch-git' as a (temporary) package name, till there would be any
> main release at upstream. So there could be two packages (git and "official"
> release). Just as an idea, you're of course free to name your package as you
> prefer if the guidelines comply.
> 
> See also
> https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Non-
> Numeric_Version_in_Release

Well, the main reason for switching to git version was because the URL where I was downloading the sources died, and the tar.gz downloaded from git have a different name than the one in the URL, but now I know how to fix that. ¿It would be better to stay in release version? Because I don't want to make a new package for every commit...
Comment 21 Rex Dieter 2014-09-15 08:41:54 EDT
Re: Comment #19

"the AUR  way" isn't the fedora way.  Please don't follow that advice here, but rather:
https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Snapshot_packages


Looks like the existing reviewer has not responded, so I'll take it from here.
Comment 22 Rex Dieter 2014-09-15 08:48:07 EDT
Initial comments:

1.  MUST: fix version/release to match snapshot guidelines referenced in comment #21

2.  MUST: Your %changelog is pretty empty.  Please update changelog for *every* change made to the packaging, including those here.  See also:
https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Changelogs

3.  SHOULD drop extraneous "mkdir" in %install section, and use install -D flag installing the stuff to %_bindir instead (it'll create the target dir(s) for you).  While you're at it, use -p flag too to preserve timestamps.
Comment 23 Rex Dieter 2014-09-15 08:54:45 EDT
And with item 2, while fixing changelog, please use a valid email address.  If you'd rather not use a personal one, then zironid@fedoraproject.org will suffice too.
Comment 24 Martín Buenahora 2014-09-15 09:45:26 EDT
(In reply to Rex Dieter from comment #22)
> Initial comments:
> 
> 1.  MUST: fix version/release to match snapshot guidelines referenced in
> comment #21
> 

As the upstream does occasionally releases, I prefer using that method, rather that commit versioning, as I don't totally understand how will be that way.  

> 2.  MUST: Your %changelog is pretty empty.  Please update changelog for
> *every* change made to the packaging, including those here.  See also:
> https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/
> Guidelines#Changelogs
>

(I haven't looked at the changelog guidelines yet)
As I've been changing the version from release to git, I thought that I had to remove them, because rpmlint showed warnings of "inconsistent changelog". As I uploaded everything to a git repo, I can recover the changelogs, put them at the begining, and change the release to 3.2.2, and the version of the package to the number that should be.
 
> 3.  SHOULD drop extraneous "mkdir" in %install section, and use install -D
> flag installing the stuff to %_bindir instead (it'll create the target
> dir(s) for you).  While you're at it, use -p flag too to preserve timestamps.

Ok.

I hope to be making the changes today.
Comment 25 Rex Dieter 2014-09-15 10:01:33 EDT
"As the upstream does occasionally releases, I prefer using that method, rather
that commit versioning, as I don't totally understand how will be that way.  "

The method is approximately the same, it just matters how you treat the
Version:
Release:
tags differently.

fedora's guildelines recommend you use
Version:
to refer to either the previous or upcoming application Version number

and put the git commit/tag into
Release:

so, in this case, you'll end up with something like:

Version: 3.2.2
Release: 1.20140914gitdec1cd6


I'd recommend you (re)read:
https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Snapshot_packages

and let me know if you have any more questions.
Comment 26 Gwyn Ciesla 2014-09-15 11:07:06 EDT
No SCM request found.
Comment 27 Rex Dieter 2014-09-15 11:12:31 EDT
Oops, I'd set fedora-cvs instead of fedora-review flag, sorry. (fixed)
Comment 28 Martín Buenahora 2014-09-15 12:43:41 EDT
Well, finally I'm happy with Version/Release tag :)

I recovered the old changelog entries, and changed the release to 
3.6.2-<number of release>.20140914gitdec1cd6c, and made the changes you showed me.

SPEC URL: https://git.fedorahosted.org/cgit/screenfetch.git/plain/screenfetch.spec
SRPM URL: https://git.fedorahosted.org/cgit/screenfetch.git/plain/screenfetch-3.6.2-5.20140914gitdec1cd6c.fc20.src.rpm

Koji build (for rawhide): http://koji.fedoraproject.org/koji/taskinfo?taskID=7579632
Comment 29 Rex Dieter 2014-09-24 16:14:08 EDT
naming: ok

license: ok

sources: ok
dff25a3e7593526390c695875f9c2ec0  screenFetch-dec1cd6c2471defe4459967fbc8ae15b55714338.tar.gz

1. SHOULD consider replacing in %files:
%doc %{_mandir}/man1/screenfetch.1.gz
with
%{_mandir}/man1/screenfetch.1*

man pages are automatically marked %doc, and the compressed filename is an implementation detail (ie, theoretically fedora could someday switch to uncompressed or bzip2-compressed manpages, and the build would fail).

scriptlets: ok (N/A)

macros: ok

otherwise, fairly simple and clean.


APPROVED (and sponsored).


you can move on to the next steps of the process:
https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner
Comment 30 Martín Buenahora 2014-09-24 20:59:38 EDT
Thanks for all your help, Rex!!!!! :D

Updated the manpage to * (if you want the spec is here: https://git.fedorahosted.org/cgit/screenfetch.git/plain/screenfetch.spec)

I'll proced to request cvs

Again, Thank you!
Comment 31 Martín Buenahora 2014-09-24 21:04:58 EDT
New Package SCM Request
=======================
Package Name: screenfetch
Short Description: A "Bash Screenshot Information Tool"
Upstream URL: https://github.com/KittyKatt/screenFetch
Owners: zironid
Branches: f21
InitialCC:
Comment 32 Rex Dieter 2014-09-24 21:58:30 EDT
still includes the extraneous %doc
Comment 33 Martín Buenahora 2014-09-24 23:31:59 EDT
(In reply to Rex Dieter from comment #32)
> still includes the extraneous %doc

Sorry, forgot that. Fixed. (https://git.fedorahosted.org/cgit/screenfetch.git/plain/screenfetch.spec)
Comment 34 Gwyn Ciesla 2014-09-25 08:38:32 EDT
Git done (by process-git-requests).
Comment 35 Eduardo Echeverria 2014-09-27 09:49:43 EDT
I want apologize, I'm unable to receive email notification from bugzilla. I'm investigating the cause, but still I don't know what happens.
Comment 36 Raphael Groner 2014-10-31 12:05:28 EDT
Already pushed to stable.

https://admin.fedoraproject.org/updates/FEDORA-2014-11616/screenfetch-3.6.2-7.20140914gitdec1cd6c.fc21

Would you mind to provide also a F20 package? If not, you could set CLOSED RAWHIDE.
Comment 37 Martín Buenahora 2014-11-04 08:34:19 EST
(In reply to Raphael Groner from comment #36)
> Already pushed to stable.
> 
> https://admin.fedoraproject.org/updates/FEDORA-2014-11616/screenfetch-3.6.2-
> 7.20140914gitdec1cd6c.fc21
> 
> Would you mind to provide also a F20 package? If not, you could set CLOSED
> RAWHIDE.

Hello. 

Sorry for the delayed response.

Originally, I planned the package to be for F20, but the time passed and when the package was reviewed it was changed to F21, due to the short time remaining for F21 to get released, something that is closer now than then.

So I'll set it as CLOSED RAWHIDE.

Thanks for the advice.

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