From e68535f8a67957a5da505462f717dfa2895c346a Mon Sep 17 00:00:00 2001 From: Anil Tuncel Date: Thu, 11 Apr 2024 13:47:29 +0200 Subject: [PATCH 01/11] FIX: use canonical paths and a set for loaded dlls --- share/lib/python/neuron/__init__.py | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/share/lib/python/neuron/__init__.py b/share/lib/python/neuron/__init__.py index c48fdd56ef..fda341cc87 100644 --- a/share/lib/python/neuron/__init__.py +++ b/share/lib/python/neuron/__init__.py @@ -290,12 +290,12 @@ def test_rxd(exitOnError=True): # h.anyclass methods may be overridden. If so the base method can be called # using the idiom self.basemethod = self.baseattr('methodname') # ------------------------------------------------------------------------------ - +from pathlib import Path import sys, types from neuron.hclass3 import HocBaseObject, hclass # global list of paths already loaded by load_mechanisms -nrn_dll_loaded = [] +nrn_dll_loaded = set() def load_mechanisms(path, warn_if_already_loaded=True): @@ -313,10 +313,11 @@ def load_mechanisms(path, warn_if_already_loaded=True): import platform global nrn_dll_loaded - if path in nrn_dll_loaded: - if warn_if_already_loaded: - print("Mechanisms already loaded from path: %s. Aborting." % path) - return True + path_obj = Path(path).resolve() + if path_obj.as_posix() in nrn_dll_loaded: + if warn_if_already_loaded: + print(f"Mechanisms already loaded from path: {path_obj}. Aborting.") + return True # in case NEURON is assuming a different architecture to Python, # we try multiple possibilities @@ -332,10 +333,10 @@ def load_mechanisms(path, warn_if_already_loaded=True): arch_list = [""] for arch in arch_list: - lib_path = os.path.join(path, arch, libsubdir, libname) - if os.path.exists(lib_path): - h.nrn_load_dll(lib_path) - nrn_dll_loaded.append(path) + lib_path = path_obj / arch / libsubdir / libname + if lib_path.exists(): + h.nrn_load_dll(lib_path.as_posix()) + nrn_dll_loaded.add(path_obj.as_posix()) return True print("NEURON mechanisms not found in %s." % path) return False From 6e5d44ea147fcea68726c8edb8dd35b766b7ae15 Mon Sep 17 00:00:00 2001 From: Anil Tuncel Date: Thu, 11 Apr 2024 13:58:43 +0200 Subject: [PATCH 02/11] apply black formatting --- share/lib/python/neuron/__init__.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/share/lib/python/neuron/__init__.py b/share/lib/python/neuron/__init__.py index fda341cc87..43f4d2144e 100644 --- a/share/lib/python/neuron/__init__.py +++ b/share/lib/python/neuron/__init__.py @@ -315,9 +315,9 @@ def load_mechanisms(path, warn_if_already_loaded=True): global nrn_dll_loaded path_obj = Path(path).resolve() if path_obj.as_posix() in nrn_dll_loaded: - if warn_if_already_loaded: - print(f"Mechanisms already loaded from path: {path_obj}. Aborting.") - return True + if warn_if_already_loaded: + print(f"Mechanisms already loaded from path: {path_obj}. Aborting.") + return True # in case NEURON is assuming a different architecture to Python, # we try multiple possibilities From 974eb60bb917629e3096034f91f05af3e0c48553 Mon Sep 17 00:00:00 2001 From: Anil Tuncel Date: Thu, 11 Apr 2024 15:16:56 +0200 Subject: [PATCH 03/11] add test_load_mechanisms_from_non_existent_path --- share/lib/python/neuron/tests/test_neuron.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/share/lib/python/neuron/tests/test_neuron.py b/share/lib/python/neuron/tests/test_neuron.py index 6c49934f30..ad7afb6720 100644 --- a/share/lib/python/neuron/tests/test_neuron.py +++ b/share/lib/python/neuron/tests/test_neuron.py @@ -5,6 +5,7 @@ $Id$ """ +from pathlib import Path import unittest import neuron from neuron import h @@ -199,6 +200,14 @@ def testHelp(self): self.assertFalse(error) return 0 + def test_load_mechanisms_from_non_existent_path(self): + """Assure mechanisms are not loaded from a non-existent directory.""" + from neuron import load_mechanisms, nrn_dll_loaded + + mech_path1 = "fake_mechanisms_dir" + assert not load_mechanisms(mech_path1) + assert Path(mech_path1).resolve().as_posix() not in nrn_dll_loaded + def testRxDexistence(self): from neuron import config From 59b543e170dd6cfdfe3aced2d893eeafb6998ec8 Mon Sep 17 00:00:00 2001 From: Anil Tuncel Date: Thu, 11 Apr 2024 16:16:41 +0200 Subject: [PATCH 04/11] add test_load_mechanisms_reloading --- share/lib/python/neuron/tests/test_neuron.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/share/lib/python/neuron/tests/test_neuron.py b/share/lib/python/neuron/tests/test_neuron.py index ad7afb6720..bad7fa1f97 100644 --- a/share/lib/python/neuron/tests/test_neuron.py +++ b/share/lib/python/neuron/tests/test_neuron.py @@ -6,7 +6,9 @@ """ from pathlib import Path +from tempfile import TemporaryDirectory import unittest +from unittest.mock import patch import neuron from neuron import h @@ -208,6 +210,23 @@ def test_load_mechanisms_from_non_existent_path(self): assert not load_mechanisms(mech_path1) assert Path(mech_path1).resolve().as_posix() not in nrn_dll_loaded + @patch("pathlib.Path.exists") + def test_load_mechanisms_reloading(self, mock_exists): + """Assure the same directory is not loaded multiple times.""" + from neuron import load_mechanisms, nrn_dll_loaded + + mock_exists.return_value = True # bypass path.exists() + + with TemporaryDirectory() as temp_dir: + mech_path1 = Path(temp_dir) / "mechanisms_dir" + mech_path1.mkdir() + assert load_mechanisms(mech_path1) + assert mech_path1.resolve().as_posix() in nrn_dll_loaded + assert len(nrn_dll_loaded) == 1 + mech_path2 = mech_path1 / ".." / "mechanisms_dir" + assert load_mechanisms(mech_path2) + assert len(nrn_dll_loaded) == 1 + def testRxDexistence(self): from neuron import config From cd84a0a1bfdac663a10e880fb5aa2723ef6e1fc4 Mon Sep 17 00:00:00 2001 From: Anil Tuncel Date: Fri, 12 Apr 2024 10:27:27 +0200 Subject: [PATCH 05/11] implement loaded mechanisms path check for mswin_load_dll --- src/nrnoc/init.cpp | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/src/nrnoc/init.cpp b/src/nrnoc/init.cpp index ce86426495..8dadfc0d70 100644 --- a/src/nrnoc/init.cpp +++ b/src/nrnoc/init.cpp @@ -264,27 +264,46 @@ void* nrn_realpath_dlopen(const char* relpath, int flags) { return handle; } +// Global set to keep track of loaded DLLs +static std::unordered_set loaded_dlls; + int mswin_load_dll(const char* cp1) { + char* resolved_path = realpath(cp1, NULL); + if (!resolved_path) { + fprintf(stderr, "Failed to resolve path: %s\n", strerror(errno)); + return 0; + } + + std::string full_path(resolved_path); + free(resolved_path); + + // Check if the DLL has already been loaded + if (loaded_dlls.find(full_path) != loaded_dlls.end()) { + fprintf(stderr, "DLL already loaded: %s\n", full_path.c_str()); + return 1; // Return success as the DLL is already loaded + } + void* handle; if (nrnmpi_myid < 1) if (!nrn_nobanner_ && nrn_istty_) { fprintf(stderr, "loading membrane mechanisms from %s\n", cp1); } #if DARWIN - handle = nrn_realpath_dlopen(cp1, RTLD_NOW); + handle = nrn_realpath_dlopen(full_path.c_str(), RTLD_NOW); #else // not DARWIN - handle = dlopen(cp1, RTLD_NOW); + handle = dlopen(full_path.c_str(), RTLD_NOW); #endif // not DARWIN if (handle) { Pfrv mreg = (Pfrv) dlsym(handle, "modl_reg"); if (mreg) { (*mreg)(); + loaded_dlls.insert(full_path); // Insert the path into the set + return 1; } else { fprintf(stderr, "dlsym modl_reg failed\n%s\n", dlerror()); dlclose(handle); return 0; } - return 1; } else { fprintf(stderr, "dlopen failed - \n%s\n", dlerror()); } From 3f7274cce7afb4aff22fc97654c230c0265f67df Mon Sep 17 00:00:00 2001 From: Anil Tuncel Date: Fri, 12 Apr 2024 10:42:20 +0200 Subject: [PATCH 06/11] add missing sympy requirement --- nrn_requirements.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/nrn_requirements.txt b/nrn_requirements.txt index 77ccb32ddc..2a56550097 100644 --- a/nrn_requirements.txt +++ b/nrn_requirements.txt @@ -13,3 +13,4 @@ pytest-cov mpi4py numpy find_libpython +sympy From 26ecf1362d120d1af19f59528a82ca9f71b67ccd Mon Sep 17 00:00:00 2001 From: Anil Tuncel Date: Fri, 12 Apr 2024 10:42:55 +0200 Subject: [PATCH 07/11] add #include --- src/nrnoc/init.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/nrnoc/init.cpp b/src/nrnoc/init.cpp index 8dadfc0d70..4ed9af7d43 100644 --- a/src/nrnoc/init.cpp +++ b/src/nrnoc/init.cpp @@ -22,6 +22,7 @@ #include #include +#include /* change this to correspond to the ../nmodl/nocpout nmodl_version_ string*/ static char nmodl_version_[] = "7.7.0"; From f7125d4f0f8800a8808ea1208c6c3ca219a091f9 Mon Sep 17 00:00:00 2001 From: Anil Tuncel Date: Fri, 12 Apr 2024 11:40:12 +0200 Subject: [PATCH 08/11] consider realpath not being avaialble --- src/nrnoc/init.cpp | 86 ++++++++++++++++------------------------------ 1 file changed, 29 insertions(+), 57 deletions(-) diff --git a/src/nrnoc/init.cpp b/src/nrnoc/init.cpp index 4ed9af7d43..9e5a4a6664 100644 --- a/src/nrnoc/init.cpp +++ b/src/nrnoc/init.cpp @@ -225,53 +225,29 @@ int nrn_is_cable(void) { return 1; } -void* nrn_realpath_dlopen(const char* relpath, int flags) { +// the consumer has to deallocate the returned value +char* nrn_realpath(const char* relpath) { char* abspath = NULL; - void* handle = NULL; - /* use realpath or _fullpath even if is already a full path */ - -#if defined(HAVE_REALPATH) + #if defined(HAVE_REALPATH) abspath = realpath(relpath, NULL); -#else /* not HAVE_REALPATH */ -#if defined(__MINGW32__) - abspath = _fullpath(NULL, relpath, 0); -#else /* not __MINGW32__ */ + #elif defined(__MINGW32__) + abspath = _fullpath(NULL, relpath, _MAX_PATH); + #else abspath = strdup(relpath); -#endif /* not __MINGW32__ */ -#endif /* not HAVE_REALPATH */ - if (abspath) { - handle = dlopen(abspath, flags); -#if DARWIN - if (!handle) { - nrn_possible_mismatched_arch(abspath); - } -#endif // DARWIN - free(abspath); - } else { - int patherr = errno; - handle = dlopen(relpath, flags); - if (!handle) { - Fprintf(stderr, - "realpath failed errno=%d (%s) and dlopen failed with %s\n", - patherr, - strerror(patherr), - relpath); -#if DARWIN - nrn_possible_mismatched_arch(abspath); -#endif // DARWIN - } + #endif + if (!abspath) { + fprintf(stderr, "Failed to resolve path: %s, due to error: %s\n", relpath, strerror(errno)); } - return handle; + return abspath; } // Global set to keep track of loaded DLLs static std::unordered_set loaded_dlls; int mswin_load_dll(const char* cp1) { - char* resolved_path = realpath(cp1, NULL); + char* resolved_path = nrn_realpath(cp1); if (!resolved_path) { - fprintf(stderr, "Failed to resolve path: %s\n", strerror(errno)); return 0; } @@ -284,31 +260,27 @@ int mswin_load_dll(const char* cp1) { return 1; // Return success as the DLL is already loaded } - void* handle; - if (nrnmpi_myid < 1) - if (!nrn_nobanner_ && nrn_istty_) { - fprintf(stderr, "loading membrane mechanisms from %s\n", cp1); - } + // Load the DLL + void* handle = dlopen(full_path.c_str(), RTLD_NOW); + if (!handle) { + fprintf(stderr, "dlopen failed: %s\n", dlerror()); #if DARWIN - handle = nrn_realpath_dlopen(full_path.c_str(), RTLD_NOW); -#else // not DARWIN - handle = dlopen(full_path.c_str(), RTLD_NOW); -#endif // not DARWIN - if (handle) { - Pfrv mreg = (Pfrv) dlsym(handle, "modl_reg"); - if (mreg) { - (*mreg)(); - loaded_dlls.insert(full_path); // Insert the path into the set - return 1; - } else { - fprintf(stderr, "dlsym modl_reg failed\n%s\n", dlerror()); - dlclose(handle); - return 0; - } + // Darwin-specific architecture mismatch handling + nrn_possible_mismatched_arch(full_path.c_str()); +#endif + return 0; + } + + Pfrv mreg = (Pfrv) dlsym(handle, "modl_reg"); + if (mreg) { + (*mreg)(); + loaded_dlls.insert(full_path); // Insert the path into the set + return 1; } else { - fprintf(stderr, "dlopen failed - \n%s\n", dlerror()); + fprintf(stderr, "dlsym modl_reg failed\n%s\n", dlerror()); + dlclose(handle); + return 0; } - return 0; } void hoc_nrn_load_dll(void) { From 2d7f661ec22d51b6275cf55b8e9be9178a2fce2f Mon Sep 17 00:00:00 2001 From: Anil Tuncel Date: Fri, 12 Apr 2024 11:48:28 +0200 Subject: [PATCH 09/11] make format --- src/nrnoc/init.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/nrnoc/init.cpp b/src/nrnoc/init.cpp index 9e5a4a6664..5ba1a828f7 100644 --- a/src/nrnoc/init.cpp +++ b/src/nrnoc/init.cpp @@ -229,13 +229,13 @@ int nrn_is_cable(void) { char* nrn_realpath(const char* relpath) { char* abspath = NULL; - #if defined(HAVE_REALPATH) +#if defined(HAVE_REALPATH) abspath = realpath(relpath, NULL); - #elif defined(__MINGW32__) +#elif defined(__MINGW32__) abspath = _fullpath(NULL, relpath, _MAX_PATH); - #else +#else abspath = strdup(relpath); - #endif +#endif if (!abspath) { fprintf(stderr, "Failed to resolve path: %s, due to error: %s\n", relpath, strerror(errno)); } From 6c09955cb5385ca9baccc1b939a33918202e161d Mon Sep 17 00:00:00 2001 From: Anil Tuncel Date: Fri, 12 Apr 2024 12:47:07 +0200 Subject: [PATCH 10/11] Revert "add missing sympy requirement" This reverts commit 3f7274cce7afb4aff22fc97654c230c0265f67df. --- nrn_requirements.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/nrn_requirements.txt b/nrn_requirements.txt index 2a56550097..77ccb32ddc 100644 --- a/nrn_requirements.txt +++ b/nrn_requirements.txt @@ -13,4 +13,3 @@ pytest-cov mpi4py numpy find_libpython -sympy From 61787846e315db8931d413d4b53dde71cbaca3c2 Mon Sep 17 00:00:00 2001 From: Anil Tuncel Date: Fri, 12 Apr 2024 16:48:24 +0200 Subject: [PATCH 11/11] return std::string in nrn_realpath --- src/nrnoc/init.cpp | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/nrnoc/init.cpp b/src/nrnoc/init.cpp index 5ba1a828f7..ffcc94c659 100644 --- a/src/nrnoc/init.cpp +++ b/src/nrnoc/init.cpp @@ -225,45 +225,44 @@ int nrn_is_cable(void) { return 1; } -// the consumer has to deallocate the returned value -char* nrn_realpath(const char* relpath) { +std::string nrn_realpath(const char* relpath) { char* abspath = NULL; #if defined(HAVE_REALPATH) abspath = realpath(relpath, NULL); #elif defined(__MINGW32__) - abspath = _fullpath(NULL, relpath, _MAX_PATH); + abspath = _fullpath(NULL, relpath, 0); #else abspath = strdup(relpath); #endif if (!abspath) { - fprintf(stderr, "Failed to resolve path: %s, due to error: %s\n", relpath, strerror(errno)); + Fprintf(stderr, "Failed to resolve path: %s, due to error: %s\n", relpath, strerror(errno)); + return ""; } - return abspath; + std::string resolved_path(abspath); + free(abspath); + return resolved_path; } // Global set to keep track of loaded DLLs static std::unordered_set loaded_dlls; int mswin_load_dll(const char* cp1) { - char* resolved_path = nrn_realpath(cp1); - if (!resolved_path) { + std::string full_path = nrn_realpath(cp1); + if (full_path.empty()) { return 0; } - std::string full_path(resolved_path); - free(resolved_path); - // Check if the DLL has already been loaded if (loaded_dlls.find(full_path) != loaded_dlls.end()) { - fprintf(stderr, "DLL already loaded: %s\n", full_path.c_str()); + Fprintf(stderr, "DLL already loaded: %s\n", full_path.c_str()); return 1; // Return success as the DLL is already loaded } // Load the DLL void* handle = dlopen(full_path.c_str(), RTLD_NOW); if (!handle) { - fprintf(stderr, "dlopen failed: %s\n", dlerror()); + Fprintf(stderr, "dlopen failed: %s\n", dlerror()); #if DARWIN // Darwin-specific architecture mismatch handling nrn_possible_mismatched_arch(full_path.c_str()); @@ -277,7 +276,7 @@ int mswin_load_dll(const char* cp1) { loaded_dlls.insert(full_path); // Insert the path into the set return 1; } else { - fprintf(stderr, "dlsym modl_reg failed\n%s\n", dlerror()); + std::cerr << "dlsym modl_reg failed: " << dlerror() << std::endl; dlclose(handle); return 0; }