Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace hocload.sh with std::filesystem utils #3293

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

JCGoran
Copy link
Collaborator

@JCGoran JCGoran commented Dec 16, 2024

hoc_load seems to be doing the following:

  • takes as arg a hoc keyword like proc, func, begintemplate
  • looks up whether the current hoc argument (from gargstr) is a defined symbol in the current scope
  • if the symbols is not defined, call hocload.sh with 3 args: the keyword, the symbol name, and hoc_pid. This script seems to grep all HOC files in ${PWD}, ${HOC_LIBRARY_PATH} for the pattern func|proc|begintemplate, stores them in the temp file names, then uses sed to find out whether there is a matching symbol in some file. If there is, it echoes the first match, and returns 0, and if there aren't any matches, it returns 1.
  • this is followed by a bunch of logic to see whether the file containing the definition actually exists, followed by a call to hoc_Load_file

I've replaced most of the above using std::filesystem, though I am not sure whether the logic of returning the first match is right in the first place, but nevertheless I tried replicating the existing behavior as closely as possible.

Copy link

codecov bot commented Dec 16, 2024

Codecov Report

Attention: Patch coverage is 3.70370% with 52 lines in your changes missing coverage. Please review.

Project coverage is 67.05%. Comparing base (b17d872) to head (92de136).

Files with missing lines Patch % Lines
src/oc/fileio.cpp 3.70% 52 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3293      +/-   ##
==========================================
- Coverage   67.08%   67.05%   -0.03%     
==========================================
  Files         571      571              
  Lines      111044   111086      +42     
==========================================
  Hits        74489    74489              
- Misses      36555    36597      +42     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bbpbuildbot

This comment has been minimized.

Copy link

✔️ 6a3f8d8 -> Azure artifacts URL

@JCGoran
Copy link
Collaborator Author

JCGoran commented Dec 17, 2024

Model 150245 from modelDB fails (see https://github.com/neuronsimulator/nrn-modeldb-ci/actions/runs/12369627580/job/34521930207), but I am 99.99% sure it's not related since it fails on the nightly as well (see https://github.com/neuronsimulator/nrn-modeldb-ci/actions/runs/12361469702/job/34498702435).

@JCGoran JCGoran marked this pull request as ready for review December 17, 2024 11:02
Copy link

✔️ 253637f -> Azure artifacts URL

@JCGoran JCGoran requested a review from matz-e December 18, 2024 07:13
@JCGoran JCGoran requested a review from nrnhines January 9, 2025 12:58
Copy link

✔️ 0bc2c1d -> Azure artifacts URL

Copy link
Member

@nrnhines nrnhines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm of two minds about this. On the one hand, if the change does not affect behavior, I'm fine with it. On the other hand, I don't want to spend any time on it. This supports an old feature of hoc for the functions load_func, load_proc, and load_template. see, e.g. https://nrn.readthedocs.io/en/latest/hoc/programming/dynamiccode.html#load_func which probably can't be removed but is practically superseded by the less complex, easier to understand, and very commonly used load_file. My prejudice is not to spend too much time on hoc development (python is so much more useful!) but only to keep it maintained. This pr perhaps simplifies maintenance so I'm approving. But let's first be sure it passes nrn-modeldb-ci

Copy link

✔️ 609d27f -> Azure artifacts URL

Copy link

✔️ 92de136 -> Azure artifacts URL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants