Bug 571993 - Review Request: non-sequencer - A powerful, real-time, pattern-based MIDI sequencer
Summary: Review Request: non-sequencer - A powerful, real-time, pattern-based MIDI seq...
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard: StalledSubmitter
Depends On:
Blocks: FedoraAudio
TreeView+ depends on / blocked
 
Reported: 2010-03-09 23:02 UTC by Adam Huffman
Modified: 2015-12-05 23:07 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-12-05 23:07:56 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Adam Huffman 2010-03-09 23:02:58 UTC
Spec URL: http://verdurin.fedorapeople.org/reviews/non-sequencer/non-sequencer.spec
SRPM URL: http://verdurin.fedorapeople.org/reviews/non-sequencer/non-sequencer-0.0-1.20100131gitba94d2c354145.fc12.src.rpm
Description: 

The Non Sequencer is a powerful real-time, pattern-based MIDI
sequencer for Linux-released under the GPL. Filling the void left by
countless DAWs, piano-roll editors, and other purely performance based
solutions, it is a composition tool-one that transforms MIDI
music-making on Linux from a complex nightmare into a pleasurable,
efficient, and streamlined process.

Comment 1 Martin Gieseking 2010-03-29 12:51:17 UTC
Just a couple of quick comments:

- according to the source file headers, the license seems to be GPLv2+

- add the following line to the %prep section in order to disable early stripping of debuginfo:
sed -i '/^ifneq (\$(USE_DEBUG),yes)/,+4 d' Makefile
(alternatively, you can patch the Makefile)

- the configure command in the %build section should look like this:
%configure --enable-lash

- I think, the documentation is extensive enough to put it in a separate doc subpackage

- add TODO.mu to %doc


