Bug 1411984 - Review Request: neofetch - a CLI system information tool written in Bash
Summary: Review Request: neofetch - a CLI system information tool written in Bash
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Ben Rosser
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-01-10 22:22 UTC by Kees de Jong
Modified: 2017-12-17 19:48 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-12-17 19:48:25 UTC
Type: Bug
rosser.bjr: fedora-review+


Attachments (Terms of Use)

Description Kees de Jong 2017-01-10 22:22:31 UTC
Spec file: https://github.com/AquaL1te/neofetch/blob/master/neofetch.spec
SRPM: https://github.com/AquaL1te/neofetch/blob/master/neofetch-2.0.2-1.fc25.src.rpm

Description of package: Neofetch displays information about your system next to an image, your OS logo, or any ascii file of your choice. The main purpose of Neofetch is to be used in screenshots to show other users what OS/distro you're running, what theme/icons you're using and more.

This is my first package.

Comment 1 Mikolaj Izdebski 2017-01-12 12:12:20 UTC
SRPM URL points to HTML document.
You should include your FAS account name.

Spec file looks quite good. The only real problem I can see from it is that the package should own the whole %{_datadir}/%{name}. Also "rm -rf %{buildroot}" are not needed and should be removed.

Comment 2 Michael Schwendt 2017-01-12 23:39:06 UTC
> %build
> 
> %install

Also be aware of:
https://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps


> install -m755 config/config %{buildroot}%{_datadir}/%{name}/config 

There is no other configuration method in /etc or so? Just this executable shell script?


> install -m644 %{name}.* %{buildroot}%{_mandir}/man1

> %{_mandir}/man1/%{name}.1.gz

More correct would be

  %{_mandir}/man1/%{name}.1*

which would cover uncompressed manual pages as well as different compression, because the gzip compression of manual pages is not explicitly done inside this spec file but performed by rpmbuild on-the-fly depending on default rpmbuild configuration.

Comment 3 M. Herdiansyah 2017-01-13 13:41:01 UTC
Hello, I am one of the contributors of neofetch, and the maintainer for the third-party package for neofetch in copr.

My only concern will be in

>%install

Since neofetch does have a Makefile, won't it be better to use

  %make_install

instead of copying the files manually?

Also, for

