Clean up handling of path-related environment variables #1578

jougs · 2020-05-04T20:50:57Z

This fixes #1586 and fixes #1331 by

removing all code handling the environment variables NEST_DOC_DIR and NEST_DATA_DIR from SLIStartup
removing the setting of NEST_DOC_DIR, NEST_DATA_DIR and NEST_INSTALL_DIR from nest_vars.sh and
replacing all code involving NEST_DOC_DIR, NEST_DATA_DIR and NEST_INSTALL_DIR in PyNEST and do_tests.py by code achieving the same based on entries in the kernel status dictionary
adding the NEST library install directory to the dynamic loader's searchpath as suggested by @clinssen in Instructions for custom modules are incomplete/outdated(?) #1331.

Along the way, I've

fixed pertaining documentation
removed the old ReadData example for SLI and
renamed slihomepath to slilibdir in SLIStartup.

See the discussion in #1586 for more details.

extras/nest_vars.sh.in

Co-authored-by: clinssen <[email protected]>

clinssen · 2020-05-05T22:05:27Z

This PR removes NEST_INSTALL_DIR from the exported variables, but it is still used in

nest-simulator/pynest/nest/lib/hl_api_helper.py

Line 447 in 07d2acc

if 'NEST_INSTALL_DIR' not in os.environ:

The same goes for some of the other variables that would disappear. What to do with this code—should it be systematically removed?

jougs · 2020-05-05T22:13:05Z

Good catch!

Can you please also point me to the other places in the code for the other variables? I'll check what can be do about this.

clinssen · 2020-05-05T22:22:55Z

Sure! More systematically:

Variables to be removed:

NEST_DOC_DIR
NEST_INSTALL_DIR
NEST_DATA_DIR
NEST_PYTHON_PREFIX

Files affected:

sli/slistartup.cc
pynest/nest/lib/hl_api_info.py
pynest/nest/lib/hl_api_helper.py
testsuite/do_tests.sh.in
examples/nest/ReadData_demo.sli

jougs

@clinssen: I've investigated some more. Please find my assessment for usage of the different variables as inline comments below. Please let me know what you think!

extras/nest_vars.sh.in

…e_environment

This commit removes the possibility to set NEST_DOC_DIR as this did more harm than good. All uses of the variable have been replaced by appropriate queries to the NEST kernel. It also removes the ancient data reading example for SLI, which used the variable to find an example file.

This commit removes the possibility to set NEST_DATA_DIR as this did more harm than good. All uses of the variable have also been removed. It also renames slihomepath to a more appropriate slilibdir.

jougs · 2020-06-17T23:24:29Z

@heplesser: apparently I have overlooked that #1587 also removed the possibility to run our custom PyNEST test harness (which I think is a good thing!). I have thus removed do_tests.py and all references to it in 62c1f17 contained in this PR and hope that this is fine with you, although that change does not strictly belong here.

If you prefer the perfectly clean solution, I can cherry-pick that commit to another branch, remove it from this one and file an additional PR.

jougs · 2020-06-17T23:43:18Z

On second thought, I came to the conclusion that it's probably really better to create another PR to clean up the PyNEST testsuite. I have thus removed 62c1f17 from the history of this branch. As if it never happened. Sorry for the noise!

heplesser · 2020-06-18T10:33:01Z

@jougs I am confused. As long as this PR now will not re-introduce things that #1587 cleaned up, I am all for it.

jougs · 2020-06-19T00:22:50Z

@heplesser: no worries and sorry for the confusion. I leave this now for a re-review.

heplesser · 2020-06-19T07:11:14Z

Thanks @jougs, I am still happy 👍.

clinssen

I'm getting the following on make installcheck:

Phase 8: Running C++ tests (experimental)
-----------------------------------------
  Not running C++ tests because NEST was compiled without Boost.
Traceback (most recent call last):
  File "/home/archels/nest-simulator-fork-unpollute-environment-build/share/nest/extras/summarize_tests.py", line 30, in <module>
    import junitparser as jp
ImportError: No module named 'junitparser'
CMakeFiles/installcheck.dir/build.make:57: recipe for target 'CMakeFiles/installcheck' failed

