Bug 834239 - Review Request: monobristol - frontend for britsol in mono
Review Request: monobristol - frontend for britsol in mono
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Orcan Ogetbil
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FedoraAudio
  Show dependency treegraph
 
Reported: 2012-06-21 06:00 EDT by Jørn Lomax
Modified: 2012-11-26 06:26 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-11-26 06:26:02 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
oget.fedora: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)
Patch for .desktop file (508 bytes, patch)
2012-06-26 09:14 EDT, Jørn Lomax
no flags Details | Diff
Updates -desktop.patch (552 bytes, patch)
2012-06-27 07:57 EDT, Jørn Lomax
no flags Details | Diff
patch for monoBRistol.desktop (657 bytes, patch)
2012-07-04 07:16 EDT, Jørn Lomax
no flags Details | Diff

  None (edit)
Description Jørn Lomax 2012-06-21 06:00:38 EDT
Spec URL: http://jvlomax.fedorapeople.org/packeging/monobristol.spec
SRPM URL: http://jvlomax.fedorapeople.org/packeging/monobristol-0.60.3-1.fc17.src.rpm
Description: monobristol is a gui fronted for the sound emulator bristol written in mono 
Fedora Account System Username: jvlomax

This is only my second package, and the first one i have written completely from scratch, so there are probably plenty of errors
Comment 1 Jørn Lomax 2012-06-21 09:50:00 EDT
Updated .spec:http://jvlomax.fedorapeople.org/packeging/monobristol.spec
updated SRPM: http://jvlomax.fedorapeople.org/packeging/monobristol-0.60.3-3.fc17.src.rpm

rpmlint .spec:
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

rpmlint SRPM: 
monobristol.src: W: spelling-error Summary(en_US) bristol -> Bristol, bristle, bristly
monobristol.src: W: spelling-error %description -l en_US synthesisers -> synthesizers, synthesizer's, synthesizes
monobristol.src: W: spelling-error %description -l en_US subtractive -> subtracting, subtracted, subtract
1 packages and 0 specfiles checked; 0 errors, 3 warnings.
Comment 2 Orcan Ogetbil 2012-06-25 21:37:53 EDT
I got some notes to get things going.

* Package fails to build [1] with errors:
   RPM build errors:
      Macro %__isa_name has empty body
      Macro %__isa_bits has empty body
      File not found: /builddir/build/BUILDROOT/monobristol-0.60.3-3.fc18.noarch/usr/share/monobristol/monoBristol.exe

* Mono packages are not supposed to be noarch. Please follow the Mono packaging guidelines [2]. Note that you will need to suppress the empty debuginfo failure as explained in the guidelines.

* In the buid log, the .desktop file installation gives the warning. 

   monoBristol.desktop: error: (will be fatal in the future): value "monobristol.png" for key "Icon" in group "Desktop Entry" is an icon name with an extension, but there should be no extension as described in the Icon Theme Specification if the value is not an absolute path

Please fix this. This fix is upstreamable.


[1]   http://koji.fedoraproject.org/koji/taskinfo?taskID=4195234
[2]   http://fedoraproject.org/wiki/Packaging:Mono
Comment 3 Jørn Lomax 2012-06-26 09:14:13 EDT
Created attachment 594478 [details]
Patch for .desktop file
Comment 4 Jørn Lomax 2012-06-26 09:26:09 EDT
SPEC:http://jvlomax.fedorapeople.org/packeging/monobristol.spec
SRPM: http://jvlomax.fedorapeople.org/packeging/monobristol-0.60.3-4.fc17.src.rpm

rpmlint gives the following:
> hardcoded-library-path in /usr/lib/monobristol/monoBristol.exe

The reason is obvious, but i can't find a fitting macro. Should i create a macro in the spec file for this(%define/%global)?
Comment 5 Orcan Ogetbil 2012-06-26 22:47:38 EDT
Jorn, it is good practice in general 
- to take a look at the build log and the rpmlint output of all resulting packages (RPMs and the SRPM) for warning and error messages, 
- to build the package in mock, at least for rawhide, but better for rawhide and stable release(s) (If you do this you can rpmlint the result/ directory directly to evaluate all the resulting rpms inside. Two birds with one stone)
before you resubmit it during the review. Now,

* The fitting macro is %{_libdir}. You can find the standard macros in file /usr/lib/rpm/macros, and additional macro definition files inside /etc/rpm/

