Bug 1823265

With the existence of https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/362 and the fact that other distributions are shipping neatvnc separately, I'd suggest creating separate package.
Contents of devel package only confirm my point:
% rpm -qlp results_wayvnc/0.1.2/1.fc32/wayvnc-devel-0.1.2-1.fc32.x86_64.rpm
/usr/include/neatvnc.h
/usr/lib64/libneatvnc.so
/usr/lib64/pkgconfig/neatvnc.pc

For meson it's preferable to convert dependency('x') directly into BuildRequires: pkgconfig(x). Or BuildRequires: cmake(x) for the cases where dependency does not ship .pc file and meson is using cmake to resolve it.

I don't see the reason for BR: wlroots-devel, nothing is using wlroots in both sources. You'll need `BuildRequires: pkgconfig(pixman-1)` and `BuildRequires: pkgconfig(libdrm)` instead.

I'd also suggest to unbundle miniz but neatvnc upstream already dropped it (https://github.com/any1/neatvnc/commit/b44d1a1f58341ecd78cbdd91eb03865849bcf0a8) so it's fine to wait until the next upstream release.

Would be nice to declare that sway >= 1.4 is necessary for wayvnc without making it hard dependency. People may want to use it with wayfire >= 0.4.0 for example.
I don't have any good ideas for that except `Requires: wlroots >= 0.10`, which is still not good enough.

Thanks Aleksei,

Here's the separated neatvnc RR: https://bugzilla.redhat.com/show_bug.cgi?id=1824016

New build of wayvnc based on that:
SPEC URL: https://download.copr.fedorainfracloud.org/results/wef/wayvnc/fedora-31-x86_64/01338261-wayvnc/wayvnc.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/wef/wayvnc/fedora-31-x86_64/01338261-wayvnc/wayvnc-0.1.2-2.fc31.src.rpm

Question: once neatvnc is approved it will take some time to be available in koji - maybe a week or more. During that time, is there any way we can progress wayvnc in koji?

