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

Remove HAVE_ECL_INPUT and HAVE_ECL_OUTPUT conditionals #1129

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

Conversation

akva2
Copy link
Member

@akva2 akva2 commented Oct 18, 2019

opm-common (should) always provide these. closes #1127

@akva2
Copy link
Member Author

akva2 commented Oct 18, 2019

jenkins build this opm-material=352 opm-grid=401 opm-models=566 opm-simulators=2072 opm-upscaling=283 please

@alfbr
Copy link
Member

alfbr commented Oct 18, 2019

Please refrain from merging until we have broad community support for the change.

@totto82
Copy link
Member

totto82 commented Oct 18, 2019

I agree with @alfbr. I don't think this should be merged before we have a broad discussion about the topic. Including the repository structures as we have it now. As Libecl is gone we may think differently about the topic. If I remember correctly the conditional dependency was to a large degree motivated from the libecl dependency. Currently there are simulators in opm-models that don't depend on opm-common. Maybe we should keep the conditionals dependency on HAVE_ECL_INPUT and instead remove the conditional dependency on opm-common in opm-models? Maybe a hangout on the topic after the release?

@akva2
Copy link
Member Author

akva2 commented Oct 18, 2019

jenkins build this opm-material=352 opm-grid=401 opm-models=566 opm-simulators=2072 opm-upscaling=283 please

@joakim-hove
Copy link
Member

Since I suggested this in #1127 I would obviously like to merge this; not a big thing - but it will reduce complexity somewhat. I acknowledge the wish for a broader discussion before a decision is made; but that leads to an important point: How do actually reach a consensus on questions like this?

@atgeirr
Copy link
Member

atgeirr commented Oct 22, 2019

How do actually reach a consensus on questions like this?

I support @totto82' suggestion to have a hangout about the topic (future module structure and dependencies) with interested participants after the release.

@joakim-hove
Copy link
Member

Time to merge this?

@totto82
Copy link
Member

totto82 commented Apr 20, 2021

Time to merge this?

I am not against merging this, but I would like to keep the opportunity to include opm-grid opm-material and opm-models as modules using the dune build system. I.e. the downstream changes should be HAVE_ECL_INPUT/OUTPUT--> HAVE_OPM_COMMON, IMO.
I am not sure about opm-grid, but opm-material and opm-models used to build without opm-common using dunecontrol.

@joakim-hove
Copy link
Member

OK - we have had 1.5 years with no real objections. Time to revive this.

@blattms
Copy link
Member

blattms commented Apr 23, 2021

I am not sure about opm-grid, but opm-material and opm-models used to build without opm-common using dunecontrol

That must have been a time when the buildsystem was replicated in every module. I doubt that it is still the case.

But even if opm-common is needed it might make sense to deactivate ECL (it takes a lot of time and if you don't need it, you might not want to build it ....).

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.

Remove conditionals ENABLE_ECL_INPUT and ENABLE_ECL_OUTPUT
6 participants