Skip to content

Fix NMODL install paths - #3581

Merged
JCGoran merged 1 commit into
masterfrom
jelic/fix_install_paths
Aug 28, 2025
Merged

Fix NMODL install paths#3581
JCGoran merged 1 commit into
masterfrom
jelic/fix_install_paths

Conversation

@JCGoran

@JCGoran JCGoran commented Aug 26, 2025

Copy link
Copy Markdown
Collaborator

Fixes #3580.

Note that the various prefix forward slashes have been removed because if the preceding variable is empty, it will try to install it to the absolute path (for instance, /bin instead of bin, and the latter gets expanded to ${CMAKE_INSTALL_PREFIX}bin, where CMAKE_INSTALL_PREFIX already takes care of the paths).

@sonarqubecloud

Copy link
Copy Markdown

@azure-pipelines

Copy link
Copy Markdown

✔️ 3d69b5e -> Azure artifacts URL

@github-actions

Copy link
Copy Markdown
Contributor

✔️ 3d69b5e -> artifacts URL

@JCGoran
JCGoran marked this pull request as ready for review August 26, 2025 15:16
@JCGoran
JCGoran requested a review from nrnhines August 26, 2025 15:16

@nrnhines nrnhines left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Assuming a trailing '/' on path variables seems strange to me. Wouldn't an alternative be to check the variable and demand it be non empty?

@codecov

codecov Bot commented Aug 26, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.46%. Comparing base (0d99051) to head (3d69b5e).
⚠️ Report is 45 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3581   +/-   ##
=======================================
  Coverage   68.46%   68.46%           
=======================================
  Files         685      685           
  Lines      116720   116720           
=======================================
+ Hits        79914    79918    +4     
+ Misses      36806    36802    -4     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JCGoran

JCGoran commented Aug 26, 2025

Copy link
Copy Markdown
Collaborator Author

Assuming a trailing '/' on path variables seems strange to me. Wouldn't an alternative be to check the variable and demand it be non empty?

Those are already taken care of, all custom paths that we set have a trailing slash (except when calling install(... DESTINATION ...) since there it doesn't matter):

nrn/CMakeLists.txt

Lines 43 to 58 in 3d69b5e

if(SKBUILD)
set(NRN_INSTALL_PYTHON_PREFIX "neuron/")
set(NRN_INSTALL_DATA_PREFIX "neuron/.data/")
# need to force fmtlib install prefixes
set(FMT_LIB_DIR "${NRN_INSTALL_DATA_PREFIX}/lib")
set(FMT_INC_DIR "${NRN_INSTALL_DATA_PREFIX}/include")
set(FMT_PKGCONFIG_DIR "${NRN_INSTALL_DATA_PREFIX}/lib/pkgconfig")
set(FMT_CMAKE_DIR "${NRN_INSTALL_DATA_PREFIX}/lib/cmake/fmt")
# need to force IV install prefixes
set(IV_LIB_INSTALL_DIR "${NRN_INSTALL_DATA_PREFIX}/lib")
set(IV_HEADERS_INSTALL_DIR "${NRN_INSTALL_DATA_PREFIX}/include")
set(IV_BIN_INSTALL_DIR "${NRN_INSTALL_DATA_PREFIX}/bin")
else()
set(NRN_INSTALL_PYTHON_PREFIX "lib/python/neuron/")
set(NRN_INSTALL_DATA_PREFIX)
endif()

The case of DESTINATION being empty in a given call to install is handled easily: cmake --install will fail, and we will get CI errors.

@JCGoran
JCGoran merged commit dfa7a00 into master Aug 28, 2025
51 of 52 checks passed
@JCGoran
JCGoran deleted the jelic/fix_install_paths branch August 28, 2025 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CMake sometimes installs NMODL build artifacts in dot dirs instead of regular ones

2 participants