https://bugzilla.redhat.com/show_bug.cgi?id=1372123
Bug ID: 1372123 Summary: Review Request: pacmanager - Perl Auto Connector, a multi-purpose SSH/terminal connection manager Product: Fedora Version: rawhide Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: mike@mgoodwin.net QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
*FIRST PACKAGE*
Spec URL: https://github.com/xenithorb/pacmanager-spec/blob/master/SPECS/pacmanager.sp... SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/8035/15458035/pacmanager-4.5.... RPM URL: https://kojipkgs.fedoraproject.org//work/tasks/8035/15458035/pacmanager-4.5.... COPR URL: https://copr.fedorainfracloud.org/coprs/xenithorb/pacmanager/
Description:
PAC is a telnet/ssh/rsh/etc connection manager/automator written in Perl GTK aimed at making administration easier. Users who may have used SecureCRT, PuTTY, and/or mRemoteNG in the past may find this application useful.
Fedora Account System Username: xenithorb
https://bugzilla.redhat.com/show_bug.cgi?id=1372123
Mike Goodwin mike@mgoodwin.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |mike@mgoodwin.net Blocks| |177841 (FE-NEEDSPONSOR)
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor
https://bugzilla.redhat.com/show_bug.cgi?id=1372123
--- Comment #1 from Mike Goodwin mike@mgoodwin.net --- *UPDATED INFO*
- Fixed missing hicolor app icons - Added icon-updating scriptlets
SRPM: https://kojipkgs.fedoraproject.org//work/tasks/3213/15473213/pacmanager-4.5.... RPM: https://kojipkgs.fedoraproject.org//work/tasks/3213/15473213/pacmanager-4.5....
The rest of links remain the same.
https://bugzilla.redhat.com/show_bug.cgi?id=1372123
Rex Dieter rdieter@math.unl.edu changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|nobody@fedoraproject.org |rdieter@math.unl.edu Flags| |fedora-review?
--- Comment #2 from Rex Dieter rdieter@math.unl.edu --- I'll try to help review this in the next day or 2
https://bugzilla.redhat.com/show_bug.cgi?id=1372123
--- Comment #3 from Rex Dieter rdieter@math.unl.edu --- $ rpmlint pacmanager pacmanager.noarch: W: spelling-error Summary(en_US) multi -> mulch, mufti pacmanager.noarch: W: spelling-error %description -l en_US rsh -> rah, rs, sh pacmanager.noarch: W: spelling-error %description -l en_US automator -> automaton, automate, automatism pacmanager.noarch: W: spelling-error %description -l en_US mRemoteNG -> remoteness pacmanager.noarch: W: incoherent-version-in-changelog 4.5.5.7 ['4.5.5.7-2.fc24', '4.5.5.7-2'] pacmanager.noarch: E: standard-dir-owned-by-package /usr/share/man pacmanager.noarch: E: standard-dir-owned-by-package /usr/share/icons pacmanager.noarch: W: non-conffile-in-etc /etc/bash_completion.d/pacmanager pacmanager.noarch: E: standard-dir-owned-by-package /usr/share/man/man1 pacmanager.noarch: W: no-manual-page-for-binary pacmanager_from_putty pacmanager.noarch: W: no-manual-page-for-binary pacmanager_from_mcm 1 packages and 0 specfiles checked; 3 errors, 8 warnings.
So, the biggest issue here is the overuse of globs in %files:
%doc %{_mandir}/man1/%{name}* %{_datadir}/* %{_sysconfdir}/* %{_bindir}/*
1. MUST: %files need to get a little more specific here, and own only the stuff needed. I'd suggest:
%files %doc README %license LICENSE %{_mandir}/man1/pacmanager* %{_datadir}/pacmanager/ %{_datadir}/applications/pacmanager.desktop %{_datadir}/icons/hicolor/*/apps/pacmanager.* %{_sysconfdir}/bash_completion.d/pacmanager %{_bindir}/pacmanager*
that should address most of the important rpmlint issues wrt %files
pkg naming: ok
license: ok
macros: ok
scriptlets: ok
sources: ok 2e27507401adc2d7d9a7631d79a7a45f pac-4.5.5.7-all.tar.gz
2. MUST include .desktop file validation, see: https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#...
either use desktop-file-install to install it (instead of manual copy), or do a desktop-file-validate in %check section.
3. MUST app icons. I see one of the icons you use/install is jpg. that type is not supported for icon themes, see also: https://specifications.freedesktop.org/icon-theme-spec/icon-theme-spec-lates... Either omit that one, or convert it to png.
4. bash completion files SHOULD go under /usr/share/bash-completion/completions ie, BuildRequires: pkgconfig(bash-completion) and install/use the output from pkg-config --variable=completionsdir bash-completion (we can fix this collaboratively after review, if this is in anyway unclear)
Please fix the 3 MUST items, and we should have a winner
https://bugzilla.redhat.com/show_bug.cgi?id=1372123
--- Comment #4 from Mike Goodwin mike@mgoodwin.net --- SPEC diff: https://github.com/xenithorb/pacmanager-spec/commit/92f266138579f580e5444f00...
SRPM Update: https://copr-be.cloud.fedoraproject.org/results/xenithorb/pacmanager/fedora-...
I decided to just remove the offending icon because it had drop shadows already and it was nearly impossible to get it to look good with convert.
https://bugzilla.redhat.com/show_bug.cgi?id=1372123
Rex Dieter rdieter@math.unl.edu changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+
--- Comment #5 from Rex Dieter rdieter@math.unl.edu --- non-blocker nit-picks (please fix before submitting any official builds):
5. .spec update lacked %changelog entry
6. %files includes: %{_datadir}/%{name}/* which still leaves the parent dir unowned, better: %{_datadir}/%{name}/
7. consider not using %{name} macro in %files, it's fragile (ie, if the package name ever changed for any reason, the build would fail).
APPROVED.
https://bugzilla.redhat.com/show_bug.cgi?id=1372123
--- Comment #6 from Jon Ciesla limburgher@gmail.com --- Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/pacmanager
https://bugzilla.redhat.com/show_bug.cgi?id=1372123
Mike Goodwin mike@mgoodwin.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution|--- |ERRATA Last Closed| |2016-12-28 10:30:00
package-review@lists.fedoraproject.org