Remove redundant setting of environment variables #1561

clinssen · 2020-04-28T18:58:26Z

Replaces #1545 and fixes #972.

…t_vars

clinssen · 2020-04-29T08:51:04Z

(Please see prior discussion in #1545.)

The underlying issue was as follows: the binaries could find the libraries (libnestkernel.so and so on) because their runpath includes ../lib/nest.

pynestkernel.so could find the libraries because its runpath includes ../../../lib/nest.

But the libraries themselves (e.g. libnestkernel.so) could not find the other libraries (e.g. libsli.so) because . (or, indirectly, ../../lib/nest, which from the perspective of these libraries points back to .), was missing. The latest commit adds the latter to the RPATH.

jougs · 2020-04-29T13:51:49Z

extras/travis_build.sh

@@ -108,7 +106,7 @@ if [ "$xLIBNEUROSIM" = "1" ] ; then
    CONFIGURE_LIBNEUROSIM="-Dwith-libneurosim=$HOME/.cache/libneurosim.install"
    chmod +x extras/install_csa-libneurosim.sh
    ./extras/install_csa-libneurosim.sh
-    export LD_LIBRARY_PATH=$HOME/.cache/csa.install/lib:$LD_LIBRARY_PATH
+    export LD_LIBRARY_PATH=$HOME/.cache/csa.install/lib${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}


extras/travis_build.sh

…ce nest_vars instead

jougs

I'll approve once the tests pass

CMakeLists.txt

gtrensch

@clinssen
This change causes make installcheck to behave like if it is running on Travis.
bash: /home/travis/build/nest/nest-simulator/result/bin/nest_vars.sh: No such file or directory
The user specified install directory seems to be ignored.

clinssen · 2020-05-04T09:23:57Z

I really appreciate all the feedback so far, but we are still in draft mode and I'm still experimenting with possible solutions!

@jougs: regrettably, it seems that the adding a custom dependency option will not work. The environment sourced in the "update_environment" custom CMake target does not carry over to the environment in which do_tests.sh will be run.

It looks like the invocation in "make installcheck" could be changed from just invoking do_tests.sh to invoking bash -c "source nest_vars.sh && do_tests.sh. I tested this and it works.

@heplesser: I can still add the dependency of installcheck on install without a problem.

heplesser · 2020-05-05T14:32:52Z

It looks like the invocation in "make installcheck" could be changed from just invoking do_tests.sh to invoking bash -c "source nest_vars.sh && do_tests.sh. I tested this and it works.

This looks like a clean solution.

@heplesser: I can still add the dependency of installcheck on install without a problem.

Good.

jougs · 2020-05-05T16:09:22Z

Just a quick heads up: I decided to resect my solution to #345 out of #1404 and make it into a new standalone PR. With that finished, do_test.sh will set paths according to what it is given on the command line instead of relying on the environment. This means that this PR does not have to handle that at all.

clinssen · 2020-05-05T21:44:08Z

Thanks @jougs, that simplifies things. Then hopefully you won't mind if I crudely insert a source ${CMAKE_INSTALL_FULL_BINDIR}/nest_vars.sh in do_tests.sh.in as a temporary solution.

There is one related issue that I'm now running into. After #1513, we are falsely advertising nest_vars.sh as being POSIX-compliant. Actually, for the MacOS check, we use $OSTYPE and the [[ ]] operators, which are not in /bin/sh. I have replaced this with a more compliant check using uname, so we wouldn't have to "downgrade" /bin/sh to /bin/bash in nest_vars.sh, and by extension in do_tests.sh.

@heplesser: adding a dependency of "installtarget" on "install" turns out to be not supported by CMake, because "install" is a built-in target. They've had an issue open for this for over 10 years (https://gitlab.kitware.com/cmake/cmake/-/issues/8438). There are no workarounds that I could find.

jougs · 2020-05-05T22:09:19Z

Thanks @jougs, that simplifies things. Then hopefully you won't mind if I crudely insert a source ${CMAKE_INSTALL_FULL_BINDIR}/nest_vars.sh in do_tests.sh.in as a temporary solution.

I do exactly the same in my refactoring branch 😄

There is one related issue that I'm now running into. After #1513, we are falsely advertising nest_vars.sh as being POSIX-compliant. Actually, for the MacOS check, we use $OSTYPE and the [[ ]] operators, which are not in /bin/sh. I have replaced this with a more compliant check using uname, so we wouldn't have to "downgrade" /bin/sh to /bin/bash in nest_vars.sh, and by extension in do_tests.sh.

This sounds good to me.

@heplesser: adding a dependency of "installtarget" on "install" turns out to be not supported by CMake, because "install" is a built-in target. They've had an issue open for this for over 10 years (https://gitlab.kitware.com/cmake/cmake/-/issues/8438). There are no workarounds that I could find.

Probably this is the reason why it is not implemented. Can you please add a corresponding comment, so we don't have to re-discover this each second year? Thanks!

…llcheck in CMake script

heplesser · 2020-05-06T06:23:26Z

@clinssen Let me know when this PR can be moved out of Draft state!

clinssen · 2020-05-06T08:21:42Z

@heplesser: it's ready for review! Could you double-check the new MacOS test in nest_vars.sh? Cheers!

heplesser

@clinssen Looks good!

jougs

Nice! Please see my inline comments for a more refined assessment ;-)

NP: Kid Loco – Confessions (Parov Stelar Remix)

CMakeLists.txt

cmake/ProcessOptions.cmake

extras/travis_build.sh

