Bug 1243530

Spec URL: https://jgu.fedorapeople.org/winswitch.spec
SRPM URL: https://jgu.fedorapeople.org/winswitch-0.12.21-1.fc22.src.rpm

Current rpmlint output:

$rpmlint /var/lib/mock/fedora-22-x86_64/result/*.rpm
winswitch.noarch: E: explicit-lib-dependency nx-libs
winswitch.noarch: E: explicit-lib-dependency python-xlib

-> both are bogus errors

winswitch.noarch: W: non-conffile-in-etc /etc/winswitch/server_defaults.conf

-> Not sure why it thinks that's not a config file.

winswitch.noarch: E: script-without-shebang /usr/share/nautilus-python/extensions/nautilus_winswitch.py

-> Nautilus extensions aren't executed, so no shebang needed. See: 

https://projects-old.gnome.org/nautilus-python/documentation/html/nautilus-python-overview-example.html#id2684063



winswitch.noarch: W: non-conffile-in-etc /etc/winswitch/ports.conf

-> Again, seems bogus.

winswitch.src: W: spelling-error %description -l en_US xpra -> Praia

-> bogus

winswitch.src:65: E: hardcoded-library-path in %{_prefix}/lib/nautilus/extensions-2.0/python/*

-> Bogus - this is just in the script moving the extension to the right place

winswitch.src:68: E: hardcoded-library-path in %{_prefix}/lib/nautilus

-> Ditto

winswitch.src:3: W: mixed-use-of-spaces-and-tabs (spaces: line 3, tab: line 3)
2 packages and 0 specfiles checked; 5 errors, 4 warnings.

-> oops, ok, that needs fixing.

Spec URL: https://jgu.fedorapeople.org/winswitch.spec
SRPM URL: https://jgu.fedorapeople.org/winswitch-0.12.21-2.fc22.src.rpm

* Wed Jul 15 2015 Jonathan Underwood <jonathan.underwood> - 0.12.21-2
- Untabify spec file

> winswitch.noarch: E: script-without-shebang /usr/share/nautilus-python/extensions/nautilus_winswitch.py

Because the file is executable.

> winswitch.noarch: W: non-conffile-in-etc /etc/winswitch/server_defaults.conf
> W: non-conffile-in-etc /etc/winswitch/ports.conf

You need to mark it with %config(noreplace).

cicku?

Spec URL: http://cicku.me/winswitch.spec
SRPM URL: http://cicku.me/winswitch-0.12.21-3.fc20.src.rpm

* Tue Aug  4 2015 Jonathan Underwood <jonathan.underwood> - 0.12.21-3
- Remove Requires for nx-libs
- Add Requires for nxproxy
- Mark config files as %%config(noreplace)
- Remove executable bit from the nautilus plugin script

Spec URL: https://jgu.fedorapeople.org/winswitch.spec
SRPM URL: https://jgu.fedorapeople.org/winswitch-0.12.21-2.fc22.src.rpm

* Tue Aug  4 2015 Jonathan Underwood <jonathan.underwood> - 0.12.21-3
- Remove Requires for nx-libs
- Add Requires for nxproxy
- Mark config files as %%config(noreplace)
- Remove executable bit from the nautilus plugin script

Oops, cut and paste error with the URL in comment #5

Spec URL: https://jgu.fedorapeople.org/winswitch.spec
SRPM URL: https://jgu.fedorapeople.org/winswitch-0.12.21-4.fc22.src.rpm

* Tue Aug  4 2015 Jonathan Underwood <jonathan.underwood> - 0.12.21-4
- Add Requires for nxagent

* Tue Aug  4 2015 Jonathan Underwood <jonathan.underwood> - 0.12.21-3
- Remove Requires for nx-libs
- Add Requires for nxproxy
- Mark config files as %%config(noreplace)
- Remove executable bit from the nautilus plugin script

Not a blocker for review, but it would be nice to provide an appdata file [https://fedoraproject.org/wiki/Packaging:AppData].

Build now fails:
+ mkdir -p /builddir/build/BUILDROOT/winswitch-0.12.21-4.fc24.x86_64/usr/share/nautilus-python/extensions
+ for file in '/builddir/build/BUILDROOT/winswitch-0.12.21-4.fc24.x86_64/usr/lib/nautilus/extensions-2.0/python/*'
+ install -p -m 0644 '/builddir/build/BUILDROOT/winswitch-0.12.21-4.fc24.x86_64/usr/lib/nautilus/extensions-2.0/python/*' /builddir/build/BUILDROOT/winswitch-0.12.21-4.fc24.x86_64/usr/share/nautilus-python/extensions
install: cannot stat '/builddir/build/BUILDROOT/winswitch-0.12.21-4.fc24.x86_64/usr/lib/nautilus/extensions-2.0/python/*': No such file or directory

That's very weird - it builds locally for me under mock/f22. Will investigate.

Also builds fine locally under mock/rawhide. What's your build environment?

mock/rawhide. Dunno, might have been some transient error. It builds fine now.

Vash.jar — it'll have to be built from source. Most likely as a separate package.

Oh dear, that is a pain. I haven't actually managed to work out what vash is used  for - it's only called from the util/vash_util.py script. I wonder if it's actually needed.

OK, it's called from client/server_line_connection.py so it seems needed. I'll look into unbundling.

OK, have submitted a review request for java-vash. I'm completely unfamiliar with java though!

Spec URL: https://jgu.fedorapeople.org/winswitch.spec
SRPM URL: https://jgu.fedorapeople.org/winswitch-0.12.21-5.fc22.src.rpm

* Tue Aug  4 2015 Jonathan Underwood <jonathan.underwood> - 0.12.21-5
- Use system Vash.jar

May I ask for being a comaintainer of this? I missed the original review request since long time ago.

1. %{_datadir}/man/man1/* -> %{_mandir}/man1/*

2. https://winswitch.org/trac/ticket/212

Do we need to verify that what is the default remote service?

3. Lots of deps, based on upcoming RPM weak deps support, any enhancement on these?

(In reply to Christopher Meng from comment #18)
> May I ask for being a comaintainer of this? I missed the original review
> request since long time ago.
> 

Of course, co-maintainers always more than welcome!

> 1. %{_datadir}/man/man1/* -> %{_mandir}/man1/*
> 

OK.

> 2. https://winswitch.org/trac/ticket/212
> 

What about it? I don't plan to do anything different to upstream on this issue. disabling vbox listing will surprise some people.


> Do we need to verify that what is the default remote service?
> 

I don't see why? Care to explain in more detail?

> 3. Lots of deps, based on upcoming RPM weak deps support, any enhancement on
> these?

Yes, there's lots of deps, but I don't plan to move them to Recommends or Suggests unless winswitch gracefully handles the situation where they're missing. At the moment, as far as I can see, that doesn't appear to be the case.

Spec URL: https://jgu.fedorapeople.org/winswitch.spec
SRPM URL: https://jgu.fedorapeople.org/winswitch-0.12.21-6.fc22.src.rpm

* Tue Aug 11 2015 Jonathan Underwood <jonathan.underwood> - 0.12.21-6
- Use %%{_mandir} macro in file list

OK, some testing, and it seems that removing the various components supporting the different protocols is handled gracefully by winswitch, which simply shows them as not activated (and offers to install via dbus service). So, we can trim down the dependencies using weak deps. I see a few options:

(1) Have Requires for xpra and nx, and have the rest as Suggests.

(2) Having Requires for xpra and vnc, and Suggests for the rest.

(3) Having Requires for xpra, and Suggests for the rest.

(4) Having all protocols as Reccomends.

(5) Having all protocols as Suggests.

(6) Something else.


Views welcome.

Spec URL: https://jgu.fedorapeople.org/winswitch.spec
SRPM URL: https://jgu.fedorapeople.org/winswitch-0.12.21-7.fc22.src.rpm

* Wed Aug 12 2015 Jonathan Underwood <jonathan.underwood> - 0.12.21-7
- Move protocol dependencies to Suggests instead of Requires



There may yet be more scope for moving some deps to Recommends instead of Requires. For instance, I'm not really sure why devilspie is a requirement at present.

Spec URL: https://jgu.fedorapeople.org/winswitch.spec
SRPM URL: https://jgu.fedorapeople.org/winswitch-0.12.21-8.fc22.src.rpm


* Wed Aug 12 2015 Jonathan Underwood <jonathan.underwood> - 0.12.21-8
- Add Requires for python-netifaces

Spec URL: https://jgu.fedorapeople.org/winswitch.spec
SRPM URL: https://jgu.fedorapeople.org/winswitch-0.12.21-9.fc22.src.rpm

* Wed Aug 12 2015 Jonathan Underwood <jonathan.underwood> - 0.12.21-9
- Move Requires for devilspie to a Suggests for devilspie2 and patch
  source accordingly
- Move xloads image Requires to Suggests

Spec URL: https://jgu.fedorapeople.org/winswitch.spec
SRPM URL: https://jgu.fedorapeople.org/winswitch-0.12.21-10.fc22.src.rpm

* Wed Aug 12 2015 Jonathan Underwood <jonathan.underwood> - 0.12.21-10
- Move Requires for ImageMagick to Suggests
- Move Requires for java-vash to Suggests
- Move Requires for nautilus-python to Suggests
- Own nautilus extension directory rather than just the files under it

Spec URL: https://jgu.fedorapeople.org/winswitch.spec
SRPM URL: https://jgu.fedorapeople.org/winswitch-0.12.21-11.fc22.src.rpm

* Wed Aug 12 2015 Jonathan Underwood <jonathan.underwood> - 0.12.21-11
- Change xpra to be a Recommends dep

Spec URL: https://jgu.fedorapeople.org/winswitch.spec
SRPM URL: https://jgu.fedorapeople.org/winswitch-0.12.21-12.fc22.src.rpm


* Wed Aug 12 2015 Jonathan Underwood <jonathan.underwood> - 0.12.21-12
- Change Suggests xfreerdp to freerdp

OK, i think that's enough fiddling with weak deps. We can refine that in the fullness of time with feedback from users. I think this is ready for review now.

(In reply to Zbigniew Jędrzejewski-Szmek from comment #8)
> Not a blocker for review, but it would be nice to provide an appdata file
> [https://fedoraproject.org/wiki/Packaging:AppData].
It would be really nice to have an appdata file.

Unfortunately there's more bundling:

witch/net/pybonjour.py -- this is bundled http://code.google.com/p/pybonjour/. Fortunately the latest upstream version is bundled.

The code in winswitch/twisted also looks a bit suspicious. But those are snippets of code, not a separate thing. They also appear to be windows-specific. Please check if they can be removed in %prep.

winswitch/util/pam.py appears to be bundled python-pam. Fedora version is newer.

Other files look good.

Please own /usr/share/nautilus-python dir.

winswitch.noarch: E: executable-marked-as-config-file /etc/winswitch/firewall
Please remove the executable bit and the #!/bin/sh header.

Requires
--------
winswitch (rpmlib, GLIBC filtered):
    /bin/bash      <---- this one comes from /usr/libexec/winswitch/firewall, OK.
    /bin/sh        <---- this one comes from /etc/winswitch/firewall, so should go away when /etc/winswitch/firewall header is removed.
    /usr/bin/env   <---- this one comes from /usr/libexec/winswitch/*. Most likely those should be edited to use %{__python2}.
    /usr/bin/python2 <---- OK.

More work for you, sorry :)

Spec URL: https://jgu.fedorapeople.org/winswitch.spec
SRPM URL: https://jgu.fedorapeople.org/winswitch-0.12.21-15.fc22.src.rpm

* Thu Aug 13 2015 Jonathan Underwood <jonathan.underwood> - 0.12.21-15
- Replace /usr/bin/env python from files with %%{__python2}

* Thu Aug 13 2015 Jonathan Underwood <jonathan.underwood> - 0.12.21-14
- Remove shebang from top of /etc/winswitch/firewall file and fix
  perms to not include executable bit

* Thu Aug 13 2015 Jonathan Underwood <jonathan.underwood> - 0.12.21-13
- Add appdata file
- Remove all bonjour related files (only used on OSX)
- Remove bundled python pam module and use system wide version




(In reply to Zbigniew Jędrzejewski-Szmek from comment #29)
> (In reply to Zbigniew Jędrzejewski-Szmek from comment #8)
> > Not a blocker for review, but it would be nice to provide an appdata file
> > [https://fedoraproject.org/wiki/Packaging:AppData].
> It would be really nice to have an appdata file.
> 

Done.

> Unfortunately there's more bundling:
> 
> witch/net/pybonjour.py -- this is bundled
> http://code.google.com/p/pybonjour/. Fortunately the latest upstream version
> is bundled.
> 

I've simply removed this - this is only needed on OSX.

> The code in winswitch/twisted also looks a bit suspicious. But those are
> snippets of code, not a separate thing. They also appear to be
> windows-specific. Please check if they can be removed in %prep.
> 

They're still needed on Linux.

> winswitch/util/pam.py appears to be bundled python-pam. Fedora version is
> newer.
> 

Remove bundled version and adjusted code to use system wide version.

> Other files look good.
> 
> Please own /usr/share/nautilus-python dir.
> 

It was already owned.

> winswitch.noarch: E: executable-marked-as-config-file /etc/winswitch/firewall
> Please remove the executable bit and the #!/bin/sh header.
> 

Fixed

> Requires
> --------
> winswitch (rpmlib, GLIBC filtered):
>     /bin/bash      <---- this one comes from
> /usr/libexec/winswitch/firewall, OK.
>     /bin/sh        <---- this one comes from /etc/winswitch/firewall, so
> should go away when /etc/winswitch/firewall header is removed.
>     /usr/bin/env   <---- this one comes from /usr/libexec/winswitch/*. Most
> likely those should be edited to use %{__python2}.

Fixed.


>     /usr/bin/python2 <---- OK.
> 
> More work for you, sorry :)

No need to apologise! Thanks for catching all these things.

Spec URL: https://jgu.fedorapeople.org/winswitch.spec
SRPM URL: https://jgu.fedorapeople.org/winswitch-0.12.21-16.fc22.src.rpm

* Thu Aug 13 2015 Jonathan Underwood <jonathan.underwood> - 0.12.21-16
- Disable firewall control by default




I've decided to disable the firewall control functionality by default in the interests of security. In addition it doesn't seem well integrated with firewalld, so I think it's better disabled for now. In any case, in the default configuration, the server requires an ssh tunneled connection, so firewall modification isn't required.

It seems you didn't upload the latest version, winswitch.spec is still version -15 and srpm is missing.

Opps, sorry. Is there now (may need to refresh your browser).

===== 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". 5 files have unknown license. Detailed
     output of licensecheck in /var/tmp/1243530-winswitch/licensecheck.txt
[x]: Package requires other packages for directories it uses.
     Note: No known owner of /usr/share/nautilus-python
This seems to be a bug in nautilus-python itself. It owns nautilus-python/extensions, but not nautilus-python itself.

[!]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/share/nautilus-python
This package should co-own /usr/share/nautilus-python with nautilus-python.

[x]: Package does not own files or directories owned by other packages.
     Note: Dirs in package are owned also by:
     /usr/share/Thunar/sendto(winswitch, Thunar), /usr/share/nautilus-
     python/extensions(nautilus-python), /usr/share/Thunar(winswitch,
     Thunar)
OK.

[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: 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]: update-desktop-database is invoked in %post and %postun if package
     contains desktop file(s) with a MimeType: entry.
     Note: desktop file(s) with MimeType entry in winswitch
[x]: gtk-update-icon-cache is invoked in %postun and %posttrans if package
     contains icons.
     Note: icons in winswitch
[x]: Package is not known to require an ExcludeArch tag.
[x]: 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]: 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]: Package contains desktop file if it is a GUI application.
[x]: Package installs a %{name}.desktop using desktop-file-install or
     desktop-file-validate if there is such a file.
[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]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 0 bytes in 0 files.
[x]: Packages must not store files under /srv, /opt or /usr/local

Python:
[x]: Python eggs must not download any dependencies during the build
     process.
[ ]: A package which is used by another package via an egg interface should
     provide egg info.
[ ]: Package meets the Packaging Guidelines::Python
[x]: Package contains BR: python2-devel or python3-devel
[x]: Binary eggs must be removed in %prep

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

Generic:
[x]: update-mime-database is invoked in %post and %postun if package stores
     mime configuration in /usr/share/mime/packages.
     Note: mimeinfo files in: winswitch
[-]: 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).
[?]: Package functions as described.
I cannot test this currently (Flock).

[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]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[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]: 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: winswitch-0.12.21-16.fc24.noarch.rpm
          winswitch-0.12.21-16.fc24.src.rpm
winswitch.noarch: E: explicit-lib-dependency python-xlib
winswitch.noarch: W: spelling-error %description -l en_US xpra -> pray
winswitch.src: W: spelling-error %description -l en_US xpra -> pray
winswitch.src:87: W: macro-in-comment %{__python2}
winswitch.src:105: E: hardcoded-library-path in %{_prefix}/lib/nautilus/extensions-2.0/python/*
winswitch.src:108: E: hardcoded-library-path in %{_prefix}/lib/nautilus
2 packages and 0 specfiles checked; 3 errors, 3 warnings.




Rpmlint (installed packages)
----------------------------
winswitch.noarch: E: explicit-lib-dependency python-xlib
1 packages and 0 specfiles checked; 1 errors, 0 warnings.



Requires
--------
winswitch (rpmlib, GLIBC filtered):
    /bin/bash
    /bin/sh
    /usr/bin/python2
    avahi
    avahi-ui-tools
    config(winswitch)
    dbus-python
    dbus-x11
    gnome-menus
    gnome-python2
    gnome-python2-rsvg
    gstreamer
    openssh-clients
    pygtk2
    python(abi)
    python-crypto
    python-imaging
    python-netifaces
    python-pam
    python-pyasn1
    python-twisted
    python-utmp
    python-xlib



Provides
--------
winswitch:
    appdata()
    appdata(winswitch.appdata.xml)
    application()
    application(winswitch.desktop)
    config(winswitch)
    mimehandler(text/x-winswitchconfig)
    winswitch

This all looks good. Package is APPROVED.

Spec URL: https://jgu.fedorapeople.org/winswitch.spec
SRPM URL: https://jgu.fedorapeople.org/winswitch-0.12.21-17.fc22.src.rpm

* Fri Aug 14 2015 Jonathan Underwood <jonathan.underwood> - 0.12.21-17
- Own /usr/share/nautilus-python


Sorry, I had misread your earlier comment about that directory - I see the issue now.

Thanks very much for the great review, you really helped iron out a number of issues, it's great to see such a careful and thorough review.

New Package SCM Request
=======================
Package Name: winswitch
Short Description: Utility for controlling remote desktop sessions
Upstream URL: http://winswitch.org/
Owners: jgu cicku
Branches: f21 f22 f23
InitialCC: jgu cicku

Git done (by process-git-requests).

winswitch-0.12.21-17.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/winswitch-0.12.21-17.fc21

winswitch-0.12.21-17.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/winswitch-0.12.21-17.fc22

winswitch-0.12.21-17.fc23 has been submitted as an update for Fedora 23.
https://admin.fedoraproject.org/updates/winswitch-0.12.21-17.fc23

winswitch-0.12.21-17.fc21 has been pushed to the Fedora 21 testing repository.

winswitch-0.12.21-17.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.

winswitch-0.12.21-17.fc21 has been pushed to the Fedora 21 stable repository. If problems still persist, please make note of it in this bug report.

winswitch-0.12.21-17.fc22 has been pushed to the Fedora 22 stable repository. If problems still persist, please make note of it in this bug report.