[Bug 1662526] New: Review Request: neuron - A flexible and powerful simulator of neurons and networks

https://bugzilla.redhat.com/show_bug.cgi?id=1662526

Bug ID: 1662526
           Summary: Review Request: neuron - A flexible and powerful
                    simulator of neurons and networks
           Product: Fedora
           Version: rawhide
          Hardware: All
                OS: Linux
            Status: NEW
         Component: Package Review
          Severity: medium
          Priority: medium
          Assignee: [e-mail]
          Reporter: [e-mail]
        QA Contact: [e-mail]
                CC: [e-mail]
  Target Milestone: ---
    Classification: Fedora

Spec URL: https://ankursinha.fedorapeople.org/neuron/neuron.spec
SRPM URL:
https://ankursinha.fedorapeople.org/neuron/neuron-7.5-1.20181214git5687519.f...

Description: 
NEURON is a simulation environment for modeling individual neurons and networks
of neurons. It provides tools for conveniently building, managing, and using
models in a way that is numerically sound and computationally efficient. It is
particularly well-suited to problems that are closely linked to experimental
data, especially those that involve cells with complex anatomical and
biophysical properties.

This package is currently built without GUI (iv) support.
It does not include MPI support.

Fedora Account System Username: ankursinha

[Bug 1662526] Review Request: neuron - A flexible and powerful simulator of neurons and networks

https://bugzilla.redhat.com/show_bug.cgi?id=1662526

Ankur Sinha (FranciscoD)  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |1276941 (fedora-neuro)

Referenced Bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=1276941
[Bug 1276941] Fedora NeuroImaging and NeuroScience tracking bug

[Bug 1662526] Review Request: neuron - A flexible and powerful simulator of neurons and networks

https://bugzilla.redhat.com/show_bug.cgi?id=1662526

--- Comment #1 from Ankur Sinha (FranciscoD)  ---
*** Bug 1249094 has been marked as a duplicate of this bug. ***

[Bug 1662526] Review Request: neuron - A flexible and powerful simulator of neurons and networks

https://bugzilla.redhat.com/show_bug.cgi?id=1662526

Ankur Sinha (FranciscoD)  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |1150105 (python-pynn)

Referenced Bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=1150105
[Bug 1150105] Review Request: python-pynn - Simulator-independent specification
of neuronal network models

[Bug 1662526] Review Request: neuron - A flexible and powerful simulator of neurons and networks

https://bugzilla.redhat.com/show_bug.cgi?id=1662526

Dominik 'Rathann' Mierzejewski  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|[e-mail]    |[e-mail]
              Flags|                            |fedora-review?

--- Comment #2 from Dominik 'Rathann' Mierzejewski  ---
# Let us tell the system where to find the sundials libraries. It is hard
coded.
sed -i
's|SUNDIALS_LIBDIRNAMES="${prefix}/lib"|SUNDIALS_LIBDIRNAMES="$MPI_LIB"|'
configure.ac

You're doing the above but not module-loading any MPI implementation
configuration, nor is any MPI library added to build dependencies.

Could you sort the BuildRequires and put each one on a separate line?

I would use 'rm' to delete Random123 files instead of doing that with a patch.
It'd make the patch much smaller.

[Bug 1662526] Review Request: neuron - A flexible and powerful simulator of neurons and networks

https://bugzilla.redhat.com/show_bug.cgi?id=1662526

--- Comment #3 from Ankur Sinha (FranciscoD)  ---
Hi Dominik,

Thanks for taking up the review. I've updated the spec/srpm:

* Sun Jan 06 2019 Ankur Sinha <ankursinha AT fedoraproject DOT org> -
7.5-2.20181214git5687519
- Put each BR on different line
- Remove unneeded comment
- Re-do random123 patch to only modify autotools files
- Remove random123 in prep

https://ankursinha.fedorapeople.org/neuron/neuron.spec
https://ankursinha.fedorapeople.org/neuron/neuron-7.5-2.20181214git5687519.f...

This is a serial (non MPI) build of NEURON. The MPI builds, and python bindings
require NEURON to be installed already. Upstream builds them as
post-installation hooks. So, I'll package them up as different packages that
will require this one as a BR. I've added this as a comment at the start of the
spec.

Cheers,
Ankur

[Bug 1662526] Review Request: neuron - A flexible and powerful simulator of neurons and networks

https://bugzilla.redhat.com/show_bug.cgi?id=1662526

--- Comment #4 from Ankur Sinha (FranciscoD)  ---
Hi Dominik,

Thanks for taking up the review. Would you have time to complete this review
sometime this week please?

Cheers,
Ankur

[Bug 1662526] Review Request: neuron - A flexible and powerful simulator of neurons and networks