(In reply to Bob Hepple from comment #2)
> 
> Question: once neatvnc is approved it will take some time to be available in
> koji - maybe a week or more. During that time, is there any way we can
> progress wayvnc in koji?

Yes, you can add neatvnc to buildroot override[1] and once koji applies override (15-30 min) you can build wayvnc. You don't need to do that for rawhide as it's configured to add packages to buildroot with just a minor delay. However, using chain build[2] is still recommended.
Note that if you are using buildroot override for dependent packages, it also makes sense to publish them in one bodhi updade.

[1] https://fedoraproject.org/wiki/Bodhi/BuildRootOverrides
[2] https://fedoraproject.org/wiki/Using_the_Koji_build_system#Chained_builds

Unnecessary BRs: pkgconfig(glib), pkgconfig(libglvnd), pkgconfig(wayland-eglstream). Build passes without all of those.

> %meson --buildtype=release

Please, remove --buildtype. %meson macro already sets `--buildtype plain`

> %doc README.md

Consider adding FAQ.md to the doc.

> Requires: wlroots >= 0.10

Please, add a comment that the dependency here is added only to enforce minimal version of a wlroots-based compositor.

Also, technically that makes package not installable on Fedora < 32 without sway module. It's safe to ignore f30 (EOL by the end of May) and epel8 (does not even have sway modular builds), but f31 is still supported for a while. However if we allow installation with older sway it would only serve as a source of confusion and bug reports.
I admit that I don't know a good solution for that besides outright ignoring f31.

Just chipping in here:
(In reply to Aleksei Bavshin from comment #1)

> 
> Would be nice to declare that sway >= 1.4 is necessary for wayvnc without
> making it hard dependency. 

I think you could do this with rich dependencies. Something like
  Requires: sway >= 1.4.0 if sway
should work. I haven't double-checked though.

(In reply to Aleksei Bavshin from comment #4)
> 
> Also, technically that makes package not installable on Fedora < 32 without
> sway module. It's safe to ignore f30 (EOL by the end of May) and epel8 (does
> not even have sway modular builds), but f31 is still supported for a while.
> However if we allow installation with older sway it would only serve as a
> source of confusion and bug reports.
> I admit that I don't know a good solution for that besides outright ignoring
> f31.

That's a perfectly valid solution. New packages don't have to be shipped with all active releases.
We can still include the package in the module.

What about neatvnc:include/sys/queue.h: SPDX-License-Identifier: BSD-3-Clause

It's not one I've seen before nor is it in the list at https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#SoftwareLicenses

.... just 'BSD' per https://fedoraproject.org/wiki/Licensing:BSD?rd=Licensing/BSD#3ClauseBSD ???

wrt:

> %meson --buildtype=release

... removing that yields -O2 instead of -O3 and upstream recommends it:

'Setting the buildtype flag is not required but it is recommended as there are significant performance gains to be had from an optimised build.'

... so should we use 'release'???

RPM documentation[1] says that rich dependencies should be enclosed with parenthesis, because otherwise RPM will treat this as a 3 separate package clauses:
Requires: (sway >= 1.4 if sway)

I confirmed that it works as desired on Fedora 31:

% sudo dnf install sway wayvnc
Last metadata expiration check: 0:00:55 ago on Wed 15 Apr 2020 03:27:30 PM UTC.
Error:
 Problem: conflicting requests
  - package sway-1.2-2.module_f31+6388+036e501b.x86_64 requires (sway >= 1.4 if sway), but none of the providers can be installed
  - package sway-1.1.1-3.fc31.x86_64 requires (sway >= 1.4 if sway), but none of the providers can be installed
  - package sway-1.1.1-2.fc31.x86_64 requires (sway >= 1.4 if sway), but none of the providers can be installed
  - package sway-1.4-3.module_f31+7694+72cbf402.x86_64 is filtered out by modular filtering
(try to add '--skip-broken' to skip uninstallable packages)


(In reply to Bob Hepple from comment #7)
> .... just 'BSD' per
> https://fedoraproject.org/wiki/Licensing:BSD?rd=Licensing/BSD#3ClauseBSD ???

Yes, just BSD. It's in the license list under 'BSD License (no advertising)'.


(In reply to Bob Hepple from comment #8)
> ... removing that yields -O2 instead of -O3 and upstream recommends it:
> 
> 'Setting the buildtype flag is not required but it is recommended as there
> are significant performance gains to be had from an optimised build.'

I'm pretty sure that -O2 is still considered optimized build. But let's see what `--buildtype release` actually does:

> %meson

[26/35] cc -Iwayvnc@exe -I. -I.. -I../include -I/usr/include/pixman-1 -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -std=gnu11 -D_GNU_SOURCE -DNDEBUG -O2 -g -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -MD -MQ 'wayvnc@exe/src_main.c.o' -MF 'wayvnc@exe/src_main.c.o.d' -o 'wayvnc@exe/src_main.c.o' -c ../src/main.c

> %meson --buildtype release

[26/35] cc -Iwayvnc@exe -I. -I.. -I../include -I/usr/include/pixman-1 -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -std=gnu11 -O3 -D_GNU_SOURCE -DNDEBUG -O2 -g -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -MD -MQ 'wayvnc@exe/src_main.c.o' -MF 'wayvnc@exe/src_main.c.o.d' -o 'wayvnc@exe/src_main.c.o' -c ../src/main.c

In this case gcc takes the last specified -O flag which is still -O2.

The working way of applying -O3 is to override %optflags macro, however guidelines[2] say that this is discouraged unless you have a good reason. A benchmark that shows significant performance improvement from -O3 could be such reason.
It doesn't seem critical for wayvnc (I don't see a code that could significantly win from -O3), but it might be worth investigating for neatvnc.

[1] https://rpm.org/user_doc/boolean_dependencies.html
[2] https://docs.fedoraproject.org/en-US/packaging-guidelines/#_compiler_flags

Thanks again for your thoughtful and in-depth review. I'm learning a lot!

I'll pick up the -O2/3 issue with upstream. Meanwhile here is a plain %meson version:

SPEC URL: https://download.copr.fedorainfracloud.org/results/wef/wayvnc/fedora-31-x86_64/01339287-wayvnc/wayvnc.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/wef/wayvnc/fedora-31-x86_64/01339287-wayvnc/wayvnc-0.1.2-3.fc31.src.rpm

Kevin kindly fixed bugzilla permissions for me and now I can try on the reviewer hat and do this:
---
Package approved. Thanks for all the work on this package!




Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: If your application is a C or C++ application you must list a
     BuildRequires against gcc, gcc-c++ or clang.
[x]: Header files in -devel subpackage, if present.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

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: "ISC License", "Unknown or generated". 17 files have unknown
     license. Detailed output of licensecheck in
     /home/alebastr/rpmbuild/GIT/neatvnc/1823265-wayvnc/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: %build honors applicable compiler flags or justifies otherwise.
[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]: Useful -debuginfo package or justification otherwise.
[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.
[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]: 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]: 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 must not depend on deprecated() packages.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[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.
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
     Note: gpgverify is not used.
[-]: 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]: Fully versioned dependency in subpackages if applicable.
[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 a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on debuginfo package(s).
     Note: No rpmlint messages.
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: wayvnc-0.1.2-3.fc33.x86_64.rpm
          wayvnc-debuginfo-0.1.2-3.fc33.x86_64.rpm
          wayvnc-debugsource-0.1.2-3.fc33.x86_64.rpm
          wayvnc-0.1.2-3.fc33.src.rpm
wayvnc.x86_64: W: spelling-error Summary(en_US) wlroots -> roots
wayvnc.x86_64: W: spelling-error %description -l en_US wlroots -> roots
wayvnc.x86_64: W: no-manual-page-for-binary wayvnc
wayvnc.src: W: spelling-error Summary(en_US) wlroots -> roots
wayvnc.src: W: spelling-error %description -l en_US wlroots -> roots
4 packages and 0 specfiles checked; 0 errors, 5 warnings.




Rpmlint (debuginfo)
-------------------
Checking: wayvnc-debuginfo-0.1.2-3.fc33.x86_64.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.





Rpmlint (installed packages)
----------------------------
wayvnc-debugsource.x86_64: W: invalid-url URL: https://github.com/any1/wayvnc <urlopen error [Errno -2] Name or service not known>
wayvnc-debuginfo.x86_64: W: invalid-url URL: https://github.com/any1/wayvnc <urlopen error [Errno -2] Name or service not known>
wayvnc.x86_64: W: spelling-error Summary(en_US) wlroots -> roots
wayvnc.x86_64: W: spelling-error %description -l en_US wlroots -> roots
wayvnc.x86_64: W: invalid-url URL: https://github.com/any1/wayvnc <urlopen error [Errno -2] Name or service not known>
wayvnc.x86_64: W: no-manual-page-for-binary wayvnc
3 packages and 0 specfiles checked; 0 errors, 6 warnings.



Source checksums
----------------
https://github.com/any1/wayvnc/archive/v0.1.2/wayvnc-0.1.2.tar.gz :
  CHECKSUM(SHA256) this package     : 0ba19a279b62d1907a62e813183c7b10af6d4d1fcb4d274e26d6567956bb1349
  CHECKSUM(SHA256) upstream package : 0ba19a279b62d1907a62e813183c7b10af6d4d1fcb4d274e26d6567956bb1349


Requires
--------
wayvnc (rpmlib, GLIBC filtered):
    (sway >= 1.4 if sway)
    libEGL.so.1()(64bit)
    libGLESv2.so.2()(64bit)
    libc.so.6()(64bit)
    libm.so.6()(64bit)
    libneatvnc.so.0()(64bit)
    libpixman-1.so.0()(64bit)
    libpthread.so.0()(64bit)
    librt.so.1()(64bit)
    libuv.so.1()(64bit)
    libwayland-client.so.0()(64bit)
    libxkbcommon.so.0()(64bit)
    libxkbcommon.so.0(V_0.5.0)(64bit)
    libxkbcommon.so.0(V_0.6.0)(64bit)
    rtld(GNU_HASH)

wayvnc-debuginfo (rpmlib, GLIBC filtered):

wayvnc-debugsource (rpmlib, GLIBC filtered):



Provides
--------
wayvnc:
    wayvnc
    wayvnc(x86-64)

wayvnc-debuginfo:
    debuginfo(build-id)
    wayvnc-debuginfo
    wayvnc-debuginfo(x86-64)

wayvnc-debugsource:
    wayvnc-debugsource
    wayvnc-debugsource(x86-64)



Generated by fedora-review 0.7.5 (5fa5b7e) last change: 2020-02-16
Command line :/usr/bin/fedora-review -b 1823265
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api, C/C++
Disabled plugins: PHP, SugarActivity, R, fonts, Ocaml, Python, Haskell, Perl, Java
Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH

Thanks Aleksei!

I've requested the repos for wayvnc & neatvnc for rawhide, f31, f32

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/wayvnc

FEDORA-2020-70065f522a has been submitted as an update to Fedora 32. https://bodhi.fedoraproject.org/updates/FEDORA-2020-70065f522a

FEDORA-2020-4f0b3668f7 has been submitted as an update to Fedora 31. https://bodhi.fedoraproject.org/updates/FEDORA-2020-4f0b3668f7

FEDORA-2020-70065f522a has been pushed to the Fedora 32 testing repository.
In short time you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --advisory=FEDORA-2020-70065f522a \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2020-70065f522a

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

FEDORA-2020-4f0b3668f7 has been pushed to the Fedora 31 testing repository.
In short time you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --advisory=FEDORA-2020-4f0b3668f7 \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2020-4f0b3668f7

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

FEDORA-2020-70065f522a has been pushed to the Fedora 32 stable repository.
If problem still persists, please make note of it in this bug report.

FEDORA-2020-4f0b3668f7 has been pushed to the Fedora 31 stable repository.
If problem still persists, please make note of it in this bug report.