* rpmlint says (duplicates removed):
   monobristol.src: W: spelling-error %description -l en_US subtractive -> subtraction, subtract, attractive
   monobristol.x86_64: E: no-binary
   monobristol.x86_64: W: only-non-binary-in-usr-lib
   monobristol.x86_64: W: no-manual-page-for-binary monobristol

Let us ignore the above

   monobristol.src: W: spelling-error Summary(en_US) bristol -> Bristol
   monobristol.src: W: spelling-error %description -l en_US synthesisers -> synthesizers, synthesizer, synthesis

Please correct the above. Americanos use "synthesizers"

   monobristol.x86_64: E: non-executable-script /usr/share/applications/monoBristol.desktop 0644L /usr/bin/env

This should be fixed. I don't know why they ship a .desktop file with a shebang. Unless this is some very new .desktop file specification, please remove the shebang in your patch. I guess you will need to re-email (sorry).

* You are installing a .desktop file, which has a "Icons" tag. This means we better ship an icon with this package, so we don't end up with a naked menu entry. A good candidate is the monobristol.png file in the top source directory. Since this icon is 48x48 we shall install it at
   %{_datadir}/icons/hicolor/48x48/apps/

! BuildRequires: mono-devel monodevelop
   These don't seem necessary.

! You don't need the line
   mkdir -p %{buildroot}%{_datadir}/applications
it doesn't do any harm though, not a blocker.

? How do I use this application? None of the synths seem to open here.
Comment 6 Orcan Ogetbil 2012-06-26 22:50:50 EDT
* Note that since you will install an icon in %{_datadir}/icons/ , the relevant scriptlets need to be added to your specfile:
   http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache
Comment 7 Jørn Lomax 2012-06-27 07:54:08 EDT
I always run rpmlint and mock before submiting an updated package. I'll admit i only use the default mock configuration though. I'll make sure to run it on both rawhide and stable from now on. I looked everywhere for that macro, wiki, google and i grep'ed both /etc/rpm and /usr/lib/rpm/macro in hopes of finding it, no luck.


What i did not know was that i could run rpmlint on .rpm files, i thought i would get the same output as with the srpms, Now,

SPEC: http://jvlomax.fedorapeople.org/packeging/monobristol.spec
SRPMS:  http://jvlomax.fedorapeople.org/packeging/monobristol-0.60.3-5.fc17.src.rpm


rpmlint on the specfile comes out clean

rpmlint on SRPMS:
>[makerpm@Fafnir SPECS]$ rpmlint ../SRPMS/monobristol-0.60.3-5.fc17.src.rpm 
>monobristol.src: W: spelling-error Summary(en_US) bristol -> Bristol, bristle, >bristly
>monobristol.src: W: spelling-error %description -l en_US subtractive -> >subtracting, subtracted, subtract
>1 packages and 0 specfiles checked; 0 errors, 2 warnings.

rpmlint on the built package:
>monobristol.i686: W: spelling-error %description -l en_US subtractive -> >subtracting, subtracted, subtract
>monobristol.i686: E: no-binary
>monobristol.i686: W: only-non-binary-in-usr-lib
>monobristol.i686: W: no-manual-page-for-binary monobristol
>monobristol.i686: W: percent-in-%post

From what i have read on the wikipage, the no-binary error is solved by making the package a "noarch" package? Which  bring up another question i have. When i was searching for macros earlier, i found one titled %mono_arches which is the arches mono builds on. Should i be using this (i was originally, and was told not to anymore)

As for getting it to run? is bristol showing up at all after you have pressed a button, or is the monobristol gui not showing at all? I never got any sound from bristol without running it through JACK instead of alsa
Comment 8 Jørn Lomax 2012-06-27 07:57:39 EDT
Created attachment 594749 [details]
Updates -desktop.patch
Comment 9 Orcan Ogetbil 2012-06-28 00:00:42 EDT
Thanks, I made a full review on this. 

Before I get to that let me explain my experience. The monobristol gui shows up for me with bunch of buttons. When I click on a button a black window pops up, stays for a few seconds, then disappears. There I get error messages in the terminal such as
   "Failed to open audio device default"
When I switch from ALSA to Jack, the Bristol synth GUI shows up, but I don't get a sound. 
   
Here is the review:

! Please replace "bristol" with "Bristol" in Summary too.

* A package must own all directories that it creates or require another package that does [1]. But %{_libdir}/%{name} remains unowned. So you will need to replace
   %{_libdir}/%{name}/monoBristol.exe
by either
   %dir %{_libdir}/%{name}/
   %{_libdir}/%{name}/monoBristol.exe
or by simply
   %{_libdir}/%{name}/

