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 1250833 - Review Request: helm - Polyphonic software synth with lots of modulation and easy to use UI
Summary: Review Request: helm - Polyphonic software synth with lots of modulation and ...
Keywords:
Status: CLOSED DUPLICATE of bug 1661657
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-NEEDSPONSOR
TreeView+ depends on / blocked
 
Reported: 2015-08-06 06:26 UTC by L.L.Robinson
Modified: 2019-02-21 20:58 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2019-02-21 20:58:15 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description L.L.Robinson 2015-08-06 06:26:15 UTC
Spec URL: http://therobinsonfamily.net/SPECS/helm.spec
SRPM URL: http://therobinsonfamily.net/SRPMS/helm-0.4.1-3.fc22.src.rpm
Description: Helm is a Software Synth and lv2 plugin which is designed to have a simple UI
Fedora Account System Username: baggypants

Comment 1 L.L.Robinson 2015-08-06 06:32:34 UTC
Scratch builds available on cop https://copr.fedoraproject.org/coprs/baggypants/helm-stable/

Comment 2 L.L.Robinson 2015-08-06 12:16:27 UTC
This is my first request and I need a sponsor.

Comment 3 Igor Gnatenko 2015-08-06 12:30:44 UTC
I'm not a sponsor, but I will give you some notes.

-> Source0: https://github.com/mtytel/helm/archive/v0.4.1.zip
   Source0: https://github.com/mtytel/helm/archive/v%{version}.tar.gz#/%{name}-%{version}.tar.gz
   Look at: https://fedoraproject.org/wiki/Packaging:SourceURL#Git_Tags
   You also can use %{url} instead of first part in Source0 tag

-> Requires:	mesa-libGL, alsa-lib, jack-audio-connection-kit, freetype, libXrandr, libXinerama, libXcursor, helm-common
   Please make it separate (I mean in some lines, not it one.
   I don't think that most of them is needed (I'm sure that RPM will pick up 90% of them automatically)

-> Requires: helm-common
   you should specify version at least. Example:
   Requires: helm-common = %{version}

-> For common subpkg please build noarch version.

-> $RPM_BUILD_ROOT
   replace with %{buildroot}

-> make %{?_smp_mflags}
   you could use %make_build

-> "%_libdir/lv2/helm.lv2/helm.so"
   don't need to use quotes there

-> Also i think you just want to have there just: %{_libdir}/lv2/helm.lv2/
   don't need to specify files manually
   The same for common subpackage

-> %doc COPYING
   should be %license COPYING

-> "%{_bindir}/helm"
   again, no quotes and try to use %{name} where it's possible

-> %setup -q 
   You could use %autosetup

-> License: GPL3
   GPL3 is not valid license. You'd want GPLv3 or GPLv3+ (haven't checked sources)

-> I've not checked desktop file, but looks like no icons in hicolor theme, only in /usr/share/helm. So if you want to use icons there -- use %{_datadir}/icons/hicolor/* as place for icons

Comment 4 L.L.Robinson 2015-08-07 11:10:04 UTC
Excellent review, really helpful. I'm pretty sure I did everything. Regarding the icon, it was in pixmaps but now it's in icons/hicolor/

New version
http://therobinsonfamily.net/SRPMS/helm-0.4.1-4.fc22.src.rpm

http://therobinsonfamily.net/SPECS/helm.spec

Comment 6 Michael Schwendt 2015-08-27 13:45:24 UTC
Please fill in your full real name in your bugzilla account preferences.


Consider pointing the fedora-review tool at this ticket:

  fedora-review -b 1250833

It downloads the latest spec file and src.rpm from the "Spec URL:" and "SRPM URL:" lines (or additional URLs it finds) and performs many checks that are relevant during review and should be most interesting to the package maintainer.


> -> Requires: helm-common
>    you should specify version at least. Example:
>    Requires: helm-common = %{version}

Omitting -%{release} serves no purpose. Actually, -common subpackages are disguised base packages, and this one applies except the %{?_isa}:

  https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package


> %doc

%doc is not a section but an attribute. Empty %doc lines are no-op. It's more cleaner to delete them.


> %package lv2
> Summary:	Helm lv2 plugin
> Group:		Applications/Multimedia
> Requires: 	%{name}-common = %{version}
> %description lv2
> Helm is a polyphonic software synth with lots of modulation and and easy to use UT

The %description ought to explain what this particular package does. What's the relevance of LV2, for example? Why is the %description longer than the %description of the base package?

> %description
> Helm is a software synth designed to be easy to use

The subpackage is missing an explicit dependency on "lv2". Note that since LV2 is a library-less API, there is no automatic dependency on it, but package "lv2" is the one that provides the ownership of %_libdir/lv2.


> %files lv2
> %{_libdir}/lv2/helm.lv2/