https://bugzilla.redhat.com/show_bug.cgi?id=1662526

--- Comment #5 from Dominik 'Rathann' Mierzejewski  ---
Hi, Ankur.
The new version looks better.

1. I read your comment at the top of the spec:
> # This is a serial build of NEURON without Python or other bindings.
> # Both the MPI builds and Python bindings require NEURON to be already
> # installed in the system---they are build as post-installation hooks. So, we
> # first package a serial version of NEURON and then package those separately
> # after using this package as a BR

Are you sure this is really the case? Looking at src/nrnmpi/Makefile.am,
it seems that specifying the path to the just-built libraries should suffice
to build it after the serial version is built.

2. I noticed that many source files under src/ have executable permission
bits. I'd suggest something like this in %prep:

find src -type f -executable ! -name "*.sh" | xargs chmod -x

3. The header file in -devel subpackage:
# should this be here?!
%{_libdir}/nrnconf.h

It shouldn't. Please put it in %{_includedir} .

4. It looks like there's more bundled code:
src/gnu - libstdc++-devel (?)
src/readline - readline-devel

5. You might want to change the License: tag to GPLv3, because two files
   are GPLv3:
./src/ivoc/nrngsl_hc_radix2.c: GPL (v3 or later)
./src/ivoc/nrngsl_real_radix2.c: GPL (v3 or later)

Do check if they're compiled and included in the binaries.

6. README.md isn't very useful for the installed package as it talks only
   about installing from source.

[Bug 1662526] Review Request: neuron - A flexible and powerful simulator of neurons and networks

https://bugzilla.redhat.com/show_bug.cgi?id=1662526

