*** Bug 1249094 has been marked as a duplicate of this bug. ***

# 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.

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.fc29.src.rpm


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

Hi Dominik,

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

Cheers,
Ankur

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.

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

Hi Dominik,

Thanks for the review.


> 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.

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).


> 
> 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

Done. 

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

Moved.

> 
> 4. 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.

> 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.

Done. They are both included in src/ivoc/fourier.cpp.


> 6. 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.fc29.src.rpm

Cheers,
Ankur

(In reply to Ankur Sinha (FranciscoD) from comment #6)
> (In reply to Dominik 'Rathann' Mierzejewski from comment #5)
[...]
> > 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.
> 
> 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.

[...]
> > 3. 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.

> > 4. 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.

[...]
> > 6. 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.

Updated spec/srpm:

https://ankursinha.fedorapeople.org/neuron/neuron.spec
https://ankursinha.fedorapeople.org/neuron/neuron-7.5-4.20181214git5687519.fc29.src.rpm

* 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

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.

(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

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/neuron

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

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

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

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

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.

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.

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

(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.