255 organize odeint as stand alone library#256
Conversation
The location of mod needs to be specified explicit if used outside the libneo build. We also need to explicit compile odeint with omp if it needs to be run by e.g. libneo tests in that mode.
Review Summary by QodoOrganize odeint as standalone library module
WalkthroughsDescription• Reorganize odeint as independent library module • Move odeint source to dedicated src/odeint directory • Create standalone CMakeLists.txt for odeint compilation • Add explicit OpenMP support for odeint library Diagramflowchart LR
A["CMakeLists.txt"] -->|"add_subdirectory"| B["src/odeint/CMakeLists.txt"]
C["CMakeSources.in"] -->|"remove odeint_allroutines.f90"| D["src/odeint/odeint_allroutines.f90"]
B -->|"create library"| E["odeint STATIC library"]
E -->|"link to neo"| F["neo library"]
E -->|"conditional OpenMP"| G["OpenMP::OpenMP_Fortran"]
File Changes1. CMakeLists.txt
|
Code Review by Qodo
1. ODEINT modules not installed
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| set_target_properties(odeint PROPERTIES | ||
| Fortran_MODULE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}) | ||
| target_include_directories(odeint PUBLIC | ||
| $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>) |
There was a problem hiding this comment.
1. Odeint modules not installed 🐞 Bug ⛯ Reliability
odeint writes its .mod files to src/odeint's binary dir instead of the project-wide
${CMAKE_Fortran_MODULE_DIRECTORY}; existing install/copy steps that package modules from the
global module directory will omit ODEINT modules, breaking downstream compilation against installed
artifacts.
Agent Prompt
## Issue description
`odeint` currently emits Fortran `.mod` files into its local binary dir (`src/odeint` build dir) rather than the project-wide `${CMAKE_Fortran_MODULE_DIRECTORY}` used elsewhere. Existing module packaging/install steps that copy `${CMAKE_Fortran_MODULE_DIRECTORY}` will therefore omit ODEINT modules, breaking downstream compilation against installed artifacts.
## Issue Context
The root build sets a global module directory (`${binary}/include`). Other parts of the build/install logic copy modules from that directory.
## Fix Focus Areas
- src/odeint/CMakeLists.txt[11-14]
- CMakeLists.txt[61-65]
- src/hdf5_tools/CMakeLists.txt[31-37]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
this was done to not rely on CMAKE_Fortran_MODULE_DIRECTORY to be set by libneo if odeint is build independant. Thoughts?
There was a problem hiding this comment.
Question:
this was done to not rely on CMAKE_Fortran_MODULE_DIRECTORY to be set by libneo if odeint is build independant. Thoughts?
Answer:
You can steer Qodo Merge’s /improve suggestions to explicitly account for “odeint can be built standalone” and avoid recommendations that assume ${CMAKE_Fortran_MODULE_DIRECTORY} is always set.
Add a repo-level instruction for the improve tool:
[pr_code_suggestions]
extra_instructions = """\
When reviewing CMake/Fortran module installation issues:
- Assume subprojects (like odeint) may be built standalone, so do NOT assume CMAKE_Fortran_MODULE_DIRECTORY is always set by a parent project.
- If suggesting to use CMAKE_Fortran_MODULE_DIRECTORY, require a safe fallback behavior when it is unset (e.g., set a sensible default module dir or guard logic), and preserve standalone-build correctness.
- Prefer solutions that keep install/packaging of .mod files correct both when embedded in libneo and when built independently.
"""If you want this guidance only for a single PR run, you can pass it inline:
/improve --pr_code_suggestions.extra_instructions="(same text as above)"
If you share which platform you’re using (GitHub App vs Action vs CLI) and whether this feedback is coming from /improve (code suggestions) or /review, I can suggest the most appropriate place to set it (wiki vs local vs global .pr_agent.toml).
Relevant Sources:
- https://docs.qodo.ai/qodo-documentation/code-review/qodo-merge/tools/improve#extra-instructions-and-best-practices
- https://docs.qodo.ai/qodo-documentation/code-review/qodo-merge/usage-guide/additional_configurations#extra-instructions
- https://docs.qodo.ai/qodo-documentation/code-review/qodo-merge/usage-guide/configuration_options#local-configuration-file
There was a problem hiding this comment.
- Odeint modules not installed
🐞 Bug⛯ Reliabilityodeint writes its .mod files to src/odeint's binary dir instead of the project-wide
${CMAKE_Fortran_MODULE_DIRECTORY}; existing install/copy steps that package modules from the
global module directory will omit ODEINT modules, breaking downstream compilation against installed
artifacts.Agent Prompt
this was done to not rely on CMAKE_Fortran_MODULE_DIRECTORY to be set by libneo if odeint is build independant. Thoughts?
There was a problem hiding this comment.
Question:
🐞 Bug⛯ Reliability> > odeint writes its .mod files to src/odeint's binary dir instead of the project-wide > ${CMAKE_Fortran_MODULE_DIRECTORY}; existing install/copy steps that package modules from the > global module directory will omit ODEINT modules, breaking downstream compilation against installed > artifacts. > > Agent Prompt this was done to not rely on CMAKE_Fortran_MODULE_DIRECTORY to be set by libneo if odeint is build independant. Thoughts?
Answer:
You can steer Qodo Merge’s /improve (code suggestions) to not assume a parent project sets CMAKE_Fortran_MODULE_DIRECTORY, and to require a safe fallback that preserves standalone builds.
Add repo-level guidance for the improve tool in .pr_agent.toml (wiki or repo root):
[pr_code_suggestions]
extra_instructions = """\
When reviewing CMake/Fortran module (.mod) output+install issues:
- Assume subprojects (like odeint) may be built standalone, so do NOT assume CMAKE_Fortran_MODULE_DIRECTORY is set by a parent project.
- If suggesting to use CMAKE_Fortran_MODULE_DIRECTORY, require a safe fallback when it is unset (e.g., set a sensible default module dir or guard logic).
- Prefer solutions that keep packaging/install of .mod files correct both when embedded in a parent build and when built independently.
"""If you only want this for a single run, pass it inline:
/improve --pr_code_suggestions.extra_instructions="(same text as above)"
If you tell me which platform you use (GitHub App vs GitHub Action vs CLI) I can suggest the best place to store this (wiki vs local .pr_agent.toml vs action env vars).
Relevant Sources:
- https://docs.qodo.ai/qodo-documentation/code-review/qodo-merge/tools/improve#extra-instructions-and-best-practices
- https://docs.qodo.ai/qodo-documentation/code-review/qodo-merge/usage-guide/additional_configurations#extra-instructions
- https://docs.qodo.ai/qodo-documentation/code-review/qodo-merge/usage-guide/configuration_options#local-configuration-file
- https://docs.qodo.ai/qodo-documentation/code-review/qodo-merge/usage-guide/configuration_options#wiki-configuration-file
There was a problem hiding this comment.
Those lines are doing the right thing for a Fortran static library that produces .mod files:
-
Fortran_MODULE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
Ensures all module outputs for targetodeintare written to the target’s build dir. This avoids scattering.modfiles and makes downstream consumption predictable. -
target_include_directories(odeint PUBLIC $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>)
Exposes that module directory to consumers when building from the build tree, so targets thattarget_link_libraries(... odeint)can finduse <module>.modfiles.
A couple of suggested tweaks/considerations:
- Install/export support: if you ever plan to
install(TARGETS odeint EXPORT ...), you’ll likely also want anINSTALL_INTERFACEinclude dir and to install the.modfiles, e.g.target_include_directories(odeint PUBLIC $<BUILD_INTERFACE:...> $<INSTALL_INTERFACE:include>)- plus
install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/ DESTINATION include FILES_MATCHING PATTERN "*.mod")(or better: use a dedicated module install dir).
- Multi-config generators (Visual Studio, Xcode):
${CMAKE_CURRENT_BINARY_DIR}is fine, but module output may collide across configs unless CMake places them per-config. If you hit issues, consider a config-specific path like${CMAKE_CURRENT_BINARY_DIR}/$<CONFIG>for the module dir.
If this library is only intended for in-tree builds right now, the current setup is solid.
There was a problem hiding this comment.
Those lines are doing the right thing for a Fortran static library that produces
.modfiles:* `Fortran_MODULE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}` Ensures all module outputs for target `odeint` are written to the target’s build dir. This avoids scattering `.mod` files and makes downstream consumption predictable. * `target_include_directories(odeint PUBLIC $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>)` Exposes that module directory to consumers _when building from the build tree_, so targets that `target_link_libraries(... odeint)` can find `use <module>` `.mod` files.A couple of suggested tweaks/considerations:
* **Install/export support**: if you ever plan to `install(TARGETS odeint EXPORT ...)`, you’ll likely also want an `INSTALL_INTERFACE` include dir and to install the `.mod` files, e.g. * `target_include_directories(odeint PUBLIC $<BUILD_INTERFACE:...> $<INSTALL_INTERFACE:include>)` * plus `install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/ DESTINATION include FILES_MATCHING PATTERN "*.mod")` (or better: use a dedicated module install dir). * **Multi-config generators** (Visual Studio, Xcode): `${CMAKE_CURRENT_BINARY_DIR}` is fine, but module output may collide across configs unless CMake places them per-config. If you hit issues, consider a config-specific path like `${CMAKE_CURRENT_BINARY_DIR}/$<CONFIG>` for the module dir.If this library is only intended for in-tree builds right now, the current setup is solid.
/ask But do you think that this current approach is okay then?
There was a problem hiding this comment.
Yes—this is a correct and common setup for an in-tree Fortran target that produces .mod files.
Fortran_MODULE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}keeps module outputs centralized (and avoids polluting other build dirs).target_include_directories(... $<BUILD_INTERFACE:...>)is the right way to make dependents find those.modfiles during the same build when they link toodeint.
A couple small, practical caveats to consider (only if they apply to your project):
- Multi-config generators: If you build multiple configs into the same binary dir (VS/Xcode),
.modcollisions can happen. If you support those, you may want:Fortran_MODULE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/$<CONFIG>- and mirror that in the
BUILD_INTERFACEinclude dir.
- Install/export: If you’re not installing/exporting
odeintyet, skippingINSTALL_INTERFACEand module installation is totally fine. If you ever do, you’ll need to add install rules for.modand anINSTALL_INTERFACEinclude path.
So: for “stand-alone library but used from the build tree” (your current scope), this approach is okay as-is.
|
@marjohma @phizenz @jonatanschatzlmayr I have finished my modifications of
stil run e.g. start your codes test suit for this version of libneo if your project depends on libneo. If you are sure that your code has no dependancy to in the meantime. 
|
In the CI check, only a shallow copy is used, so `git tag` does not return anything and the latest commit on the checkout branch is used i.e. the check is tautologic. Getting the tags from the remote origin and fetching it, makes the golden record accessible also in a shallow clone without local tags.
GeorgGrassler
temporarily deployed
to
github-pages
GitHub Actions
Inactive
|
@marjohma thank you for pointing out the CI issue. I did indeed "break" the CI test, but it turns out it was already "broken" (i.e. the CI check had problems with how it checksout the repo), but did still pass until I changed the folder structure. Basically, in the CI pipeline, libneo is only checkout as a shallow clone, that has no tags available. So it never fetches the actual golden record commit, but just uses the latest commit in the shallow clone. I have changed that now in the Sidenote: the failing tests that I mentioned previously, only appear in my local execution and they appear even if I return to the 
TD;LR: The changes seem to not break |
1 participant
No description provided.