[Bug 1411984] neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984 Ben Rosser changed: What|Removed |Added Flags|fedora-review? |fedora-review+ --- Comment #30 from Ben Rosser --- Alright, package looks fine and is APPROVED. Again, really sorry for the delay. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984 --- Comment #29 from Ben Rosser --- 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984
--- Comment #28 from Kees de Jong ---
(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-fontsnoarch
5.50-36.fc26 fedora
328 k
gpm-libs x86_64
1.20.7-10.fc26fedora
36 k
graphite2x86_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.fc26fedora
51 k
lcms2x86_64
2.8-3.fc26fedora
158 k
libICE x86_64
1.0.9-9.fc26 fedora
70 k
libSMx86_64
1.2.2-5.fc26
[Bug 1411984] neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984
--- Comment #27 from Robert-André Mauchin ---
- 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
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984
--- Comment #26 from Kees de Jong ---
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
[Bug 1411984] neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984 Rex Dieter changed: What|Removed |Added Summary|Neofetch - a CLI system |neofetch - a CLI system |information tool written in |information tool written in |Bash|Bash -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984 Rex Dieter changed: What|Removed |Added Version|25 |rawhide -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984 --- Comment #25 from Rex Dieter --- 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 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984 --- Comment #24 from Kees de Jong --- (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 keesdej...@fedoraproject.org Password for keesdej...@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://keesdej...@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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984 Robert-André Mauchin changed: What|Removed |Added CC||zebo...@gmail.com Blocks|177841 (FE-NEEDSPONSOR) | --- Comment #23 from Robert-André Mauchin --- @ Kees de Jong I'd like this package to move forward, are you still interested in packaging it? Referenced Bugs: https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984
Ben Rosser changed:
What|Removed |Added
Assignee|nob...@fedoraproject.org|rosser@gmail.com
Flags||fedora-review?
--- Comment #22 from Ben Rosser ---
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=... does
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984 --- Comment #21 from Kees de Jong --- 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984 --- Comment #20 from Kees de Jong --- (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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984 Ben Rosser changed: What|Removed |Added CC||rosser@gmail.com --- Comment #19 from Ben Rosser --- 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984 --- Comment #18 from Kees de Jong --- 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 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984
--- Comment #17 from Nemanja Milosevic ---
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!
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984 --- Comment #16 from Kees de Jong --- 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 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984 --- Comment #15 from Kees de Jong --- Did a few package reviews myself: #1421245 and #1419032 (I will do more soon) I think this package is ready to be sponsored :-) -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984
--- Comment #14 from Kees de Jong ---
(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!
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984
Nemanja Milosevic changed:
What|Removed |Added
CC||nmilo...@fedoraproject.org
--- Comment #13 from Nemanja Milosevic ---
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. :)
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984 Loic Dachary changed: What|Removed |Added CC||l...@dachary.org --- Comment #12 from Loic Dachary --- This looks like a fine package to me :-) The only thing that came to mind is that there are too many empty lines. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984 --- Comment #11 from Kees de Jong --- (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 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984 --- Comment #10 from Kees de Jong --- 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! -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984 --- Comment #9 from Kees de Jong --- (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 :-) -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984 Dylan Araps changed: What|Removed |Added CC||dylan.ar...@gmail.com --- Comment #8 from Dylan Araps --- 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! :) -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984 --- Comment #7 from M. Herdiansyah --- 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984 --- Comment #6 from M. Herdiansyah --- (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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984
--- Comment #5 from Michael Schwendt ---
> 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*
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984
--- Comment #4 from Kees de Jong ---
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
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984
herdians...@openmailbox.org changed:
What|Removed |Added
CC||herdians...@openmailbox.org
--- Comment #3 from herdians...@openmailbox.org ---
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}/*
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984
--- Comment #2 from Michael Schwendt ---
> %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.
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
[Bug 1411984] Neofetch - a CLI system information tool written in Bash
https://bugzilla.redhat.com/show_bug.cgi?id=1411984
Mikolaj Izdebski changed:
What|Removed |Added
Blocks||177841 (FE-NEEDSPONSOR)
--- Comment #1 from Mikolaj Izdebski ---
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.
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841
[Bug 177841] Tracker: Review requests from new Fedora packagers who need a
sponsor
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