> %{_datadir}/%{name}/ascii/distro/*
> %{_datadir}/%{name}/config

We can simplify it to

  %{_datadir}/%{name}/*

Comment 4 Kees de Jong 2017-01-17 07:27:22 UTC
I think I added all the (In reply to Mikolaj Izdebski from comment #1)
> SRPM URL points to HTML document.
> You should include your FAS account name.
> 
> Spec file looks quite good. The only real problem I can see from it is that
> the package should own the whole %{_datadir}/%{name}. Also "rm -rf
> %{buildroot}" are not needed and should be removed.

FAS account is 'aqual1te', my OpenPGP key is still syncing across the SKS servers, but can be found on this pool of servers already: https://sks-keyservers.net/pks/lookup?op=vindex&search=0x0E45C98AB51428E6&fingerprint=on&exact=on

If there is no result, hit F5, the pool of servers is rotating the requests in round robin. Most of the servers in the pool have my key already.

Please let me know if I corrected the problems now for the package (links below).



(In reply to Michael Schwendt from comment #2)
> > %build
> > 
> > %install
> 
> Also be aware of:
> https://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps
> 
> 
> > install -m755 config/config %{buildroot}%{_datadir}/%{name}/config 
> 
> There is no other configuration method in /etc or so? Just this executable
> shell script?
> 
> 
> > install -m644 %{name}.* %{buildroot}%{_mandir}/man1
> 
> > %{_mandir}/man1/%{name}.1.gz
> 
> More correct would be
> 
>   %{_mandir}/man1/%{name}.1*
> 
> which would cover uncompressed manual pages as well as different
> compression, because the gzip compression of manual pages is not explicitly
> done inside this spec file but performed by rpmbuild on-the-fly depending on
> default rpmbuild configuration.

There is no system-wide configuration in e.g. /etc. When the script is ran for the first time it copies a config to ~/.config/neofetch. I added your suggestions, thanks!





(In reply to M. Herdiansyah from comment #3)
> Hello, I am one of the contributors of neofetch, and the maintainer for the
> third-party package for neofetch in copr.
> 
> My only concern will be in
> 
> >%install
> 
> Since neofetch does have a Makefile, won't it be better to use
> 
>   %make_install
> 
> instead of copying the files manually?
> 
> Also, for
> 
> > %{_datadir}/%{name}/ascii/distro/*
> > %{_datadir}/%{name}/config
> 
> We can simplify it to
> 
>   %{_datadir}/%{name}/*

Because of the suggestions from comment #2 I couldn't use the Makefile. This is because of the timestamps that need to be preserved. But still a very useful suggestions just like your other suggestions. I added them.

Spec file: https://github.com/AquaL1te/neofetch/blob/master/neofetch.spec
SRPM: https://github.com/AquaL1te/neofetch/raw/master/neofetch-2.0.2-1.fc25.src.rpm

Comment 5 Michael Schwendt 2017-01-20 12:40:40 UTC
> We can simplify it to
> 
>   %{_datadir}/%{name}/*

No, that's not enough, because it would not include the directory %{_datadir}/{%name} itself. See comment 1. 


> Because of the suggestions from comment #2 I couldn't use the Makefile. 

Not true:

  %make_install INSTALL_PROG="install -p"

I would always prefer a working Makefile, if included, and modify it, if necessary, compared with the extra burden that would be imposed on you by having to install everything yourself in the %install section and keeping that section accurate. You could even ship the addition of "-p" upstream.


> %clean
> rm -rf %{buildroot}

https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections


> %{_mandir}/man1/%{name}.1.*

One dot to much. It would not included an uncompressed manual page. This one would:

%{_mandir}/man1/%{name}.1*

Comment 6 M. Herdiansyah 2017-01-20 13:13:33 UTC
(In reply to Michael Schwendt from comment #5)
> > Because of the suggestions from comment #2 I couldn't use the Makefile. 
> 
> Not true:
> 
>   %make_install INSTALL_PROG="install -p"
> 
> I would always prefer a working Makefile, if included, and modify it, if
> necessary, compared with the extra burden that would be imposed on you by
> having to install everything yourself in the %install section and keeping
> that section accurate. You could even ship the addition of "-p" upstream.

In the next release of neofetch, we will change our commands to a POSIX-compliant one, see https://github.com/dylanaraps/neofetch/blob/master/Makefile

This would eliminate the need for install.

Once we release 2.1.0, you can use

  %make_install

just fine.

Comment 7 M. Herdiansyah 2017-01-20 13:29:46 UTC
Also, we have _a lot_ of optional dependencies, see:
https://github.com/dylanaraps/neofetch/wiki/Dependencies

You may want to add some of it (mainly for Xorg handling) to your spec.

Comment 8 Dylan Araps 2017-01-24 04:04:24 UTC
Hiya, thanks for packaging Neofetch. I've got a few suggestions:

**Required Dependencies**

Neofetch only requires `bash` to function. The only other required dependencies are `awk`,  `grep` and `uname`. While these **are** also required programs they're guaranteed to be installed already so I don't bother listing them. You can confirm this by unsetting `$PATH` and re-declaring it with only the 4 programs above. \[1\]

Every other dependency is optional and neofetch will just not display information for that function if a program is missing. What I propose for your package is making `bash` the only required dependency. Right now this spec file makes neofetch have an indirect dependency on the X server.

I'm packaging neofetch for Arch Linux this way and then listing all other dependencies as 'Optional'. \[2\]


**Neofetch 3.0**

Neofetch 3.0 was released yesterday which introduced the POSIX Makefile so it *should* work without modification on your side now.

\[1\]https://gist.githubusercontent.com/dylanaraps/9343178c7792db3c92414a8a6076285e/raw/245d8a2c1c7ee4558264a65188f0831220b1dd49/path.sh
\[2\] https://aur.archlinux.org/packages/neofetch/

Thanks! :)

Comment 9 Kees de Jong 2017-01-24 08:14:00 UTC
(In reply to Dylan Araps from comment #8)
> Every other dependency is optional and neofetch will just not display
> information for that function if a program is missing. What I propose for
> your package is making `bash` the only required dependency. Right now this
> spec file makes neofetch have an indirect dependency on the X server.
> 
> Neofetch 3.0 was released yesterday which introduced the POSIX Makefile so
> it *should* work without modification on your side now.


Hi Dylan,


Thank you for the suggestions! I indeed list a lot of dependencies because I'm discussing these things in IRC to figure out what's the best way to do this. I changed it to 'Suggests' in the spec file for everything except bash. RPM and the DNF depsolver will totally ignore this, in a future release these weak dependencies will be listed as optional. That way people can use that (in the future when DNF supports it) as a reference if not all functionality of the command switches are available. I will discuss this change as well on IRC and I'll create a test branch in git for my experimental suggestions so they won't show up in this ticket :-)

I bumped the spec and SRPM to version 3.0 in the master branch and also did a scratch build on Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=17399343

Later this week I expect to have some final draft for this, I still have to read some packaging docs as well. Stay tuned :-)

Comment 10 Kees de Jong 2017-02-03 07:21:03 UTC
I've tried a few times to get some feedback about the 'suggests' listed in the spec file on IRC (as mentioned in comment #9). But I didn't get replies on IRC. So I'll try here again to check if it's sane to do that.

The spec file: https://github.com/AquaL1te/neofetch/blob/master/neofetch.spec
Koji scratch build + SRPMS: https://github.com/AquaL1te/neofetch/blob/master/neofetch.spec

Let me know if this is sufficient, I'm eager to see this in the repositories. Thanks!

Comment 11 Kees de Jong 2017-02-03 07:22:12 UTC
(In reply to Kees de Jong from comment #10)
> I've tried a few times to get some feedback about the 'suggests' listed in
> the spec file on IRC (as mentioned in comment #9). But I didn't get replies
> on IRC. So I'll try here again to check if it's sane to do that.
> 
> The spec file: https://github.com/AquaL1te/neofetch/blob/master/neofetch.spec
> Koji scratch build + SRPMS:
> https://github.com/AquaL1te/neofetch/blob/master/neofetch.spec
> 
> Let me know if this is sufficient, I'm eager to see this in the
> repositories. Thanks!

Sorry, a copy/paste screw up, here is the Koji scratch build link mentioned in comment #10: https://koji.fedoraproject.org/koji/taskinfo?taskID=17399344

Comment 12 Loic Dachary 2017-02-14 18:13:54 UTC
This looks like a fine package to me :-) The only thing that came to mind is that there are too many empty lines.

Comment 13 Nemanja Milosevic 2017-02-16 17:27:24 UTC
Informal review (new packager, bare with me):

I also like this spec file, but I have to agree with Loic, the blank lines are making it uglier.

One other suggestion:

Currently you have this:

URL:            https://github.com/dylanaraps/%{name}/tree/%{version}
Source0:        https://github.com/dylanaraps/%{name}/archive/%{version}.tar.gz

I'm unsure if URL should be version independent. If it should be you could fix this up a bit like so:

URL:            https://github.com/dylanaraps/%{name}
Source0:        %{url}/archive/%{version}.tar.gz

Just a suggestion, great work otherwise. :)

Comment 14 Kees de Jong 2017-02-19 16:24:00 UTC
(In reply to Nemanja Milosevic from comment #13)
> Informal review (new packager, bare with me):
> 
> I also like this spec file, but I have to agree with Loic, the blank lines
> are making it uglier.
> 
> One other suggestion:
> 
> Currently you have this:
> 
> URL:            https://github.com/dylanaraps/%{name}/tree/%{version}
> Source0:       
> https://github.com/dylanaraps/%{name}/archive/%{version}.tar.gz
> 
> I'm unsure if URL should be version independent. If it should be you could
> fix this up a bit like so:
> 
> URL:            https://github.com/dylanaraps/%{name}
> Source0:        %{url}/archive/%{version}.tar.gz
> 
> Just a suggestion, great work otherwise. :)

Thanks for your review! I will change the URL, the version is indeed not really relevant.

(In reply to Loic Dachary from comment #12)
> This looks like a fine package to me :-) The only thing that came to mind is
> that there are too many empty lines.

Yes I agree, I kept the blank lines as is from the `rpmdev-newspec` template. I thought it was some sort of standard layout. I will change this as well!

Comment 15 Kees de Jong 2017-02-24 08:06:52 UTC
Did a few package reviews myself: #1421245 and #1419032 (I will do more soon)
I think this package is ready to be sponsored :-)

Comment 16 Kees de Jong 2017-02-24 10:31:31 UTC
Just as a final note, my FAS account is: keesdejong
This is where the spec file can be found: https://github.com/AquaL1te/neofetch
This is a Koji scratch build of the latest commit: https://koji.fedoraproject.org/koji/taskinfo?taskID=18028522

Comment 17 Nemanja Milosevic 2017-02-24 10:55:46 UTC
Looks cleaner now! Good job. :)

About the:

Source0:        https://github.com/dylanaraps/%{name}/archive/%{version}.tar.gz

In the future you can also use something like this:

Source0:        https://github.com/dylanaraps/%{name}/archive/%{version}.tar.gz#/%{name}-%{version}.tar.gz

It works the same, but when you use spectool to download sources you don't end up with '3.0.tar.gz', you get 'neofetch-3.0.tar.gz' which is better when you have a lot of packages in your tree.

Cheers!

Comment 18 Kees de Jong 2017-04-21 07:56:08 UTC
Bumped to version 3.0.1.
Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=19122579
Source: https://github.com/AquaL1te/neofetch/blob/master/neofetch.spec
FAS username: keesdejong

Comment 19 Ben Rosser 2017-06-30 09:57:50 UTC
I quickly glanced over the spec and didn't see any obvious problems, but could you please provide a link to the latest SRPM as well, and also point at a plaintext version of the spec (i.e. https://raw.githubusercontent.com/AquaL1te/neofetch/master/neofetch.spec)?

This will make it easier to run the automated fedora-review tool over this package.

Comment 20 Kees de Jong 2017-06-30 10:43:23 UTC
(In reply to Ben Rosser from comment #19)
> I quickly glanced over the spec and didn't see any obvious problems, but
> could you please provide a link to the latest SRPM as well, and also point
> at a plaintext version of the spec (i.e.
> https://raw.githubusercontent.com/AquaL1te/neofetch/master/neofetch.spec)?
> 
> This will make it easier to run the automated fedora-review tool over this
> package.

Thank you.

Bumped the version of Neofetch to 3.2.0 (latest as of now).
Spec: https://raw.githubusercontent.com/AquaL1te/neofetch/master/neofetch.spec
Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=20262006
SRPM: https://kojipkgs.fedoraproject.org//work/tasks/2006/20262006/neofetch-3.2.0-1.fc26.src.rpm
FAS username: keesdejong

Looking for a sponsor.

Comment 21 Kees de Jong 2017-06-30 12:05:08 UTC
Just ran the fedora-review tool myself and fixed these minor errors already:
neofetch.src: W: name-repeated-in-summary C Neofetch
neofetch.src: W: spelling-error %description -l en_US ascii -> ASCII
neofetch.src: W: spelling-error %description -l en_US distro -> bistro, district

Spec: https://raw.githubusercontent.com/AquaL1te/neofetch/master/neofetch.spec
Spec diff: https://github.com/AquaL1te/neofetch/commit/9eeb72eaaae1be8824f2d694b93e49fdc5beafbf
Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=20262958
SRPM: https://kojipkgs.fedoraproject.org//work/tasks/2959/20262959/neofetch-3.2.0-1.fc26.src.rpm
FAS username: keesdejong

Looking for a sponsor.

Comment 22 Ben Rosser 2017-06-30 12:56:12 UTC
In general the package looks very good! I have one blocking issue and a couple
of comments/pointers.

I am not a sponsor, but I can review the package, at which point you can file
a ticket at https://pagure.io/packager-sponsors and ask for a sponsor. A sponsor
will almost certainly ask you to do some reviews of other packages though before
actually sponsoring you.

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed

Issues
======

- As per https://fedoraproject.org/wiki/Packaging:Guidelines#Configuration_files,
you must mark any configuration files with %config or %config(noreplace). I see
that there's an /etc/neofetch/config installed by the package; this should
be marked as configuration. (Probably %config(noreplace)).

Comments (non-blocking)
=======================

- Your justification for using Recommends is fine but outdated; modern dnf
_does_ install Recommends these days (but allows their removal without removing
the main package, and also this behavior can be configured).

- It certainly doesn't hurt, but you probably don't strictly need to condition
on bash >= 3. Even RHEL5 has a bash at least this new. :)

- The "wrong-script-interpreter" rpmlint messages are complaining because of
the use of #!/usr/bin/env bash rather than just #!/bin/bash. Someting like
this in %prep will fix this:

sed 's,/usr/bin/env bash,%{_bindir}/bash,g' -i neofetch
sed 's,/usr/bin/env bash,%{_bindir}/bash,g' -i config/config

- Does the config file need a shebang? I assume it doesn't, since it appears
neofetch simply sources the config to load it
(https://github.com/dylanaraps/neofetch/blob/master/neofetch#L3604). If you
remove it in %prep you can eliminate the other rpmlint error too.

===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "Unknown or generated". 131 files have unknown license.
     Detailed output of licensecheck in
     /home/bjr/Programming/fedora/reviews/1411984-neofetch/licensecheck.txt
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 10240 bytes in 2 files.
[!]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[x]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: neofetch-3.2.0-1.fc27.noarch.rpm
          neofetch-3.2.0-1.fc27.src.rpm
neofetch.noarch: E: wrong-script-interpreter /usr/bin/neofetch /usr/bin/env bash
neofetch.noarch: W: non-conffile-in-etc /etc/neofetch/config
neofetch.noarch: E: wrong-script-interpreter /etc/neofetch/config /usr/bin/env bash
neofetch.noarch: E: non-executable-script /etc/neofetch/config 644 /usr/bin/env bash
2 packages and 0 specfiles checked; 3 errors, 1 warnings.




Rpmlint (installed packages)
----------------------------
sh: /usr/bin/python: No such file or directory
neofetch.noarch: W: non-conffile-in-etc /etc/neofetch/config
neofetch.noarch: E: wrong-script-interpreter /etc/neofetch/config /usr/bin/env bash
neofetch.noarch: E: non-executable-script /etc/neofetch/config 644 /usr/bin/env bash
neofetch.noarch: E: wrong-script-interpreter /usr/bin/neofetch /usr/bin/env bash
1 packages and 0 specfiles checked; 3 errors, 1 warnings.



Requires
--------
neofetch (rpmlib, GLIBC filtered):
    /usr/bin/env
    bash



Provides
--------
neofetch:
    neofetch



Source checksums
----------------
https://github.com/dylanaraps/neofetch/archive/3.2.0.tar.gz#/neofetch-3.2.0.tar.gz :
  CHECKSUM(SHA256) this package     : 6aecd51c165a36692b4f6481b3071ab936aafc3fccffabbbfda140567f16431d
  CHECKSUM(SHA256) upstream package : 6aecd51c165a36692b4f6481b3071ab936aafc3fccffabbbfda140567f16431d


Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02
Command line :/usr/bin/fedora-review -b 1411984 -m fedora-rawhide-x86_64
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api
Disabled plugins: Java, C/C++, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP
Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6

Comment 23 Robert-André Mauchin 🐧 2017-10-04 17:26:21 UTC
@ Kees de Jong

I'd like this package to move forward, are you still interested in packaging it?

Comment 24 Kees de Jong 2017-10-04 19:39:18 UTC
(In reply to Robert-André Mauchin from comment #23)
> @ Kees de Jong
> 
> I'd like this package to move forward, are you still interested in packaging
> it?

Hi Robert-Andre,


I still am, I got sponsored and the package is approved. But for some reason I can't get access to my git repo, which is needed to commit the spec and push the build.

$ fedrepo-req neofetch -t 1411984
Error: The Bugzilla bug's review was approved over 60 days ago

^ The above was after 2 weeks the pagure ticket was approved. So I then thought, let's just continue to the next step.


$ kinit keesdejong@FEDORAPROJECT.ORG          
Password for keesdejong@FEDORAPROJECT.ORG:                          
$ fedpkg clone neofetch                                                                                                                                                                                                                                     
Cloning into 'neofetch'...                                          
FATAL: R any rpms/neofetch keesdejong DENIED by fallthru            
(or you mis-spelled the reponame)                                   
fatal: Could not read from remote repository.                       
                                                                    
Please make sure you have the correct access rights                 
and the repository exists.                                          
Could not execute clone: Command '['git', 'clone', 'ssh://keesdejong@pkgs.fedoraproject.org/rpms/neofetch', '--origin', 'origin']' returned non-zero exit status 128


I asked a few times on IRC on what to do, but no one replied. So it slowly went down my priorities. I'll contact my sponsor this weekend to find out what I'm missing. Or maybe you know more about this? In any case, expect an update in about a week.

Comment 25 Rex Dieter 2017-10-04 19:48:06 UTC
The package has not yet passed review, comment #22 includes:

"- As per https://fedoraproject.org/wiki/Packaging:Guidelines#Configuration_files,
you must mark any configuration files with %config or %config(noreplace). I see
that there's an /etc/neofetch/config installed by the package; this should
be marked as configuration. (Probably %config(noreplace))."

Fix that first, and the reviewer will likely approve it then

Comment 26 Kees de Jong 2017-10-08 20:52:23 UTC
Mock build: https://koji.fedoraproject.org/koji/taskinfo?taskID=22339006
Spec file: https://raw.githubusercontent.com/AquaL1te/neofetch/master/neofetch.spec
SRPM: https://kojipkgs.fedoraproject.org//work/tasks/9007/22339007/neofetch-3.3.0-1.fc28.src.rpm


Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



===== MUST items =====

Generic:
[ ]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[ ]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "MIT/X11 (BSD like)", "Unknown or generated". 144 files have
     unknown license. Detailed output of licensecheck in
     /home/k.dejong/git/neofetch/review-neofetch/licensecheck.txt
[ ]: Package contains no bundled libraries without FPC exception.
[ ]: Changelog in prescribed format.
[ ]: Sources contain only permissible code or content.
[ ]: Package contains desktop file if it is a GUI application.
[ ]: Development files must be in a -devel package
[ ]: Package uses nothing in %doc for runtime.
[ ]: Package consistently uses macros (instead of hard-coded directory
     names).
[ ]: Package is named according to the Package Naming Guidelines.
[ ]: Package does not generate any conflict.
[ ]: Package obeys FHS, except libexecdir and /usr/target.
[ ]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[ ]: Requires correct, justified where necessary.
[ ]: Spec file is legible and written in American English.
[ ]: Package contains systemd file(s) if in need.
[ ]: Package is not known to require an ExcludeArch tag.
[ ]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 20480 bytes in 2 files.
[ ]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: No rpmlint messages.
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: %config files are marked noreplace or the reason is justified.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: No %config files under /usr.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[ ]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[ ]: Final provides and requires are sane (see attachments).
[ ]: Package functions as described.
[x]: Latest version is packaged.
[ ]: Package does not include license text files separate from upstream.
[ ]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[ ]: Package should compile and build into binary rpms on all supported
     architectures.
[ ]: %check is present and all tests pass.
[ ]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: neofetch-3.3.0-1.fc26.noarch.rpm
          neofetch-3.3.0-1.fc26.src.rpm
2 packages and 0 specfiles checked; 0 errors, 0 warnings.




Rpmlint (installed packages)
----------------------------
sh: /usr/bin/python: No such file or directory
neofetch.noarch: W: invalid-url URL: https://github.com/dylanaraps/neofetch <urlopen error [Errno -2] Name or service not known>
1 packages and 0 specfiles checked; 0 errors, 1 warnings.



Requires
--------
neofetch (rpmlib, GLIBC filtered):
    /usr/bin/bash
    bash
    config(neofetch)



Provides
--------
neofetch:
    config(neofetch)
    neofetch



Source checksums
----------------
https://github.com/dylanaraps/neofetch/archive/3.3.0.tar.gz#/neofetch-3.3.0.tar.gz :
  CHECKSUM(SHA256) this package     : 4808e76bd81da3602cb5be7e01dfed8223b1109e2792755dd0d54126014ee696
  CHECKSUM(SHA256) upstream package : 4808e76bd81da3602cb5be7e01dfed8223b1109e2792755dd0d54126014ee696


Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02
Command line :/usr/bin/fedora-review -n neofetch
Buildroot used: fedora-26-x86_64
Active plugins: Generic, Shell-api
Disabled plugins: Java, C/C++, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP
Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6

Comment 27 Robert-André Mauchin 🐧 2017-10-09 11:19:52 UTC
 - I don't think you should be using Recommends for optional dependencies. Just use normal Requires to give all fonctionalities to the user.

Following https://github.com/dylanaraps/neofetch/wiki/Dependencies, we need:

Requires:       w3m-img
Requires:       ImageMagick
Requires:       feh
Requires:       scrot
Requires:       curl
Requires:       coreutils
Requires:       xwininfo
Requires:       xprop
Requires:       xrandr
Requires:       bind-utils
Requires:       pciutils

The gawk dependencies is only for iOS. xdotool is not necessary, the function is already covered by xwininfo + xprop or xwininfo + xdpyinfo provided by xorg-x11-utils

 + Use a simplified Source0:

Source0:        https://github.com/dylanaraps/%{name}/archive/%{version}/%{name}-%{version}.tar.gz

Comment 28 Kees de Jong 2017-10-18 15:03:12 UTC
(In reply to Robert-André Mauchin from comment #27)
>  - I don't think you should be using Recommends for optional dependencies.
> Just use normal Requires to give all fonctionalities to the user.
> 
> Following https://github.com/dylanaraps/neofetch/wiki/Dependencies, we need:
> 
> Requires:       w3m-img
> Requires:       ImageMagick
> Requires:       feh
> Requires:       scrot
> Requires:       curl
> Requires:       coreutils
> Requires:       xwininfo
> Requires:       xprop
> Requires:       xrandr
> Requires:       bind-utils
> Requires:       pciutils
> 
> The gawk dependencies is only for iOS. xdotool is not necessary, the
> function is already covered by xwininfo + xprop or xwininfo + xdpyinfo
> provided by xorg-x11-utils
> 
>  + Use a simplified Source0:
> 
> Source0:       
> https://github.com/dylanaraps/%{name}/archive/%{version}/%{name}-%{version}.
> tar.gz


Those are listed as optional dependencies. The reason I choose weak dependencies is because this is package will mostly will be used in a terminal. Some other functionality need the optional dependencies. So if you're running a bare install i.e. a server, then you won't be in favor (I guess) to install all these extra stuff, you won't need on a system without a GUI.





An example on a fresh install of Fedora Server:
# dnf install /tmp/neofetch-3.3.0-1.fc26.noarch.rpm
Last metadata expiration check: 0:09:19 ago on wo 18 okt 2017 16:40:43 CEST.
Dependencies resolved.
======================================================================================================================================================================================
 Package                                          Arch                             Version                                               Repository                              Size
======================================================================================================================================================================================
Installing:
 neofetch                                         noarch                           3.3.0-1.fc26                                          @commandline                            95 k
Installing dependencies:
 ImageMagick-libs                                 x86_64                           6.9.9.15-1.fc26                                       updates                                2.2 M
 OpenEXR-libs                                     x86_64                           2.2.0-6.fc26                                          fedora                                 628 k
 compat-openssl10                                 x86_64                           1:1.0.2j-9.fc26                                       updates                                1.1 M
 fftw-libs-double                                 x86_64                           3.3.5-4.fc26                                          fedora                                 980 k
 gdk-pixbuf2-xlib                                 x86_64                           2.36.9-1.fc26                                         updates                                 50 k
 ghostscript-core                                 x86_64                           9.20-10.fc26                                          fedora                                 4.5 M
 ghostscript-fonts                                noarch                           5.50-36.fc26                                          fedora                                 328 k
 gpm-libs                                         x86_64                           1.20.7-10.fc26                                        fedora                                  36 k
 graphite2                                        x86_64                           1.3.10-1.fc26                                         fedora                                 117 k
 harfbuzz                                         x86_64                           1.4.4-1.fc26                                          fedora                                 253 k
 ilmbase                                          x86_64                           2.2.0-8.fc26                                          fedora                                 104 k
 jbigkit-libs                                     x86_64                           2.1-6.fc26                                            fedora                                  51 k
 lcms2                                            x86_64                           2.8-3.fc26                                            fedora                                 158 k
 libICE                                           x86_64                           1.0.9-9.fc26                                          fedora                                  70 k
 libSM                                            x86_64                           1.2.2-5.fc26                                          fedora                                  42 k
 libXcomposite                                    x86_64                           0.4.4-9.fc26                                          fedora                                  26 k
 libXcursor                                       x86_64                           1.1.14-8.fc26                                         fedora                                  33 k
 libXfont                                         x86_64                           1.5.2-2.fc26                                          fedora                                 154 k
 libXft                                           x86_64                           2.3.2-5.fc26                                          fedora                                  63 k
 libXi                                            x86_64                           1.7.9-2.fc26                                          fedora                                  44 k
 libXinerama                                      x86_64                           1.1.3-7.fc26                                          fedora                                  17 k
 libXmu                                           x86_64                           1.1.2-5.fc26                                          fedora                                  74 k
 libXrandr                                        x86_64                           1.5.1-2.fc26                                          fedora                                  30 k
 libXt                                            x86_64                           1.1.5-4.fc26                                          fedora                                 179 k
 libXtst                                          x86_64                           1.2.3-2.fc26                                          fedora                                  24 k
 libXv                                            x86_64                           1.0.11-2.fc26                                         fedora                                  21 k
 libXxf86dga                                      x86_64                           1.1.4-7.fc26                                          fedora                                  23 k
 libXxf86misc                                     x86_64                           1.0.3-12.fc26                                         fedora                                  23 k
 libdatrie                                        x86_64                           0.2.9-4.fc26                                          fedora                                  30 k
 libdmx                                           x86_64                           1.1.3-7.fc26                                          fedora                                  19 k
 libfontenc                                       x86_64                           1.1.3-4.fc26                                          fedora                                  34 k
 libjpeg-turbo                                    x86_64                           1.5.1-0.fc26                                          fedora                                 153 k
 libmcpp                                          x86_64                           2.7.2-17.fc26                                         fedora                                  78 k
 librsvg2                                         x86_64                           2.40.18-1.fc26                                        updates                                134 k
 libthai                                          x86_64                           0.1.25-2.fc26                                         fedora                                 198 k
 libtiff                                          x86_64                           4.0.8-1.fc26                                          fedora                                 179 k
 libwebp                                          x86_64                           0.6.0-2.fc26                                          fedora                                 259 k
 libwmf-lite                                      x86_64                           0.2.8.4-53.fc26                                       updates                                 72 k
 libxdo                                           x86_64                           1:3.20150503.1-3.fc26                                 fedora                                  39 k
 mcpp                                             x86_64                           2.7.2-17.fc26                                         fedora                                  29 k
 openjpeg2                                        x86_64                           2.2.0-3.fc26                                          updates                                140 k
 pango                                            x86_64                           1.40.12-1.fc26                                        updates                                288 k
 perl                                             x86_64                           4:5.24.3-395.fc26                                     updates                                6.1 M
 perl-Carp                                        noarch                           1.40-366.fc26                                         fedora                                  28 k
 perl-Errno                                       x86_64                           1.25-395.fc26                                         updates                                 69 k
 perl-Exporter                                    noarch                           5.72-367.fc26                                         fedora                                  32 k
 perl-File-Path                                   noarch                           2.12-367.fc26                                         fedora                                  34 k
 perl-IO                                          x86_64                           1.36-395.fc26                                         updates                                134 k
 perl-NKF                                         x86_64                           1:2.1.4-4.fc26                                        fedora                                 135 k
 perl-PathTools                                   x86_64                           3.63-367.fc26                                         fedora                                  87 k
 perl-Scalar-List-Utils                           x86_64                           3:1.48-1.fc26                                         updates                                 65 k
 perl-Socket                                      x86_64                           4:2.024-2.fc26                                        fedora                                  55 k
 perl-Text-Tabs+Wrap                              noarch                           2013.0523-366.fc26                                    fedora                                  22 k
 perl-Unicode-Normalize                           x86_64                           1.25-366.fc26                                         fedora                                  79 k
 perl-constant                                    noarch                           1.33-368.fc26                                         fedora                                  23 k
 perl-libs                                        x86_64                           4:5.24.3-395.fc26                                     updates                                1.5 M
 perl-macros                                      x86_64                           4:5.24.3-395.fc26                                     updates                                 65 k
 perl-parent                                      noarch                           1:0.236-2.fc26                                        fedora                                  18 k
 perl-threads                                     x86_64                           1:2.16-1.fc26                                         fedora                                  58 k
 perl-threads-shared                              x86_64                           1.57-1.fc26                                           fedora                                  45 k
 poppler-data                                     noarch                           0.4.7-7.fc26                                          fedora                                 2.2 M
 urw-fonts                                        noarch                           3:2.4-23.fc26                                         fedora                                 3.0 M
 w3m                                              x86_64                           0.5.3-31.git20170102.fc26                             fedora                                 1.0 M
 xorg-x11-font-utils                              x86_64                           1:7.5-33.fc26                                         fedora                                  81 k
Installing weak dependencies:
 ImageMagick                                      x86_64                           6.9.9.15-1.fc26                                       updates                                181 k
 w3m-img                                          x86_64                           0.5.3-31.git20170102.fc26                             fedora                                  37 k
 xdotool                                          x86_64                           1:3.20150503.1-3.fc26                                 fedora                                  52 k
 xorg-x11-server-utils                            x86_64                           7.7-21.fc26                                           fedora                                 184 k
 xorg-x11-utils                                   x86_64                           7.5-23.fc26                                           updates                                118 k

Transaction Summary
======================================================================================================================================================================================
Install  70 Packages

Total size: 28 M
Total download size: 28 M
Installed size: 87 M
Is this ok [y/N]:




And now the same package, but then without the weak dependencies:
# dnf --setopt=install_weak_deps=False --best install /tmp/neofetch-3.3.0-1.fc26.noarch.rpm
Last metadata expiration check: 0:14:00 ago on wo 18 okt 2017 16:40:43 CEST.
Dependencies resolved.
======================================================================================================================================================================================
 Package                                   Arch                                    Version                                        Repository                                     Size
======================================================================================================================================================================================
Installing:
 neofetch                                  noarch                                  3.3.0-1.fc26                                   @commandline                                   95 k

Transaction Summary
======================================================================================================================================================================================
Install  1 Package

Total size: 95 k
Installed size: 255 k
Is this ok [y/N]: 




I will contact the developer to verify this, but from what I've seen by using this package, those optional dependencies are mostly used for 'power users'. Which won't apply to a server without a desktop environment.

Comment 29 Ben Rosser 2017-10-24 03:20:04 UTC
I apologize, I'd forgotten I'd taken this review!

I think the use of weak dependencies is perfectly reasonable here to separate libraries not required in a headless environment.

I'll just double-check that everything is good and then hopefully approve the package.

Comment 30 Ben Rosser 2017-10-24 03:25:18 UTC
Alright, package looks fine and is APPROVED.

Again, really sorry for the delay.

Comment 31 Kees de Jong 2017-10-24 08:33:24 UTC
(In reply to Ben Rosser from comment #30)
> Alright, package looks fine and is APPROVED.
> 
> Again, really sorry for the delay.

No worries, I am super busy anyway, thank you! I will close the 'bug' once the package is submitted to the build and accepted.

Comment 32 Gwyn Ciesla 2017-10-24 14:14:08 UTC
(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/neofetch

Comment 33 Kees de Jong 2017-10-26 14:34:49 UTC
Build successful: https://koji.fedoraproject.org/koji/taskinfo?taskID=22707330

Comment 34 Fedora Update System 2017-12-04 20:52:01 UTC
neofetch-3.3.0-1.fc27 has been submitted as an update to Fedora 27. https://bodhi.fedoraproject.org/updates/FEDORA-2017-b9c158f42a

Comment 35 Fedora Update System 2017-12-10 00:31:05 UTC
neofetch-3.3.0-1.fc27 has been pushed to the Fedora 27 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-b9c158f42a

Comment 36 Fedora Update System 2017-12-17 19:48:25 UTC
neofetch-3.3.0-1.fc27 has been pushed to the Fedora 27 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.