doc/guides/spatial/guide_spatially_structured_networks.rst

sli/slistartup.cc

clinssen · 2020-06-21T08:46:15Z

extras/nest_vars.sh.in

-# Make nest/sli/... executables visible.
-export PATH="${NEST_INSTALL_DIR}/bin:${PATH}"
+# Make NEST executables available by prepending their path to PATH.
+export PATH="@CMAKE_INSTALL_PREFIX@/@CMAKE_INSTALL_BINDIR@:${PATH}"


Jochen, I fully trust your definition of "reasonable", it's just that I wouldn't trust this in the general case!

heplesser · 2020-06-21T10:15:58Z

I'm getting the following on make installcheck:

Phase 8: Running C++ tests (experimental)
-----------------------------------------
  Not running C++ tests because NEST was compiled without Boost.
Traceback (most recent call last):
  File "/home/archels/nest-simulator-fork-unpollute-environment-build/share/nest/extras/summarize_tests.py", line 30, in <module>
    import junitparser as jp
ImportError: No module named 'junitparser'
CMakeFiles/installcheck.dir/build.make:57: recipe for target 'CMakeFiles/installcheck' failed

@clinssen Since the transition to the new test summary generator, the testsuite requires the Python junitparser package to be available. We should probably find a way to specify these requirements properly. Could you create a separate issue for that?

clinssen · 2020-06-21T12:08:08Z

This PR should also update the description of nest_vars.sh on the installation instructions page: https://github.com/nest/nest-simulator/blob/master/doc/installation/linux_install.rst#environment-variables

jougs · 2020-06-24T08:43:45Z

@clinssen: I've addressed all your comments in my recent commits.

clinssen

Cheers!

jougs · 2020-06-24T19:01:27Z

@heplesser: would you click the button? I have more in the pipeline :-)

…e_environment

jougs · 2020-06-25T09:17:02Z

@heplesser: sorry for converting to draft again. I have found a problem with custom module loading. A fix for that should still go into this one.

jougs · 2020-06-25T13:24:58Z

The problem mentioned earlier was that with this PR in place dynamic modules were again not found without manually setting environment variables. The reason was that this PR removes the setting of (DY)LD_LIBRARY_PATH from nest_vars.sh as I was under the impression that this would not be needed anymore as #1513 already cleaned up the library paths used by NEST.

It turned out that paths defined in R(UN)PATH are actually not considered by the dynamic runtime linker after application startup and some additional measures are indeed necessary.

0c39c0a now fixes the problem by adding the library installation directory to the search path for the dynamic loader. This now means that in a default situation, everything just works. If users decide to install their custom extension modules to non-standard directories, they can still set (DY)LD_LIBRARY_PATH to make things found.

Can you please have another look?

Don't export variables that can confuse NEST

1dcee7b

jougs added T: Bug Wrong statements in the code or documentation S: Normal Handle this with default priority I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels May 4, 2020

jougs added this to the NEST 3.0 milestone May 4, 2020

jougs requested review from terhorstd and clinssen May 4, 2020 20:50

jougs self-assigned this May 4, 2020

Fix path for bin dir

f1736d6

heplesser reviewed May 5, 2020

View reviewed changes

extras/nest_vars.sh.in Outdated Show resolved Hide resolved

clinssen requested changes May 5, 2020

View reviewed changes

extras/nest_vars.sh.in Outdated Show resolved Hide resolved

extras/nest_vars.sh.in Outdated Show resolved Hide resolved

extras/nest_vars.sh.in Outdated Show resolved Hide resolved

jougs and others added 3 commits May 5, 2020 13:40

Update extras/nest_vars.sh.in

ba6a4ca

Co-authored-by: clinssen <[email protected]>

Update extras/nest_vars.sh.in

078800b

Co-authored-by: clinssen <[email protected]>

Update extras/nest_vars.sh.in

6c7f863

heplesser approved these changes May 5, 2020

View reviewed changes

jougs requested review from clinssen and removed request for terhorstd May 5, 2020 14:53

jougs commented May 7, 2020

View reviewed changes

