Clean headers dependencies in nrn #2389
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
✔️ 411eec3 -> Azure artifacts URL |
|
✔️ 787d3ea -> Azure artifacts URL |
Codecov Report
@@ Coverage Diff @@
## master #2389 +/- ##
==========================================
+ Coverage 60.20% 60.28% +0.07%
==========================================
Files 627 650 +23
Lines 121085 121697 +612
==========================================
+ Hits 72905 73368 +463
- Misses 48180 48329 +149
... and 36 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
This comment has been minimized.
This comment has been minimized.
|
✔️ 4aa5393 -> Azure artifacts URL |
This comment has been minimized.
This comment has been minimized.
| @@ -859,15 +859,6 @@ install(PROGRAMS ${CMAKE_CURRENT_SOURCE_DIR}/share/lib/cleanup | |||
|
|
|||
| # Copy NEURON headers that will be included in the installation into the build directory. | |||
| set(headers_in_build_dir) | |||
| foreach(header_below_src ${HEADER_FILES_TO_INSTALL}) | |||
There was a problem hiding this comment.
Just a comment for other reviewers: I understand the idea here is that src/dir/header.h used to get installed as include/header.h, but now it gets installed as include/dir/header.h.
| @@ -19,7 +19,7 @@ UserLDFLAGS = | |||
| # install dirs | |||
| bindir := ${ROOT}/@CMAKE_INSTALL_BINDIR@ | |||
| libdir := ${ROOT}/@CMAKE_INSTALL_LIBDIR@ | |||
| incdir := ${ROOT}/@CMAKE_INSTALL_INCLUDEDIR@ | |||
| incdir := -I${ROOT}/@CMAKE_INSTALL_INCLUDEDIR@ -I${ROOT}/@CMAKE_INSTALL_INCLUDEDIR@/crout -I${ROOT}/@CMAKE_INSTALL_INCLUDEDIR@/neuron -I${ROOT}/@CMAKE_INSTALL_INCLUDEDIR@/newton -I${ROOT}/@CMAKE_INSTALL_INCLUDEDIR@/nrncvode -I${ROOT}/@CMAKE_INSTALL_INCLUDEDIR@/nrniv -I${ROOT}/@CMAKE_INSTALL_INCLUDEDIR@/nrnmpi -I${ROOT}/@CMAKE_INSTALL_INCLUDEDIR@/nrnoc -I${ROOT}/@CMAKE_INSTALL_INCLUDEDIR@/oc -I${ROOT}/@CMAKE_INSTALL_INCLUDEDIR@/scopmath -I${ROOT}/@CMAKE_INSTALL_INCLUDEDIR@/sparse13 | |||
There was a problem hiding this comment.
I'm confused by this change. If we are now including a lot more of these directory names in the #include <...> statements, why do we need these here?
There was a problem hiding this comment.
because mod files still include "raw" files from the old grouped <BUILD>/include directory
There was a problem hiding this comment.
I suppose that is widespread in VERBATIM blocks, not just in the code generated by nocmodl?
I'm not sure it's a step forwards to introduce two different #include schemes, one for the library source files and one for the generated .cpp...
There was a problem hiding this comment.
We can slowly clean all mod files.
There was a problem hiding this comment.
I don't buy that; the cost/reward for updating all of ModelDB for that is such that it will never happen, in my view.
So: this seems like it's creating and locking in a new source of inconsistency between different bits of C++. Is that really worth it?
There was a problem hiding this comment.
I agree on the first part that it will never happens.
For the second part, I, personnaly, think, it worse it, to clean more the code and know who include what.
There was a problem hiding this comment.
Thoughts @pramodk @alexsavulescu @nrnhines ?
There was a problem hiding this comment.
I don't buy that; the cost/reward for updating all of ModelDB for that is such that it will never happen, in my view.
I agree on the first part that it will never happens.
We are on the same page!
So: this seems like it's creating and locking in a new source of inconsistency between different bits of C++. Is that really worth it?
For the second part, I, personnaly, think, it worse it, to clean more the code and know who include what.
I understand the "inconsistency" aspects that Olli mentioned (i.e. having a single way would be better).
I don't have recent enough hands-on experience to assess the impact or propose a better solution here. Maybe @olupton you can propose what should be done here?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
alkino
force-pushed
the
cornu/clean_headers
branch
from
02bbca1
to
ccb080a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Replace all nrn includes by a general include.
Remove all the -I from CMake and move them inside the script nrnivmodl.
"" => <>
<> => <>