--- Comment #6 from Ankur Sinha (FranciscoD)  ---
(In reply to Dominik 'Rathann' Mierzejewski from comment #5)
> Hi, Ankur.
> The new version looks better.

Hi Dominik,

Thanks for the review.
> I read your comment at the top of the spec:
> > # This is a serial build of NEURON without Python or other bindings.
> > # Both the MPI builds and Python bindings require NEURON to be already
> > # installed in the system---they are build as post-installation hooks. So, we
> > # first package a serial version of NEURON and then package those separately
> > # after using this package as a BR
>
> Are you sure this is really the case? Looking at src/nrnmpi/Makefile.am,
> it seems that specifying the path to the just-built libraries should suffice
> to build it after the serial version is built.

I did try that, but I wasn't able to get it to work. nrnmpi depends on other
bits that are not in subdirs of the nrnmpi directory. So, the dep chain isn't
set up correctly. From what I could find, there's a way to force the dependent
targets that are not in subdirs. (The other alternative is to drop the subdir
method of using Autotools and re-do the project.) So, it seems that this why
upstream build nrnmpi in the post-install hook (and not post-build).
> I noticed that many source files under src/ have executable permission
>
> bits. I'd suggest something like this in %prep:
>
> find src -type f -executable ! -name "*.sh" | xargs chmod -x

Done.
> The header file in -devel subpackage:
>
> # should this be here?!
> %{_libdir}/nrnconf.h
>
> It shouldn't. Please put it in %{_includedir} .

Moved.
> It looks like there's more bundled code:
>
> src/gnu - libstdc++-devel (?)
> src/readline - readline-devel

Unfortunately it seems to be a version from 1988(!), and libstdc++ has changed
so much since then that I cannot even find the bundled headers. I've filed a
ticket upstream. For the time being, though, I've included it. Upstream changed
the soname, so it won't conflict with the upstream version we provide in
Fedora.
> You might want to change the License: tag to GPLv3, because two files
> are GPLv3:
>
> ./src/ivoc/nrngsl_hc_radix2.c: GPL (v3 or later)
> ./src/ivoc/nrngsl_real_radix2.c: GPL (v3 or later)
>
> Do check if they're compiled and included in the binaries.

Done. They are both included in src/ivoc/fourier.cpp.
> README.md isn't very useful for the installed package as it talks only
> about installing from source.

Removed.

Updated spec/srpm:
https://ankursinha.fedorapeople.org/neuron/neuron.spec
https://ankursinha.fedorapeople.org/neuron/neuron-7.5-3.20181214git5687519.f...

Cheers,
Ankur

[Bug 1662526] Review Request: neuron - A flexible and powerful simulator of neurons and networks

https://bugzilla.redhat.com/show_bug.cgi?id=1662526

--- Comment #7 from Dominik 'Rathann' Mierzejewski  ---
(In reply to Ankur Sinha (FranciscoD) from comment #6)
> (In reply to Dominik 'Rathann' Mierzejewski from comment #5)

[...]
> > I read your comment at the top of the spec:
> > > # This is a serial build of NEURON without Python or other bindings.
> > > # Both the MPI builds and Python bindings require NEURON to be already
> > > # installed in the system---they are build as post-installation hooks. So, we
> > > # first package a serial version of NEURON and then package those separately
> > > # after using this package as a BR
> >
> > Are you sure this is really the case? Looking at src/nrnmpi/Makefile.am,
> > it seems that specifying the path to the just-built libraries should suffice
> > to build it after the serial version is built.
>
> I did try that, but I wasn't able to get it to work. nrnmpi depends on other
> bits that are not in subdirs of the nrnmpi directory. So, the dep chain
> isn't set up correctly. From what I could find, there's a way to force the
> dependent targets that are not in subdirs. (The other alternative is to drop
> the subdir method of using Autotools and re-do the project.) So, it seems
> that this why upstream build nrnmpi in the post-install hook (and not
> post-build).

Alright. Could you open a ticket with upstream about this, too? It would be
better if MPI variants could be built in the same build process.

[...]
> > The header file in -devel subpackage:
> >
> > # should this be here?!
> > %{_libdir}/nrnconf.h
> >
> > It shouldn't. Please put it in %{_includedir} .
>
> Moved.

You seem to have left the comment which is no longer relevant.
> > It looks like there's more bundled code:
> >
> > src/gnu - libstdc++-devel (?)
> > src/readline - readline-devel
>
> Unfortunately it seems to be a version from 1988(!), and libstdc++ has
> changed so much since then that I cannot even find the bundled headers. I've
> filed a ticket upstream. For the time being, though, I've included it.
> Upstream changed the soname, so it won't conflict with the upstream version
> we provide in Fedora.

OK, thanks for opening the upstream ticket.

[...]
> > README.md isn't very useful for the installed package as it talks only
> > about installing from source.
>
> Removed.

Good, but the changelog entry doesn't explain why you did it, which might be
useful.
> Updated spec/srpm:
> https://ankursinha.fedorapeople.org/neuron/neuron.spec

# Do we need the static libraries?
%files static
%license Copyright
%doc README.md
%{_libdir}/libivoc.la
%{_libdir}/libmemacs.la
%{_libdir}/libmeschach.la
%{_libdir}/libneuron_gnu.la
%{_libdir}/libnrniv.la
%{_libdir}/libnrnmpi.la
%{_libdir}/libnrnoc.la
%{_libdir}/liboc.la
%{_libdir}/libocxt.la
%{_libdir}/libsparse13.la
%{_libdir}/libscopmath.la
%{_libdir}/libivos.la

If it's just .la (libtool archive), then these are not enough, you should have
.a files as well. So either ship those, too or don't ship .la files at all
unless they're dlopened by neuron consumers.

[Bug 1662526] Review Request: neuron - A flexible and powerful simulator of neurons and networks

https://bugzilla.redhat.com/show_bug.cgi?id=1662526

--- Comment #8 from Ankur Sinha (FranciscoD)  ---
Updated spec/srpm:

https://ankursinha.fedorapeople.org/neuron/neuron.spec
https://ankursinha.fedorapeople.org/neuron/neuron-7.5-4.20181214git5687519.f...

* Thu Jan 31 2019 Ankur Sinha <ankursinha AT fedoraproject DOT org> -
7.5-4.20181214git5687519
- Remove libtool archives
- Remove stray comment
- Improve previous changelog

I'm already in conversation with upstream about multiple issues (arch
support/sundials/libstdc++) so I'll bring up the build issue there too. I don't
want to pepper them with too many issues all at once.

Cheers,
Ankur

[Bug 1662526] Review Request: neuron - A flexible and powerful simulator of neurons and networks

https://bugzilla.redhat.com/show_bug.cgi?id=1662526

Dominik 'Rathann' Mierzejewski  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|fedora-review?              |fedora-review+

--- Comment #9 from Dominik 'Rathann' Mierzejewski  ---
Wouldn't it make sense to move
%{_includedir}/nrnconf.h
into
%{_includedir}/%{tarname}

I don't know where packages that will BuildRequires: neuron-devel expect these
files to be, so I'll leave this up to you to decide.

Also, I wouldn't define a macro that is longer than what it expands into, i.e.
I'd just use "nrn" everywhere instead of %{tarname}.

None of the above are blockers and the package looks good otherwise, so
approved.

[Bug 1662526] Review Request: neuron - A flexible and powerful simulator of neurons and networks

https://bugzilla.redhat.com/show_bug.cgi?id=1662526

Ankur Sinha (FranciscoD)  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |POST

--- Comment #10 from Ankur Sinha (FranciscoD)  ---
(In reply to Dominik 'Rathann' Mierzejewski from comment #9)
> Wouldn't it make sense to move
> %{_includedir}/nrnconf.h
> into
> %{_includedir}/%{tarname}
>
> I don't know where packages that will BuildRequires: neuron-devel expect
> these files to be, so I'll leave this up to you to decide.

I've not seen too many refer to that header so I'll leave it for the time being
and change it later if required.
> Also, I wouldn't define a macro that is longer than what it expands into,
> i.e. I'd just use "nrn" everywhere instead of %{tarname}.

Ah, yes XD
> None of the above are blockers and the package looks good otherwise, so
> approved.

Thanks very much! SCM requested:
https://pagure.io/releng/fedora-scm-requests/issue/9737

Cheers,
Ankur

[Bug 1662526] Review Request: neuron - A flexible and powerful simulator of neurons and networks

https://bugzilla.redhat.com/show_bug.cgi?id=1662526

--- Comment #11 from Igor Gnatenko  ---
(fedscm-admin):  The Pagure repository was created at
https://src.fedoraproject.org/rpms/neuron

[Bug 1662526] Review Request: neuron - A flexible and powerful simulator of neurons and networks

https://bugzilla.redhat.com/show_bug.cgi?id=1662526

Fedora Update System  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|POST                        |MODIFIED

--- Comment #12 from Fedora Update System  ---
neuron-7.5-4.20181214git5687519.fc28 has been submitted as an update to Fedora
28. https://bodhi.fedoraproject.org/updates/FEDORA-2019-aef28e1e3d

[Bug 1662526] Review Request: neuron - A flexible and powerful simulator of neurons and networks

https://bugzilla.redhat.com/show_bug.cgi?id=1662526

Fedora Update System  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|MODIFIED                    |ON_QA

--- Comment #14 from Fedora Update System  ---
neuron-7.5-4.20181214git5687519.fc29 has been pushed to the Fedora 29 testing
repository. If problems still persist, please make note of it in this bug
report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here:
https://bodhi.fedoraproject.org/updates/FEDORA-2019-214c9d5a07

[Bug 1662526] Review Request: neuron - A flexible and powerful simulator of neurons and networks

https://bugzilla.redhat.com/show_bug.cgi?id=1662526

--- Comment #13 from Fedora Update System  ---
neuron-7.5-4.20181214git5687519.fc29 has been submitted as an update to Fedora
29. https://bodhi.fedoraproject.org/updates/FEDORA-2019-214c9d5a07

[Bug 1662526] Review Request: neuron - A flexible and powerful simulator of neurons and networks

https://bugzilla.redhat.com/show_bug.cgi?id=1662526

--- Comment #15 from Fedora Update System  ---
neuron-7.5-4.20181214git5687519.fc28 has been pushed to the Fedora 28 testing
repository. If problems still persist, please make note of it in this bug
report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here:
https://bodhi.fedoraproject.org/updates/FEDORA-2019-aef28e1e3d

[Bug 1662526] Review Request: neuron - A flexible and powerful simulator of neurons and networks

https://bugzilla.redhat.com/show_bug.cgi?id=1662526

Fedora Update System  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ON_QA                       |CLOSED
         Resolution|---                         |ERRATA
        Last Closed|                            |2019-02-27 01:15:40

--- Comment #16 from Fedora Update System  ---
neuron-7.5-4.20181214git5687519.fc28 has been pushed to the Fedora 28 stable
repository. If problems still persist, please make note of it in this bug
report.

[Bug 1662526] Review Request: neuron - A flexible and powerful simulator of neurons and networks

https://bugzilla.redhat.com/show_bug.cgi?id=1662526

--- Comment #17 from Fedora Update System  ---
neuron-7.5-4.20181214git5687519.fc29 has been pushed to the Fedora 29 stable
repository. If problems still persist, please make note of it in this bug
report.

[Bug 1662526] Review Request: neuron - A flexible and powerful simulator of neurons and networks

https://bugzilla.redhat.com/show_bug.cgi?id=1662526

Christophe Fergeau  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #18 from Christophe Fergeau  ---
Fwiw, the /usr/bin/oc file this package installs conflicts with a binary from
origin-clients, I've filed rhbz#1696118 for that.

[Bug 1662526] Review Request: neuron - A flexible and powerful simulator of neurons and networks

https://bugzilla.redhat.com/show_bug.cgi?id=1662526

--- Comment #19 from Dominik 'Rathann' Mierzejewski  ---
(In reply to Christophe Fergeau from comment #18)
> Fwiw, the /usr/bin/oc file this package installs conflicts with a binary
> from origin-clients, I've filed rhbz#1696118 for that.

Indeed, sorry for missing that in review. I've just hit it myself by accident.