Bug 484591 - Review Request: muse - Midi/Audio Music Sequencer
Review Request: muse - Midi/Audio Music Sequencer
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Hans de Goede
Fedora Extras Quality Assurance
:
: 483301 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-08 15:17 EST by Orcan Ogetbil
Modified: 2009-03-09 18:46 EDT (History)
8 users (show)

See Also:
Fixed In Version: 1.0-0.4.rc1.fc10
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-03-09 18:46:10 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
hdegoede: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Orcan Ogetbil 2009-02-08 15:17:03 EST
Spec URL: http://oget.fedorapeople.org/review/muse.spec
SRPM URL: http://oget.fedorapeople.org/review/muse-1.0-0.2.rc1.fc10.src.rpm
Description: 
MusE is a MIDI/Audio sequencer with recording and editing capabilities. It can
perform audio effects like chorus/flanger in realtime via LASH and it supports
Jack and ALSA interfaces. MusE aims to be a complete multitrack virtual studio
for Linux.

rpmlint is silent.

Epoch: 1 is there for PlanetCCRMA compatibility. See AudioCreation SIG wiki for
informations about PlanetCCRMA integration:
https://fedoraproject.org/wiki/AudioCreation

NOTE:
This Review Request have been opened before (bug #483301) but someone with personality issues have closed that bug. Let us hope he doesn't taint this one.
Comment 1 Jens Petersen 2009-02-09 20:26:05 EST
*** Bug 483301 has been marked as a duplicate of this bug. ***
Comment 2 Hans de Goede 2009-02-10 03:08:09 EST
(In reply to comment #0)
> This Review Request have been opened before (bug #483301) but someone with
> personality issues have closed that bug. Let us hope he doesn't taint this one.

Orcan, I appreciate your Fedora work, and I can understand that you have issues with a certain person. But you *MUST* stop offending other Fedora contributors. Stop writing: "please do not post in my bugs" and certainly don't accuse other contributors of "personality issues". You may disagree on technical grounds with Ralf and so do others of us from time to time. But *YOUR* behaviour here is completely unacceptable, you need to *STOP* this immediately!

There is a huge difference between discussing about technical issues and sometimes being rather stubborn about them (Ralf) and between anti-social behaviour. it is *YOU* who is way out of line here not Ralf.
Comment 3 Hans de Goede 2009-02-10 03:14:23 EST
With that said, and hopefully made very clear! I would like to review this package as I think it is great that someone is working on moving packages from planetccrma into Fedora proper.

The Epoch is fine, but with the other 2 issues I have to side with Ralf, using "AutoProv: no" is not acceptable, your package will miss essential requires and adding them manually is error prone and will break when the soname's of libraries you use change (and you do a rebuild). The correct solution here is to use filtering of the generated provides as explained in the previous review request.

Also the warnings are serious and need fixing, Ralf has already explained how (replace "%d" with "%zd"), let me know if there are other warnings which you need help fixing.

Can you please do a new revision with these 2 issues fixed? then I'll do a full review.
Comment 4 Kevin Kofler 2009-02-10 03:42:02 EST
Oops, the solution I provided for filtering is faulty, see:
https://bugzilla.redhat.com/show_bug.cgi?id=484837

%{__sed} '/\.so$/d' needs to be: %{__sed} '/\.so\(()(64bit)\)\?$/d'
Comment 5 Mamoru TASAKA 2009-02-10 03:54:18 EST
(Note that I mailed to Orcan privately...)

(In reply to comment #3)
> The correct solution here is
> to use filtering of the generated provides as explained in the previous review
> request.
- As Kevin said in the comment 4 (and as I reported on the bug
  484837), the filtering method proposed on the previous review request
  is not correct on x86_64


(In reply to comment #4)
> Oops, the solution I provided for filtering is faulty, see:
> https://bugzilla.redhat.com/show_bug.cgi?id=484837
> 
> %{__sed} '/\.so$/d' needs to be: %{__sed} '/\.so\(()(64bit)\)\?$/d'
- This time it is not correct on i386.

> Also the warnings are serious and need fixing, Ralf has already explained how
> (replace "%d" with "%zd"), let me know if there are other warnings which you
> need help fixing.

- Would you explain why you "particularly" mention these warnings?
  These warnings all comes from (f)printf with passing incorrect format,
  however I have already seen in other review requests that
  many warnings which seemed more and more critical than this
  (like one related to implicit function declaration you mentioned
   before) were just ignored.
  Of course I admit that fixing these warnings are desirable, however
  I am against making these warning the "blocker" for this review
  request.
  If you surely think these warnings are blockers, would you propose
  fedora-packaging-list about what warnings should be treated as
  review blockers? 
  It is really appreciated because I had repeatedly been asked
  "is it a blocker??"
Comment 6 Hans de Goede 2009-02-10 04:06:34 EST
(In reply to comment #5)
> > Also the warnings are serious and need fixing, Ralf has already explained how
> > (replace "%d" with "%zd"), let me know if there are other warnings which you
> > need help fixing.
> 
> - Would you explain why you "particularly" mention these warnings?
>   These warnings all comes from (f)printf with passing incorrect format,
>   however I have already seen in other review requests that
>   many warnings which seemed more and more critical than this
>   (like one related to implicit function declaration you mentioned
>    before) were just ignored.
>   Of course I admit that fixing these warnings are desirable, however
>   I am against making these warning the "blocker" for this review
>   request.
>   If you surely think these warnings are blockers, would you propose
>   fedora-packaging-list about what warnings should be treated as
>   review blockers? 
>   It is really appreciated because I had repeatedly been asked
>   "is it a blocker??"

As Ralf has demonstrated these warnings are usually a real issue, which only shows on 64 bit systems. But usually these are in debug printf's and thus quite often people don't care about fixing them. I agree this is not something which we normally block reviews on. So I wont do that in this case either. Still this something which really should be fixed, so consider this a should fix item.
Comment 7 Kevin Kofler 2009-02-10 04:27:36 EST
> > %{__sed} '/\.so$/d' needs to be: %{__sed} '/\.so\(()(64bit)\)\?$/d'
> - This time it is not correct on i386.

It is, see bug 484837.
Comment 8 Ralf Corsepius 2009-02-10 04:50:19 EST
(In reply to comment #6)

> As Ralf has demonstrated these warnings are usually a real issue, which only
> shows on 64 bit systems. But usually these are in debug printf's and thus quite
> often people don't care about fixing them. I agree this is not something which
> we normally block reviews on. So I wont do that in this case either. Still this
> something which really should be fixed, so consider this a should fix item.

That's why I had labeled it "SHOULD".

It's a classic of the 32bit->64bit portability problems.

It is broken code which is guaranteed to be non-functional under certain conditions. The only question is when this bug will hit, not if this will this will hit.

Due to the nature of size_t, such kind of bugs show when some variable will exceed sizeof(int) (4GB) or when some computations will be performed on size_t's.

The former will usually only cause visible problems in "big use-cases", while the later usually shows as "corrupt output".
Comment 9 Mamoru TASAKA 2009-02-10 04:52:30 EST
(In reply to comment #7)
> > > %{__sed} '/\.so$/d' needs to be: %{__sed} '/\.so\(()(64bit)\)\?$/d'
> > - This time it is not correct on i386.
> 
> It is, see bug 484837.

Yes, I missed the question mark.

By the way this package does no build on dist-f11 (koji)
One issue is g++44 issue, however there is another.
configure says:
-----------------------------------------------
checking for DocBook V4.1... 
no
configure: WARNING: DocBook 4.1 DTD not found or not usable - documentation will not be built
-----------------------------------------------
config.log says:
-----------------------------------------------
configure:19341: checking for DocBook V4.1
onsgmls:/usr/share/sgml/docbook/xml-dtd-4.1.2-1.0-42.fc11/ent/iso-amsa.ent:8:19:E: "X21B6" is not a function name
onsgmls:/usr/share/sgml/docbook/xml-dtd-4.1.2-1.0-42.fc11/ent/iso-amsa.ent:9:19:E: "X21B7" is not a function name
onsgmls:/usr/share/sgml/docbook/xml-dtd-4.1.2-1.0-42.fc11/ent/iso-amsa.ent:10:17:E: "X21D3" is not a function name
onsgmls:/usr/share/sgml/docbook/xml-dtd-4.1.2-1.0-42.fc11/ent/iso-amsa.ent:11:18:E: "X21CA" is not a function name
onsgmls:/usr/share/sgml/docbook/xml-dtd-4.1.2-1.0-42.fc11/ent/iso-amsa.ent:12:18:E: "X21C3" is not a function name
onsgmls:/usr/share/sgml/docbook/xml-dtd-4.1.2-1.0-42.fc11/ent/iso-amsa.ent:13:18:E: "X21C2" is not a function name
onsgmls:/usr/share/sgml/docbook/xml-dtd-4.1.2-1.0-42.fc11/ent/iso-amsa.ent:14:18:E: "X21DA" is not a function name
onsgmls:/usr/share/sgml/docbook/xml-dtd-4.1.2-1.0-42.fc11/ent/iso-amsa.ent:15:17:E: "X219E" is not a function name
onsgmls:/usr/share/sgml/docbook/xml-dtd-4.1.2-1.0-42.fc11/ent/iso-amsa.ent:16:18:E: "X21C7" is not a function name
........................
........................
-------------------------------------------------
I am not sure what is causing this, because
- with dist-f10-updates-candidate, checking for DocBook V4.1...
  returns yes
- My local build passes this point
Comment 10 Orcan Ogetbil 2009-02-10 19:31:52 EST
Thank you Hans, for your concern. But I do have objections/corrections:

(In reply to comment #2)
> Orcan, I appreciate your Fedora work, and I can understand that you have issues
> with a certain person. 

I am trying to be a productive member for our community. Having started about 4 months ago (thanks to Mamoru), I made about 10 packages here at Fedora and a few at RPMfusion. I have done more than 40 package reviews and I try to participate on certain SIGs. So, you're welcome as every other community member is.

That being said, I have to make it clear that it is not me who has issues with a certain person. 

> But you *MUST* stop offending other Fedora contributors.
> Stop writing: "please do not post in my bugs" and certainly don't accuse other
> contributors of "personality issues". 

This is a free world, and I have all the right to make such "requests". The keyword here is "requests" which should not be confused with "orders".

I did not accuse anyone with anything. Having a personality issue is not a crime. It is a psychological disorder. One can not accuse another with a disorder.

I just stated a fact.

> You may disagree on technical grounds
> with Ralf and so do others of us from time to time. But *YOUR* behaviour here
> is completely unacceptable, you need to *STOP* this immediately!
> 
> There is a huge difference between discussing about technical issues and
> sometimes being rather stubborn about them (Ralf) and between anti-social
> behaviour. it is *YOU* who is way out of line here not Ralf.

Am I anti-social? I am the one who is in contact with end-users. I am the one who is in contact with upstream developers. I am the one who is in contact with fellow contributors, minus those who have certain disorders. 

Well, if excluding a few people from my contact list to keep myself sane and productive is considered anti-socialism, then I'm anti-social. And Fedora does not exclude anti-social people like me. At least, as far as I know, there is no guideline against them.

Anti-social me has had conversations with many people at #fedora-devel and learned about the enemies this disordered person made in our community. Certainly, I was not his first target. But I do hope that I will be the last.


If you still think that my behavior is unacceptable, please allow me some time to consider for myself whether it is worth to try to change myself in these matters. This is what I am and what I have been.


I would like to thank to everyone who have personally emailed me or messaged me by other means. I have replied to some of these messages and I will reply to the remaining ones ASAP.

About the package... I don't know... I do feel discouraged... Maybe I need some more time.
Comment 11 Orcan Ogetbil 2009-02-23 02:30:16 EST
I decided to finish the package.

SPEC: http://oget.fedorapeople.org/review/muse.spec
SRPM: http://oget.fedorapeople.org/review/muse-1.0-0.3.rc1.fc10.src.rpm

Changelog: 1:1.0-0.3.rc1
- Handle the Provides list within the SPEC file
- Add gcc-4.4 patch
- Fix size_t warnings
- Explain the various licenses

I did not get the DocBook issue in the rawhide build. Maybe it is fixed(?)
Comment 12 Hans de Goede 2009-02-23 04:24:12 EST
(In reply to comment #11)
> I decided to finish the package.
> 
> SPEC: http://oget.fedorapeople.org/review/muse.spec
> SRPM: http://oget.fedorapeople.org/review/muse-1.0-0.3.rc1.fc10.src.rpm
> 
> Changelog: 1:1.0-0.3.rc1
> - Handle the Provides list within the SPEC file
> - Add gcc-4.4 patch
> - Fix size_t warnings
> - Explain the various licenses
> 
> I did not get the DocBook issue in the rawhide build. Maybe it is fixed(?)

It seems so.

I did a full review and it looks very good now! Approved :)
Comment 13 Mamoru TASAKA 2009-02-23 11:55:34 EST
One comment:
- Please update icon cache script when importing to Fedora CVS.

  ref:
  https://www.redhat.com/archives/fedora-devel-list/2009-February/msg01604.html
  https://fedoraproject.org/wiki/PackagingDrafts/Icon_Cache
Comment 14 Orcan Ogetbil 2009-02-23 12:25:07 EST
Thanks everyone, 
Hans, for the review
Mamoru and Ralf, for their comments
Kevin, for the provides script
Fernando, for the initial SPEC file

Mamoru,
I'll update the script. Thanks again.


I'm adding Fernando to the owners. Please let me know if there's anyone else who wants to maintain or audit this package

New Package CVS Request
=======================
Package Name: muse
Short Description: Midi/Audio Music Sequencer
Owners: oget nando
Branches: F-10
InitialCC:
Comment 15 Kevin Fenzi 2009-02-24 15:49:19 EST
cvs done.
Comment 16 Fedora Update System 2009-02-24 17:11:11 EST
muse-1.0-0.4.rc1.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/muse-1.0-0.4.rc1.fc10
Comment 17 Fedora Update System 2009-02-25 11:23:59 EST
muse-1.0-0.4.rc1.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update muse'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-2070
Comment 18 Fedora Update System 2009-03-09 18:46:04 EDT
muse-1.0-0.4.rc1.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

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