Bug 1372123 - Review Request: pacmanager - Perl Auto Connector, a multi-purpose SSH/terminal connection manager
Summary: Review Request: pacmanager - Perl Auto Connector, a multi-purpose SSH/termina...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Rex Dieter
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-NEEDSPONSOR
TreeView+ depends on / blocked
 
Reported: 2016-09-01 00:58 UTC by Mike Goodwin
Modified: 2016-12-28 15:30 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-12-28 15:30:00 UTC
Type: ---
Embargoed:
rdieter: fedora-review+


Attachments (Terms of Use)

Description Mike Goodwin 2016-09-01 00:58:35 UTC
*FIRST PACKAGE*

Spec URL: https://github.com/xenithorb/pacmanager-spec/blob/master/SPECS/pacmanager.spec
SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/8035/15458035/pacmanager-4.5.5.7-1.fc24.src.rpm
RPM URL:  https://kojipkgs.fedoraproject.org//work/tasks/8035/15458035/pacmanager-4.5.5.7-1.fc24.noarch.rpm
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

Comment 1 Mike Goodwin 2016-09-02 18:21:10 UTC
*UPDATED INFO*

- Fixed missing hicolor app icons 
- Added icon-updating scriptlets 

SRPM: https://kojipkgs.fedoraproject.org//work/tasks/3213/15473213/pacmanager-4.5.5.7-2.fc24.src.rpm
RPM: https://kojipkgs.fedoraproject.org//work/tasks/3213/15473213/pacmanager-4.5.5.7-2.fc24.noarch.rpm

The rest of links remain the same.

Comment 2 Rex Dieter 2016-09-14 16:29:26 UTC
I'll try to help review this in the next day or 2

Comment 3 Rex Dieter 2016-09-14 16:59:21 UTC
$ 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#desktop-file-install_usage

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-latest.html#definitions
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

Comment 4 Mike Goodwin 2016-09-14 19:46:35 UTC
SPEC diff: https://github.com/xenithorb/pacmanager-spec/commit/92f266138579f580e5444f005758209e9cefb670

SRPM Update: https://copr-be.cloud.fedoraproject.org/results/xenithorb/pacmanager/fedora-24-x86_64/00453000-pacmanager/pacmanager-4.5.5.7-3.fc24.src.rpm

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.

Comment 5 Rex Dieter 2016-09-26 17:49:10 UTC
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.

Comment 6 Gwyn Ciesla 2016-09-27 21:28:04 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/pacmanager


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