Co-authored-by: Jochen Martin Eppler <[email protected]>

jougs

Just a small one left.

cmake/ProcessOptions.cmake

jougs · 2020-05-07T12:58:40Z

@gtrensch, just to be on the safe side: Does this now solve all issues you described and observed?

Co-authored-by: Jochen Martin Eppler <[email protected]>

gtrensch

Approving.

remove deprecated setting of PYTHONPATH and redundant sourcing of nes…

6a3f1e8

…t_vars

clinssen marked this pull request as draft April 28, 2020 18:58

add debug statements to Travis build/test script

6f60d78

heplesser mentioned this pull request Apr 29, 2020

Set LD_LIBRARY_PATH for installcheck #1545

Closed

add debug statements to Travis build/test script; set runpath for libs

f985feb

remove debug statements to Travis build/test script

2cda395

jougs requested review from jougs and gtrensch April 29, 2020 13:18

jougs assigned clinssen Apr 29, 2020

jougs added I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. labels Apr 29, 2020

jougs added this to the NEST 3.0 milestone Apr 29, 2020

add documentation of RPATH logic in cmake script

71d8d64

jougs reviewed Apr 29, 2020

View reviewed changes

clinssen added 7 commits May 2, 2020 12:07

add DYLD_LIBRARY_PATH for MacOS

2335d03

remove setting environment variables in CMakeLists and do_tests; sour…

e1f5e33

…ce nest_vars instead

fix syntax in Travis build script

170327e

fix path to nest_vars.sh in CMakeLists

2c7f2a8

add debug message to CMakeLists

a1c5681

add debug message to CMakeLists

a259d6a

hard-code correct Travis path for testing

b1d52bf

jougs reviewed May 4, 2020

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

gtrensch requested changes May 4, 2020

View reviewed changes

clinssen added 3 commits May 5, 2020 23:10

move sourcing nest_vars.sh to do_tests.sh

f4ab0b6

make nest_vars.sh and do_tests.sh more portable across shells

20eb440

Merge remote-tracking branch 'origin/master' into paths_cleanup

6e916a2

clinssen added 2 commits May 6, 2020 00:10

fix CMake variable syntax in do_tests.sh.in

4715911

add comment about ensuring make install always runs before make insta…

f901877

…llcheck in CMake script

clinssen marked this pull request as ready for review May 6, 2020 08:20

heplesser approved these changes May 6, 2020

View reviewed changes

jougs reviewed May 6, 2020

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

cmake/ProcessOptions.cmake Show resolved Hide resolved

extras/travis_build.sh Show resolved Hide resolved

extras/travis_build.sh Show resolved Hide resolved

clinssen and others added 3 commits May 7, 2020 11:03

update comment in CMakeLists

9501009

Co-authored-by: Jochen Martin Eppler <[email protected]>

fix RPATH setting for MacOS

57a23b2

Merge remote-tracking branch 'clinssen/paths_cleanup' into paths_cleanup

87a6733

jougs requested changes May 7, 2020

View reviewed changes

cmake/ProcessOptions.cmake Outdated Show resolved Hide resolved

update comment in ProcessOptions.cmake

23de22d

Co-authored-by: Jochen Martin Eppler <[email protected]>

jougs approved these changes May 7, 2020

View reviewed changes

clinssen mentioned this pull request May 7, 2020

make installcheck reaches wrong testing code after sourcing nest_vars.sh for a different version #972

Closed

jougs mentioned this pull request May 7, 2020

Clean up handling of path-related environment variables #1586

Closed

gtrensch approved these changes May 11, 2020

View reviewed changes

heplesser merged commit 7361bad into nest:master May 11, 2020

jougs mentioned this pull request Jan 18, 2021

[WIP] Refactor testsuite setup #1071

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove redundant setting of environment variables #1561

Remove redundant setting of environment variables #1561

clinssen commented Apr 28, 2020 •

edited by jougs

Loading

clinssen commented Apr 29, 2020

jougs Apr 29, 2020

jougs left a comment

gtrensch left a comment •

edited

Loading

clinssen commented May 4, 2020

heplesser commented May 5, 2020

jougs commented May 5, 2020

clinssen commented May 5, 2020 •

edited by jougs

Loading

jougs commented May 5, 2020

heplesser commented May 6, 2020

clinssen commented May 6, 2020

heplesser left a comment

jougs left a comment

jougs left a comment

jougs commented May 7, 2020

gtrensch left a comment

Remove redundant setting of environment variables #1561

Remove redundant setting of environment variables #1561

Conversation

clinssen commented Apr 28, 2020 • edited by jougs Loading

clinssen commented Apr 29, 2020

jougs Apr 29, 2020

Choose a reason for hiding this comment

jougs left a comment

Choose a reason for hiding this comment

gtrensch left a comment • edited Loading

Choose a reason for hiding this comment

clinssen commented May 4, 2020

heplesser commented May 5, 2020

jougs commented May 5, 2020

clinssen commented May 5, 2020 • edited by jougs Loading

jougs commented May 5, 2020

heplesser commented May 6, 2020

clinssen commented May 6, 2020

heplesser left a comment

Choose a reason for hiding this comment

jougs left a comment

Choose a reason for hiding this comment

jougs left a comment

Choose a reason for hiding this comment

jougs commented May 7, 2020

gtrensch left a comment

Choose a reason for hiding this comment

clinssen commented Apr 28, 2020 •

edited by jougs

Loading

gtrensch left a comment •

edited

Loading

clinssen commented May 5, 2020 •

edited by jougs

Loading