* Similarly %{_datadir}/icons/hicolor/ remains unowned. But this directory already depends on a standard package. So we will need to 
   Requires: hicolor-icon-theme

! Not really necessary in this case, but in my opinion, it would be nice to explain what each patch does with a short comment.

* We got a new rpmlint
   monobristol.x86_64: W: percent-in-%post
Please replace the %>/dev/null with &>/dev/null

* The source code does not specify a license. There is a COPYING file for GPLv3 and the README file says the software is GPL. In particular [2] says
   A GPL or LGPL licensed package that lacks any statement of what version that it's licensed under in the source code/program output/accompanying docs is technically licensed under *any* version of the GPL or LGPL, not just the version in whatever COPYING file they include. 
Therefore we shall set
   License: GPL+
Yet it is best to confirm this with upstream.

[1] http://fedoraproject.org/wiki/Packaging/Guidelines#File_and_Directory_Ownership
[2] http://fedoraproject.org/wiki/Licensing
Comment 10 Jørn Lomax 2012-06-28 04:19:57 EDT
SPEC: http://jvlomax.fedorapeople.org/packeging/monobristol.spec
SRPMS: http://jvlomax.fedorapeople.org/packeging/monobristol-0.60.3-6.fc17.src.rpm

No interesting rpmlint for either the .spec or rpms
rpmlint on built package on the other hand:


monobristol.i686: W: spelling-error %description -l en_US subtractive -> subtracting, subtracted, subtract
monobristol.i686: E: no-binary
monobristol.i686: W: only-non-binary-in-usr-lib
monobristol.i686: W: no-manual-page-for-binary monobristol
monobristol.i686: W: percent-in-%post
1 packages and 0 specfiles checked; 1 errors, 4 warnings.

I think it's "%{_datadir}" that it doesn't like having in post, so i think it can be ignored

I still haven't heard back from upstream, so i'll just label it as GPL+

Bristol does not automatically create jack connections, so you might have to connected it yourself. I also have trouble with the volume on Bristol being low at times too. Not sure if that's a Bristol issue or my setup
Comment 11 Orcan Ogetbil 2012-06-28 08:34:44 EDT
Thanks for the quick update.

(In reply to comment #10)
> monobristol.i686: W: percent-in-%post
> I think it's "%{_datadir}" that it doesn't like having in post, so i think
> it can be ignored
> 

I think this is because of "%postrun". It needs to be "%postun".
Comment 12 Jørn Lomax 2012-06-30 08:13:37 EDT
SPEC:http://jvlomax.fedorapeople.org/packeging/monobristol.spec
SRPMS: http://jvlomax.fedorapeople.org/packeging/monobristol-0.60.3-6.fc17.src.rpm

Same rpmlint output as above, minus "percent-in-%post"
Comment 13 Orcan Ogetbil 2012-07-02 21:25:35 EDT
Hi Jorn,
Could you post an SRPM with the new Release tag? By the way, the above links are broken due to typo in "packeging".
Comment 14 Jørn Lomax 2012-07-03 09:58:34 EDT
SPEC: http://jvlomax.fedorapeople.org/packeging/monobristol.spec
SRPMS: http://jvlomax.fedorapeople.org/packaging/monobristol-0.60.3-7.fc17.src.rpm

I had changed the version in the spec and change log, just seem to have forgotten top build :(
Comment 15 Orcan Ogetbil 2012-07-04 01:09:55 EDT
I think this is good to go. One little request: Could you append
   X-Synthesis;X-Jack;
to the Categories entry in the .desktop file? This will allow monobristol to show up in the Multimedia->Creation->DigitalProcessing and Jack submenus inside the Applications menu, once you have the "multimedia-menus" package installed. Thanks!

----------------------------------------------
This package (monobristol) is APPROVED by oget
----------------------------------------------
Comment 16 Jørn Lomax 2012-07-04 07:16:11 EDT
Created attachment 596192 [details]
patch for monoBRistol.desktop
Comment 17 Jørn Lomax 2012-07-04 07:17:53 EDT
New Package SCM Request
=======================
Package Name: monobristol
Short Description: gui for bristol synthesizer written in mono 
Owners: jvlomax
Branches:f16 f17
InitialCC:
Comment 18 Orcan Ogetbil 2012-07-04 11:07:52 EDT
I wonder how the flag got messed up. I guess it is me whose name is on the fedora-review+. Will try to fix.
Comment 19 Gwyn Ciesla 2012-07-04 17:04:36 EDT
Git done (by process-git-requests).

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