Set LD_LIBRARY_PATH for installcheck

[gtrensch]

This PR fixes the following issue:

When building from source 'make installcheck' terminates with following errror:
error while loading shared libraries: libnestutil.so: cannot open shared object file: No such file or directory

related issues: #1520
suggested reviewers: @clinssen @lekshmideepu @jougs

[jougs]

Isn't the described problem already solved by the merge of #1513?

[heplesser]

@gtrensch Looks fine, but needs a fix for macOS. Why didn't we need to set this path in the past for installcheck to work?

[heplesser]

Isn't the described problem already solved by the merge of #1513?

On my Mac, everything builds and tests fine out of the box with master per 9ca796e. So the problem might indeed be solved already.

[clinssen]

The Travis build steps are, in general terms:

1. make install
2. source nest_vars
3. make installcheck

Thus, if someone locally does a make install && make installcheck, the paths might indeed not be set up correctly, as sourcing nest_vars is skipped. @gtrensch: please tell me if this understanding is correct.

[jougs]

@clinssen: I understand that you should source nest_vars.sh in order to get everything set up. But doesn't #1513 make sure that you don't have to set LD_LIBRARY_PATH (or any other environment variable really) in order to make NEST find its own convenience libraries and modules?

My point is that both PATH and PYTHONPATH have already been set previously and the addition of LD_LIBRARY_PATH by this PR should not actually be needed in my understanding.

[gtrensch]

@jougs @clinssen
The problem is that <cmake_install_prefix>/<cmake_installlibdir>/nest which now points to our shared objects is, without sourcing nest_vars.sh, not in search path anymore. This changes the behavior and causes the following sequence to fail:

cmake ...
make
make install
male installcheck

I think CMake has to propagate this. LD_LIBRARY_PATH was the natural choice.

[jougs]

@gtrensch: thanks for the clarification. I think in this case, this rather hints at a problem with setting the RUNPATH in the executable/Python module. @clinssen: can you please have a look at that?

[heplesser]

@gtrensch I found some problems with garbled text and a missing DY, see below.

[clinssen]

@gtrensch: I submitted a comment just as you pushed a fix, so the comment got collapsed. Could you still have a look? Thanks!

[gtrensch]

@clinssen
Since my comment on your comment also got collapsed now ..
I have removed the environment variable settings from CMakeLists.txt. Instead, I added the CMake variables that are needed to do_test.sh.in and let CMake them replace. Technically, it is the same, but more clearer.

[clinssen]

Excellent, thanks!

[jougs]

Alright. But can someone pretty please explain to me why this setting of LD_LIBRARY_PATH is needed, when we add all library locations to the RUNPATH, so the executable can find them automatically?

Unless someone convinces me how (our setting of RUNPATHis correct) AND (we must set LD_LIBRARY_PATH for things to work) can yield true, I still think this is just a workaround and not the right solution to the problem.

[heplesser]

This is getting confusing! With #1513, exactly the code proposed in this PR for do_tests.sh.in, has been added to nest_vars.sh.in and additionally travis_build.sh sources nest_vars.sh. Additionally, ProcessOptions.cmake sets the RPATH in the executable, and do_tests.sh sets the PYTHONPATH and PATH.

Therefore, in my interpretation,

1. There should be no need for this PR; we should definitely not have the same code in two places.
2. It should not be necessary to source nest_vars.sh in travis_build.sh, since the necessary PATHs should be encoded in the installed binary (make installcheck works out of the box on my system, except regressiontests/issue-779-1016.py may fail due to insufficient PATH #1547, but there PATH is the problem).
3. I do not understand the logic behind the RPATH for PyNEST @tammoippen ?

[jougs]

@heplesser: thanks for finally sharing my confusion.

1. There should be no need for this PR; we should definitely not have the same code in two places.

I agree from the bottom of my heart. This is what I actually meant so say from the start of this reviewing odyssey.

2. It should not be necessary to source nest_vars.sh in travis_build.sh, since the necessary PATHs should be encoded in the installed binary (make installcheck works out of the box on my system, except regressiontests/issue-779-1016.py may fail due to insufficient PATH #1547, but there PATH is the problem).

As far as I understand it, that sourcing only wants to set PATH and PYTHONPATH correctly for the subsequent run of make installcheck, which is probably a good thing^TM. The setting of LD_LIBRARY_PATH is merely a side-effect, which should not be needed. I'm kind of against having do_tests.sh set individual environment variables itself, as sourcing the right nest_vars.sh seems much more generic to me.

On a related note, I'm now also wondering about line 55 of travis_build.sh. It looks like that's also trying to make libraries available (but only to Python, in this case).

[heplesser]

As far as I understand it, that sourcing only wants to set PATH and PYTHONPATH correctly for the subsequent run of make installcheck, which is probably a _good thing_TM. The setting of LD_LIBRARY_PATH is merely a side-effect, which should not be needed. I'm kind of against having do_tests.sh set individual environment variables itself, as sourcing the right nest_vars.sh seems much more generic to me.

That sounds sensible. Just one question: if make installcheck in while running do_tests.sh sources nest_vars.sh, will the changes to environment variables "leak" to the shell from which make installcheck is called?

How about a new PR, replacing this one, which cleans up setting of environmental variables in do_tests.sh.in (and probably also run_examples.sh)?

On a related note, I'm now also wondering about line 55 of travis_build.sh. It looks like that's also trying to make libraries available (but only to Python, in this case).

@clinssen Could you comment on that (maybe not here ...)? The line looks very Linux-specific and comes right after a block differentiating between Linux and Darwin.

[clinssen]

3. I do not understand the logic behind the RPATH for PyNEST @tammoippen ?

$ORIGIN is a special keyword used not by CMake, but the linker. It refers to the path that the object is generated in. So for the nest executable, its $ORIGIN is bin, and the runpath needs to be with respect to that directory, i.e. ../lib/nest. For the pynestkernel.so object, which is installed to lib/python3.5/site-packages/nest, the runpath needs to be ../../../lib/nest, which corresponds to the same location.

It seems like pynestkernel.so itself is not included in any runpaths. I'm not sure whether this is necessary or not. In general, it seems that the runpath mechansm seems to be working correctly. I am working on a PR for removing redundant and unnecessary setting of paths.

[heplesser]

@gtrensch In light of the discussion above and #1561 recently created by @clinssen, would you agree to closing this PR?

[gtrensch]

Now that we have three pull requests addressing the same issues, which is at most confusing, please close this one.