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

Move Dynamic Dispatch Function to Separate TU #5928

Merged
merged 1 commit into from
Jan 30, 2025

Conversation

bska
Copy link
Member

@bska bska commented Jan 29, 2025

This moves about 300 lines of code out of Main.hpp and, especially, moves the <flow/flow_*.hpp> include statements as well. This, in turn, makes Main::runStatic<>() usable for out-of-tree consumers.

@bska
Copy link
Member Author

bska commented Jan 29, 2025

I am not sure that this is the best way to go about solving my problem, but my main goal here–in support of #5801–is to make Main::runStatic<>() usable for out-of-tree consumers with custom TypeTag implementations. Having the #include <flow/flow_*.hpp> statements directly in Main.hpp makes that goal more difficult and effectively leads to having to install those headers despite them not having any publicly accessible symbols.

I welcome all suggestions for alternative strategies here.

@bska
Copy link
Member Author

bska commented Jan 29, 2025

jenkins build this please

@bska bska force-pushed the main-dispatch-dynamic-tu branch from 44d17aa to 1b292cc Compare January 29, 2025 19:41
@akva2
Copy link
Member

akva2 commented Jan 30, 2025

This entirely makes sense to me, but we need to make the FlowProblemTPFA available in some contexts where it is now missing (build with python). While that particular problem is solvable by including flow_blackoil.hpp in PyMain.hpp, for ..reasons (some machine learning extensions for flow that i'm maintaining), I'd really like if we could put it in a separate (installed) header.

@bska bska force-pushed the main-dispatch-dynamic-tu branch from 1b292cc to 2c84bcd Compare January 30, 2025 09:35
@bska
Copy link
Member Author

bska commented Jan 30, 2025

we need to make the FlowProblemTPFA available in some contexts where it is now missing (build with python)

Right. I made this PR based on a build configuration without EMBEDDED_PYTHON so I didn't run into the problem until I tried a build check.

While that particular problem is solvable by including flow_blackoil.hpp in PyMain.hpp

PR #5930 does essentially this.

I'd really like if we could put it in a separate (installed) header.

The FlowProblemTPFA you mean?

@akva2
Copy link
Member

akva2 commented Jan 30, 2025

yes please. while it is very few lines of code, I'd really like to not duplicate them in my downstream. It can potentially mean ODR violations should anything change and that is more maintenance...

@bska
Copy link
Member Author

bska commented Jan 30, 2025

yes please. while it is very few lines of code, I'd really like to not duplicate them in my downstream.

👍

Any preference for the location? Do we still keep it in opm-simulators/flow, or is there some other location that might be better?

@akva2
Copy link
Member

akva2 commented Jan 30, 2025

i'd say put it in opm/simulators/flow to keep the opm-simulators/flow folder strictly no-install.

@bska
Copy link
Member Author

bska commented Jan 30, 2025

i'd say put it in opm/simulators/flow to keep the opm-simulators/flow folder strictly no-install.

Good idea. Please have a look at PR #5931.

@bska bska force-pushed the main-dispatch-dynamic-tu branch 3 times, most recently from b77e9fe to 5e5b32b Compare January 30, 2025 11:52
@bska
Copy link
Member Author

bska commented Jan 30, 2025

The earlier, preparatory, PRs have been merged. I'm marking this as "ready for review" and I welcome any comments you may have.

@bska bska marked this pull request as ready for review January 30, 2025 11:53
@bska
Copy link
Member Author

bska commented Jan 30, 2025

jenkins build this please

opm/simulators/flow/Main.hpp Outdated Show resolved Hide resolved
This moves about 300 lines of code out of Main.hpp and, especially,
moves the <flow/flow_*.hpp> include statements as well.  This, in
turn, makes Main::runStatic<>() usable for out-of-tree consumers.
@bska bska force-pushed the main-dispatch-dynamic-tu branch from 5e5b32b to 1bf755f Compare January 30, 2025 12:18
@bska
Copy link
Member Author

bska commented Jan 30, 2025

jenkins build this please

@bska
Copy link
Member Author

bska commented Jan 30, 2025

PR approved and build check is green. I'll merge into master.

@bska bska merged commit 1df6079 into OPM:master Jan 30, 2025
1 check passed
@bska bska deleted the main-dispatch-dynamic-tu branch January 30, 2025 12:53
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.

None yet

2 participants