Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
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.