Spec URL: https://pagure.io/RaySession/raw/master/f/RaySession.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/eeickmeyer/RaySession/fedora-31-x86_64/01235034-raysession/raysession-0.8.3-1.fc31.noarch.rpm Description: Ray Session is a GNU/Linux session manager for audio programs as Ardour, Carla, QTractor, Non-Timeline, etc... It uses the same API as Non Session Manager, so programs compatible with NSM are also compatible with Ray Session. As Non Session Manager, the principle is to load together audio programs, then be able to save or close all documents together. Ray Session offers a little more: - Factory templates for NSM and LASH compatible applications - Possibility to save any client as template - Save session as template - Name files with a prettier way - remember if client was started or not - Abort session almost anytime - Change Main Folder of sessions on GUI - Possibility to KILL client if clean exit is too long - Open Session Folder button (open default file manager) Ray Session is being developed by houston4444, using Python3 and Qt5. Fedora Account System Username: eeickmeyer
*** Bug 1800994 has been marked as a duplicate of this bug. ***
Spec URL: https://pagure.io/RaySession/raw/master/f/raysession.spec
Some observations at a first glance (not a formal review), as already pointed out on IRC: 1. %global debug_package %{nil} is almost always an error and a guideline/policy violation. (They are officially called "Guidelines", but they are really policies.) I see some BuildRequires on compiled libraries, so I assume this is not pure Python, but has some C++ or C code. For that code, the debuginfo extraction MUST be used. 2. That Provides/Requires filtering: Is this package using bundled JACK libraries? If so, why? And the required Provides: bundled(jack-audio-connection-kit) is missing in that case. But ideally, you should build against the system jack-audio-connection-kit-devel instead. 3. %files hardcodes /usr/bin and /usr/share instead of using %{_bindir} and %{_datadir} as it should. 4. %files lists only files and not directories. As a result, the directories are unowned. Directories should be listed either as %dir /path/to/dir (lists only the directory) or as /path/to/dir/ (which automatically includes all the files and subdirectories in that directory so that you do not have to list every single file individually as you did). If you have multiple files in a directory (that you do not want to list as a directory because it is owned by another package), you can also use wildcards. Though those should be used with care, because they can mask some unexpectedly added or removed files in new upstream versions (and in particular, the packaging guidelines now state that you must not wildcard library soversions to avoid accidental soname bumps). There are probably more issues, but those are the ones that caught my eye.
All requested changes have been made, please take another look if able. :)
> Source: https://github.com/Houston4444/RaySession/archive/v%{version}.tar.gz This Source URL should be better structured per our SourceURL guidelines: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/ Per the guidelines, it'd be something like this: > Source0: https://github.com/Houston4444/RaySession/archive/v%{version}/RaySession-%{version}.tar.gz
Alternatively, the Source URL can be shortened to: > Source0: %{url}/archive/v%{version}/RaySession-%{version}.tar.gz This is because the URL statement is the same as the first half of the Source0 value.
I'm not sure what the following at the top of the spec is for: > # https://github.com/Houston4444/RaySession/commit/82cde82ea1329304b52d35e53a69df921d0cecad > > %global pname raysession > %global commitdate 20191125 It seems to be completely unused, so it should be removed.
The requested changes have been made, SourceURL has been shortened.
SRPM URL: https://download.copr.fedorainfracloud.org/results/eeickmeyer/Jam-Incoming/fedora-rawhide-x86_64/01242586-raysession/raysession-0.8.3-1.fc33.src.rpm Moved to new copr.
Is there anything more that needs to be done?
When I run fedora-review these are the points where I don't see a "Pass" at the moment. [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [?]: Package obeys FHS, except libexecdir and /usr/target. [?]: Requires correct, justified where necessary. [?]: Package complies to the Packaging Guidelines [!]: Uses parallel make %{?_smp_mflags} macro. [?]: Package functions as described. [?]: Patches link to upstream bugs/comments/lists or are otherwise [?]: %check is present and all tests pass. 1) REQUIRES Having looked at the source code, I found that everything is pure python, so no C-code is involved. One program is using git and another uses xdotool, so I think that the Requires-section has to be updated with at least python3, git and xdotool, maybe there are other dependencies that I haven't spotted yet. If "jack-audio-connection-kit" would be neccessary for proper funktionality, it must also be required. 2) Filenames There is a file packaged as 'snapshot explain' tha should have a name without a blank. Raysession creates a directory 'Ray Session' in the user's home directory, which will also be safer if the blank is deleted. 3) man pages "If some man pages are absent, packagers SHOULD work with upstream to add them." I assume that all three "binaries" in /usr/bin should be invoked by users not only other programs, so man pages would be beneficial. If they exist, they can be additional SOURCE files. 4) Running the binary "raysession" I tried to run raysession on F32 and got a lot of errors regarding qt5 and Wayland. To make sure users have a professional experience in using the software I advise to check any runtime error messages that may be caused by missing dependencies (not yet spotted). Here is some work to be done. 5) "Binaries" The package installs three binaries in /usr/bin that call exec and point into /usr/share/%{name} where the python source code is located under /usr/share/%{name}/src. I really don't know if that is allowed, so you may ask on the devel list about the proper location for the src-files. My point of view would be that %{_libdir}/%{name}/ is more appropriate than %{_datadir}/%{name}/. But maybe your location is permissible, I dont't know.
You also need to update the license tag to GPLv2, because the COPYING file defines the license for all software in RaySession (see https://github.com/Houston4444/RaySession/blob/master/COPYING) and this is good old version 2.
Hi Ralf, I received a response from the developer: 1) "RS uses git and wmctrl (not xdotool, but maybe wmctrl is in xdotool on Fedora). These 2 programs are not needed, RS works without them. However, if the use of wmctrl is quite trivial, the use of git is recommended. Jack connection kit is only used by the ray-jackpatch (connections memory) which is a subprogram, it should not be considered as a dependency." -- As such, I'll add all three of those as Recommends. 2) "snapshot explain", ok, I'll replace the blank. -- Until that happens, I'll be renaming it inside %prep "'Ray Sessions' folder, I need more reflection, this name was based on the template of NSM ('NSM Sessions' folder). For RaySession, this directory string is translatable. Shouldn't it and why ?" -- I agree with them here, this should not be a blocker. 3) "Globally, Manual is the most missing feature in RaySession. Maybe man pages could only take the stdout of 'command --help' in a first time." -- They're aware of the issue, but afaik, this isn't a blocker especially if there's a known bug. I can remark that in the .spec under %files. 4) "Possible. I did try RS under Wayland (plasma wayland), but a long time ago and not a long time. If someone is motivated to write bug report about this, It would be appreciated." -- I just ran it on a KDE Plasma session of F32 (Xorg) and had zero errors. Perhaps this is a Wayland issue? I'll double-check by running inside GNOME under Wayland. 5) "No Idea. Before to change anything here, I need to be 100% sure this is strictly forbidden." -- I will be following-up with #fedora-devel on this one. Either way, new files as follows: Spec URL: https://download.copr.fedorainfracloud.org/results/eeickmeyer/Jam-Incoming/fedora-rawhide-x86_64/01304388-raysession/raysession.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/eeickmeyer/Jam-Incoming/fedora-rawhide-x86_64/01304388-raysession/raysession-0.8.3-1.fc33.src.rpm
Hi Ralf, Further on point #5, I'm pretty sure it's allowed, at least on the OS level, because one of the strictest archive admins for Ubuntu, who is also a very strict Debian Developer, reviewed the package I made there for the same application and made it part of the archive. Not sure if there's any difference in Fedora land in those regards, but that should be telling. Also, I did change the license, forgot to mention that in my previous comment.
IRC log: (also requested a comment from Eighth_Doctor): <Eighth_Doctor> Eickmeyer: usually that stuff should be in /usr/share/%{name} or /usr/libexec/%{name} <Eighth_Doctor> Eickmeyer: err %{_datadir}/%{name} or %{_libexecdir}/%{name} <Eighth_Doctor> Eickmeyer: generally, I would suggest that the latter is correct, but the former is fine if it is noarch <Eickmeyer> Eighth_Doctor: Yes, it is, just confirmed. <Eickmeyer> Eighth_Doctor: And it's noarch. <Eighth_Doctor> Eickmeyer: it can live in %{_datadir}/%{name}, but even I would recommend %{_libexecdir}/%{name} <Eighth_Doctor> Eickmeyer: but it's fine <Eickmeyer> Eighth_Doctor: Thanks, good to know that's not a blocker. <Eighth_Doctor> it's probably a huge architectural mistake, but our FHS permits it in this scenario In other words, looks like no changes are needed in point #5.
I no longer need sponsorship as I'm a packager, but I really would like to get this reviewed ASAP. Thanks. :)
(In reply to Erich Eickmeyer from comment #16) > IRC log: (also requested a comment from Eighth_Doctor): > > <Eighth_Doctor> Eickmeyer: usually that stuff should be in > /usr/share/%{name} or /usr/libexec/%{name} > <Eighth_Doctor> Eickmeyer: err %{_datadir}/%{name} or %{_libexecdir}/%{name} > <Eighth_Doctor> Eickmeyer: generally, I would suggest that the latter is > correct, but the former is fine if it is noarch > <Eickmeyer> Eighth_Doctor: Yes, it is, just confirmed. > <Eickmeyer> Eighth_Doctor: And it's noarch. > <Eighth_Doctor> Eickmeyer: it can live in %{_datadir}/%{name}, but even I > would recommend %{_libexecdir}/%{name} > <Eighth_Doctor> Eickmeyer: but it's fine > <Eickmeyer> Eighth_Doctor: Thanks, good to know that's not a blocker. > <Eighth_Doctor> it's probably a huge architectural mistake, but our FHS > permits it in this scenario > > In other words, looks like no changes are needed in point #5. I agree that this is not a show stopper, because %{_datadir}/%{name} is not exactly forbidden although a different location would be better. For the fact that the source code cannot be easily changed to accommodate %{_libdir}/%{name} I regard this issue as done. If upstream provides the installation in a different location in the future, this point should be re-considered.
Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated ===== 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. All source code is GPLv2. [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]: 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 4 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]: 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 must not depend on deprecated() packages. [-]: 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: [x]: Uses parallel make %{?_smp_mflags} macro. [-]: 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. There is a function call (QWindow::requestActivate()) that is not allowed with Wayland which leads to an unusual amount of error messages in the terminal that starts raysession Unfortunately these error messages will obscure all other messages in the terminal. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [x]: Patches link to upstream bugs/comments/lists or are otherwise justified. [-]: 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. [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]: 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]: 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 all installed packages. Note: There are rpmlint messages (see attachment). [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: raysession-0.8.3-1.fc33.noarch.rpm raysession-0.8.3-1.fc33.src.rpm raysession.noarch: W: no-manual-page-for-binary ray-daemon raysession.noarch: W: no-manual-page-for-binary ray_control raysession.noarch: W: no-manual-page-for-binary raysession 2 packages and 0 specfiles checked; 0 errors, 3 warnings. Man pages are not available at the moment. Upstream is informed. Rpmlint (installed packages) ---------------------------- raysession.noarch: W: invalid-url URL: https://github.com/Houston4444/RaySession <urlopen error [Errno -2] Name or service not known> Checked: URL is valid! raysession.noarch: W: no-manual-page-for-binary ray-daemon raysession.noarch: W: no-manual-page-for-binary ray_control raysession.noarch: W: no-manual-page-for-binary raysession 1 packages and 0 specfiles checked; 0 errors, 4 warnings. Source checksums ---------------- https://github.com/Houston4444/RaySession/archive/v0.8.3/RaySession-0.8.3.tar.gz : CHECKSUM(SHA256) this package : 9efa35dae2bab66ee0ebdd141224e72db3637e94d44eec678da4352d1d5edcfc CHECKSUM(SHA256) upstream package : 9efa35dae2bab66ee0ebdd141224e72db3637e94d44eec678da4352d1d5edcfc Requires -------- raysession (rpmlib, GLIBC filtered): /usr/bin/bash /usr/bin/python3 /usr/bin/sh hicolor-icon-theme python3-pyliblo python3-qt5 shared-mime-info Provides -------- raysession: application() application(raysession.desktop) raysession Generated by fedora-review 0.7.5 (5fa5b7e) last change: 2020-02-16 Command line :/usr/bin/fedora-review -n raysession Buildroot used: fedora-rawhide-x86_64 Active plugins: Shell-api, Generic Disabled plugins: Perl, SugarActivity, Java, Python, fonts, R, Haskell, C/C++, Ocaml, PHP Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
Your package has been APPROVED.
There is one point you'll have to work with upstream to resolve: Ordinary users (not necessary JACK enthusiasts) will use raysession in a standard installation with WAYLAND. Once started in a terminal, they will be flooded with an error message that reads "qt.qpa.wayland: Wayland does not support QWindow::requestActivate()" which obfuscates numerous other messages that are printed to the terminal. This is bad user experience. In the QT Forum this was discussed here (https://forum.qt.io/topic/90639) with the result that using this function call is not allowed in wayland. "This means your window tried to grab focus in the compositor, which is not allowed in wayland. Don't create a bug for it, there's nothing we can do." So only upstream can solve this problem by avoiding this function call. Please work with upstream to get this done and update your package when there is a solution. In the meantime I think adding a file READEME-wayland in /usr/share/doc/raysession can help users to understand the issue and maybe help to avoid confusion. Feel free to add such a file and a comment in the spec file.
Thanks, Ralf. I just filed a new issue with upstream and documented it in the .spec. Additionally, I made a README-wayland explaining the situation. Thanks for your help!
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/raysession
FEDORA-2020-aac56ea73b has been submitted as an update to Fedora 31. https://bodhi.fedoraproject.org/updates/FEDORA-2020-aac56ea73b
FEDORA-2020-4eb045a393 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-4eb045a393 \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2020-4eb045a393 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2020-aac56ea73b 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-aac56ea73b \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2020-aac56ea73b See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2020-4eb045a393 has been pushed to the Fedora 32 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-2020-aac56ea73b has been pushed to the Fedora 31 stable repository. If problem still persists, please make note of it in this bug report.
*** Bug 1848934 has been marked as a duplicate of this bug. ***