extras/nest_vars.sh.in Show resolved Hide resolved

extras/nest_vars.sh.in Show resolved Hide resolved

extras/nest_vars.sh.in Show resolved Hide resolved

This was referenced May 7, 2020

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

Closed

Clean up handling of path-related environment variables #1586

Closed

jougs marked this pull request as draft May 7, 2020 16:53

jougs changed the title ~~Don't export variables that can confuse NEST~~ Clean up handling of path-related environment variables May 7, 2020

jougs added 4 commits May 8, 2020 02:36

Merge branch 'master' of github.com:nest/nest-simulator into unpollut…

2463ad9

…e_environment

Dynamically determine PyNEST test directory

2b535ab

Remove environment variable NEST_DATA_DIR

8151175

This commit removes the possibility to set NEST_DATA_DIR as this did more harm than good. All uses of the variable have also been removed. It also renames slihomepath to a more appropriate slilibdir.

jougs force-pushed the unpollute_environment branch from 62c1f17 to 157426d Compare June 17, 2020 23:31

jougs force-pushed the unpollute_environment branch from 8fa9d33 to 157426d Compare June 18, 2020 23:41

Re-add PYTHON variable accidentally lost in 157426d

0947e63

clinssen requested changes Jun 21, 2020

View reviewed changes

clinssen mentioned this pull request Jun 21, 2020

Add junitparser to installation requirements #1652

Closed

jougs added 3 commits June 24, 2020 10:39

Remove line referencing old installation instructions

b07c26b

Update wrt to removed environment variables

0613b18

Make variable names consistent by removing dollar sign

742e631

clinssen approved these changes Jun 24, 2020

View reviewed changes

Merge branch 'master' of github.com:nest/nest-simulator into unpollut…

adabf1d

…e_environment

jougs marked this pull request as draft June 25, 2020 09:15

Add library installation directory to dl search path

0c39c0a

jougs marked this pull request as ready for review June 25, 2020 13:25

Fix note about module location

bb5666c

terhorstd merged commit 82b9f20 into nest:master Jul 1, 2020

terhorstd mentioned this pull request Jul 3, 2020

Set and get structural plasticity status parameters through set and getKernelStatus #1537

Merged

jougs mentioned this pull request Jan 18, 2021

[WIP] Refactor testsuite setup #1071

Closed

jougs deleted the unpollute_environment branch August 31, 2023 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up handling of path-related environment variables #1578

Clean up handling of path-related environment variables #1578

jougs commented May 4, 2020 •

edited

Loading

clinssen commented May 5, 2020

jougs commented May 5, 2020

clinssen commented May 5, 2020

jougs left a comment

jougs commented Jun 17, 2020

jougs commented Jun 17, 2020

heplesser commented Jun 18, 2020

jougs commented Jun 19, 2020

heplesser commented Jun 19, 2020

clinssen left a comment

clinssen Jun 21, 2020

heplesser commented Jun 21, 2020

clinssen commented Jun 21, 2020

jougs commented Jun 24, 2020

clinssen left a comment

jougs commented Jun 24, 2020

jougs commented Jun 25, 2020

jougs commented Jun 25, 2020

Clean up handling of path-related environment variables #1578

Clean up handling of path-related environment variables #1578

Conversation

jougs commented May 4, 2020 • edited Loading

clinssen commented May 5, 2020

jougs commented May 5, 2020

clinssen commented May 5, 2020

jougs left a comment

Choose a reason for hiding this comment

jougs commented Jun 17, 2020

jougs commented Jun 17, 2020

heplesser commented Jun 18, 2020

jougs commented Jun 19, 2020

heplesser commented Jun 19, 2020

clinssen left a comment

Choose a reason for hiding this comment

clinssen Jun 21, 2020

Choose a reason for hiding this comment

heplesser commented Jun 21, 2020

clinssen commented Jun 21, 2020

jougs commented Jun 24, 2020

clinssen left a comment

Choose a reason for hiding this comment

jougs commented Jun 24, 2020

jougs commented Jun 25, 2020

jougs commented Jun 25, 2020

jougs commented May 4, 2020 •

edited

Loading