$ rpmlint /var/lib/mock/fedora-12-i386/result/*.rpm
non-sequencer.src:38: W: configure-without-libdir-spec
non-sequencer.src: W: invalid-url Source0: non-sequencer-20100131gitba94d2c354145.tar.bz2
non-sequencer-debuginfo.i686: E: empty-debuginfo-package
3 packages and 0 specfiles checked; 1 errors, 2 warnings.

Comment 2 Adam Huffman 2010-03-30 06:45:51 UTC
Thanks for taking a look.  I've uploaded a new version that addresses your comments:

http://verdurin.fedorapeople.org/reviews/non-sequencer/non-sequencer.spec

http://verdurin.fedorapeople.org/reviews/non-sequencer/non-sequencer-1.9.3-2.20100131gitba94d2c354145.fc12.src.rpm


$ rpmlint /var/lib/mock/fedora-12-x86_64/result/*.rpm
non-sequencer.src: W: invalid-url Source0: non-sequencer-20100131gitba94d2c354145.tar.bz2
4 packages and 0 specfiles checked; 0 errors, 1 warnings.

For me it comes up full-screen at startup - not sure whether that's a problem.

Comment 3 Martin Gieseking 2010-03-30 07:10:52 UTC
Now the doc directory is added twice. You can remove it from the base package. :)
I'll have a look at the rest later today.

Comment 5 Martin Gieseking 2010-03-30 20:38:42 UTC
I had a deeper look into the package and have to cancel my previous remark about the doc package. Sorry! The documentation files are expected to be installed with the base package. Otherwise the program's help doesn't work. 
Nonetheless, installing the doc files is a bit tricky. 

a) the make statement in %build must look like this:
make VERBOSE=1 SYSTEM_PATH=%{_datadir} DOCUMENT_PATH=%{_defaultdocdir}/%{name}-%{version}/doc/ %{?_smp_mflags}

It ensures compiling the final doc path into the program.

b) the %install section should contain the following lines:
sed -i "/\.html/d" Makefile
make install DESTDIR=$RPM_BUILD_ROOT

c) and finally the %doc line in the %files section:
%doc COPYING doc/

Now the help should be available through the help menu of the application.

Like you, I wasn't able to start non-sequencer in its own window. It always comes up full-screen. Maybe you should ask upstream whether that's a known issue.

Comment 7 Orcan Ogetbil 2010-06-28 04:16:01 UTC
Hi, I'll review this package but there are two important issues that need to be fixed first:

1- The build needs to be verbose. We want to see what flags are passed to the compiler. Currently the compiler invocations are silent because of the lines that start with @ in the Makefile. We need to patch or sed these out.

I am also not sure whether removing the whole "ifeq ($(USE_DEBUG),yes)" part from the Makefile is right. This also removes -DNDEBUG, and other flags which might be needed by the source, which I didn't check. Simply appending the optflags at the end should be sufficient. Something like
   sed -i 's|DNDEBUG|DNDEBUG $(CXXFLAGS)|' Makefile
Let me know if I am missing something.

2- Since this is a desktop application, we need a .desktop file, an icon and the scriptlets, as usual.

Comment 8 Orcan Ogetbil 2010-09-12 00:20:56 UTC
ping? are you still interested in packaging this?

Comment 9 Adam Huffman 2010-09-12 11:40:03 UTC
I am interested in packaging it.  However, the upstream activity seems rather erratic - no commits since January and there are bugs that have remained unfixed for quite a while.

If you think it's worthwhile proceeding with all that in mind, I'll do so.

Comment 10 Adam Huffman 2010-09-12 17:44:05 UTC
Put a new version addressing the compilation problems at:

http://verdurin.fedorapeople.org/reviews/non-sequencer/non-sequencer.spec

http://verdurin.fedorapeople.org/reviews/non-sequencer/non-sequencer-1.9.3-4.20100131gitba94d2c354145.fc13.src.rpm

I tried your suggestion regarding CXXFLAGS and that didn't seem to work as then the various include paths were not picked up.

The icon is a resized version of the logo file included in the documentation, for want of something better.

Comment 11 Orcan Ogetbil 2010-09-13 04:52:14 UTC
Thanks for the update. I will hopefully find time this week to have a look.

(In reply to comment #10)
> I tried your suggestion regarding CXXFLAGS and that didn't seem to work as then
> the various include paths were not picked up.
> 

My suggestion was a guide for you to start-up. It wasn't meant to be _the_ solution. I assume that you fixed the issue with the patch.

> The icon is a resized version of the logo file included in the documentation,
> for want of something better.

BuildRequires: ImageMagick and do the conversion in %build? Or at least some reference would be good that says where you got the image from (so that when someone looks into the specfile, he knows there are no license issues with the image).

Comment 12 Jason Tibbitts 2010-11-18 01:43:20 UTC
Did everything happen here?  It looks like oget was going to review, but that was a couple of months ago.

Comment 13 Orcan Ogetbil 2010-11-18 02:47:23 UTC
Thanks for the reminder Jason. This package slipped out of my mind. 

I will look into this by the end of this week. 

Adam, sorry for the delay.

Comment 14 Orcan Ogetbil 2010-11-22 04:15:24 UTC
Sorry for being late. Here is a more thorough review:

- rpmlint says
   non-sequencer.src:61: W: configure-without-libdir-spec
   non-sequencer.src: W: invalid-url Source0: non-sequencer-20100131gitba94d2c354145.tar.bz2
   non-sequencer.x86_64: W: no-manual-page-for-binary non-sequencer
These can be ignored. However
   non-sequencer.x86_64: W: no-documentation
needs a fix. You need to put at least the COPYING file in this package, also possibly the TODO file

! Currently, the -doc subpackage does not require the main package. Is this intentional? If this is what you want, the COPYING file must exist both in the main package and the -doc subpackage.

* Currently the source tarball contains the .git* stuff that we don't need. Those can be taken out. The tarball instructions tarball tell us to check out the latest git version. What we need is the instructions to generate the exact tarball. In case upstream updates their repo, current instructions will be incorrect.

* The %doc package should be noarch

* Please explain in the SPEC file where you got the other sources. Do they have copyrights? Also please explain in the SPEC file what the patch is for, and/or its upstream status.

* The directory %{_datadir}/%{name}/ must be owned by the main package. See
   http://fedoraproject.org/wiki/Packaging/Guidelines#File_and_Directory_Ownership

* You certainly don't need the BuildRequires: cmake git. I am not sure whether you really need libpng-devel libjpeg-devel. Somehow -lpng and -ljpeg flags are passed to the linking, but what adds them?

* Each package must consistently use macros. Please don't use plain /usr. Use %{_prefix} instead.

! Please span %description to 80 columns as much as possible. Currently it is set to 70.

* The icon is installed in the wrong place. It should be
    %{_datadir}/icons/hicolor/128x128/apps/
Note that you leave the scriptlets as is.

? sed -i "/\.html/d" Makefile
What is the above for? Can't that be moved to %prep?

Comment 15 Adam Huffman 2010-12-05 12:13:52 UTC
Sorry for the delay - will take a look in a couple of days.

This was one of my first packages so it's no wonder it needs a lot of work...

Comment 16 Orcan Ogetbil 2011-06-20 02:40:21 UTC
Hi Adam, what is going on with this package?

Comment 17 Adam Huffman 2011-06-21 21:42:52 UTC
I admit I've neglected this one, as upstream hasn't been active for quite a while.  However, I still think it's a useful package, so I'm hoping to have time to work on it at the weekend.

Comment 18 Brendan Jones 2012-03-20 17:49:27 UTC
Looks like upstream has pushed out a new release.

Comment 19 Brendan Jones 2012-04-11 17:21:41 UTC
Hi Adam,

can I close this so someone else can resubmit? This is being tracked by the Fedora Audio spin now.

regards,

Brendan

Comment 20 Adam Huffman 2012-04-11 18:41:58 UTC
Apologies for the delay - I'm in the middle of moving across the country and starting a new job.  That said, I do intend to complete the review, so I'm happy to continue unless someone else is desperate to do it.

I should have time to update to the new release at the weekend.

Comment 21 Adam Huffman 2012-04-15 22:35:50 UTC
There's a new version that includes the recent changes but still needs some work at:

http://verdurin.fedorapeople.org/reviews/non-sequencer/non-sequencer.spec

http://verdurin.fedorapeople.org/reviews/non-sequencer/non-sequencer-1.9.4-1.20120415git3a7d924.fc16.src.rpm

I'll contact upstream about the old FSF address errors.

Comment 22 Mario Blättermann 2012-11-25 19:25:27 UTC
May we assume this as still alive...? Otherwise, the request should be closed as FE-DEADREVIEW or replaced by a new request from someone else, marking the old one as a duplicate.

Comment 23 Adam Huffman 2012-12-02 22:48:48 UTC
No, I haven't given up, just haven't had much time for new packaging work lately.
I'll try to make some time over the next week, especially given the project seems to have been a lot more active lately.

Comment 24 Brendan Jones 2012-12-03 08:17:16 UTC
Adam, I'll build all of them through the non-daw pacakge. It actually makes sense to do it this way - I would like to dynamically link ntk (upsdtream has forked fltk!) rather than have it statically linked. I believe this is not quite finished upstream but should be soon.

Comment 25 James Hogarth 2015-12-04 01:35:08 UTC
Last comments were years ago.

Please provide an update.

If there is no response within a week as per policy this will be closed.

https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews

Comment 26 Adam Huffman 2015-12-05 23:07:56 UTC
This was actually packaged by Brendan as part of non-daw, so this review request can be closed.


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