> %package lv2

That creates a subpackage helm-lv2. Instead, I think the general add-on package naming guidelines apply:

  https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Addon_Packages_.28General.29

It is a plugin to LV2. It extends LV2.

%package -n lv2-helm


> https://kojipkgs.fedoraproject.org//work/tasks/4821/10784821/build.log

Build output is non-verbose. One cannot see which compiler and linker flags have been used actually.

https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags

Comment 7 L.L.Robinson 2015-08-31 17:58:54 UTC
new versions

Spec URL: http://therobinsonfamily.net/SPECS/helm.spec
SRPM URL: http://therobinsonfamily.net/SRPMS/helm-0.4.1-7.fc22.src.rpm

Build info: http://koji.fedoraproject.org/koji/taskinfo?taskID=10898667

Everything done apart from the verbose build output. The linked output regarding compiler flags and the optflags meant little to me as I'm not a programmer. I tried adding it to %make_build but the build failed. I'm guessing I may need to patch the Makefile to add the flags, or raise a bug upstream.

Comment 8 L.L.Robinson 2015-09-17 21:43:38 UTC
Spec URL: http://therobinsonfamily.net/SPECS/helm.spec
SRPM URL: https://therobinsonfamily.net/SRPMS/helm-0.5.0-2.fc22.src.rpm

Build info: http://koji.fedoraproject.org/koji/taskinfo?taskID=11128243

Patched Makefiles to make buildoutput verbose.

Comment 9 L.L.Robinson 2015-09-17 21:44:57 UTC
SRPM URL: http://therobinsonfamily.net/SRPMS/helm-0.5.0-2.fc22.src.rpm

Comment 10 L.L.Robinson 2015-09-24 22:45:37 UTC
An informational note that the next upstream version will not require patching to make the build verbose.

https://github.com/mtytel/helm/issues/30#issuecomment-141542740

Comment 11 Michael Schwendt 2015-10-20 11:28:56 UTC
Good.

Some more advanced topics:


> License:	GPLv3

The source files say "or later", which would be "GPLv3+":

https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#.22or_later_version.22_licenses


> ExcludeArch:	armv7hl

https://fedoraproject.org/wiki/Packaging:Guidelines#Architecture_Build_Failures


> Requires:	lv2

# dnf list lv2|grep ^lv
lv2.i686                           1.10.0-2.fc22                          fedora
lv2.x86_64                         1.10.0-2.fc22                          fedora

As you're shipping files in %{_libdir}/lv2/helm.lv2/ you want to make this explicit dependency arch-specific via %{?_isa}:
https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires


> %{_datadir}/applications/helm.desktop

/builddir/build/SOURCES/helm.desktop: warning: key "Categories" is a list and does not have a semicolon as trailing character, fixing

/builddir/build/BUILDROOT/helm-0.5.0-2.fc24.x86_64/usr/share/applications/helm.desktop: warning: value "Software Synthesizer" for key "Comment" in group "Desktop Entry" looks redundant with value "Software Synthesizer" of key "GenericName"


> %{_datadir}/applications/helm.desktop

https://fedoraproject.org/wiki/Packaging:Guidelines#AppData_files


> helm-0.5.0/JUCE/

That is a bundled library from juce.com and makes the package non-trivial. The No_Bundled_Libraries policy has been changed recently, and the controversial change is still being discussed:

https://fedoraproject.org/w/index.php?title=Packaging:No_Bundled_Libraries&oldid=406058


> helm-0.5.0/mopo/

Dunno whether the author has released this independently yet
(apart from https://github.com/mtytel/mopo ) or only together with Helm. Not an issue yet.


> helm-0.5.0/fonts/

$ grep ttf build.log 
$

Also don't appear in the packages.


> /usr/share/helm/patches/LICENSE

That's not GPLv3, and this file in the helm-common package is not marked %license either. This is some Creative Commons Attribution 4.0 license.

Note that each (sub-)package can have its own Group tag:
https://fedoraproject.org/wiki/Packaging:Guidelines#Licensing


> rpmlint

It finds some things when running it on all the built packages.


> Unknown

It GNOME Shell (Fedora 22), the application /usr/bin/helm creates a shell menu entry with the name "Unknown".

Comment 12 Igor Gnatenko 2016-08-08 05:53:15 UTC
ping?

Comment 13 L.L.Robinson 2016-08-22 16:39:06 UTC
The developer has stopped tagging builds and is now bundling more libraries like libpng. 

https://github.com/mtytel/helm/commit/468f26b7dc7b26b0fcc122391502c105c6f1d61b

I think at the moment this project and my skills are to immature for me to package at this present time.

Comment 14 Raphael Groner 2019-02-21 20:58:15 UTC

*** This bug has been marked as a duplicate of bug 1661657 ***


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