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 GeosxConfig.hpp file that should not be in the repo #2502

Closed
wants to merge 2 commits into from

Conversation

paveltomin
Copy link
Contributor

Remove src/docs/doxygen/GeosxConfig.hpp

@paveltomin paveltomin self-assigned this Jun 1, 2023
@paveltomin paveltomin marked this pull request as ready for review June 1, 2023 16:27
@klevzoff
Copy link
Contributor

klevzoff commented Jun 1, 2023

We actually keep this file in the repo on purpose. It is needed to build doxygen docs on readthedocs, which lacks the full build environment needed to generate this header (e.g. cmake and dependencies).

We have a "solution" for avoiding the diffs to this file between platforms, but it has not been updated with the addition of new macro definitions. I would suggest turning this PR into a fix for that rather than a deletion of the header. If anyone has a better long-term solution, I'm interested!

@paveltomin paveltomin marked this pull request as draft June 1, 2023 18:09
@paveltomin paveltomin closed this Jun 1, 2023
@untereiner
Copy link

I was wondering, why are config values like the build type in the documentation ? Doxygen doesn’t need them for the generation process. Am I missing something ?

@klevzoff
Copy link
Contributor

klevzoff commented Jun 2, 2023

I was wondering, why are config values like the build type in the documentation ? Doxygen doesn’t need them for the generation process. Am I missing something ?

The value of this GEOSX_CMAKE_BUILD_TYPE macro is used in code in one place.

Basically, we want that header to be more or less an exact copy of the one a developer will find in their build directory and included everywhere in GEOS (up to the values that we "fix"). The doxygen build will not fail without it, but will be "incomplete" in some sense. E.g. someone inspecting source on readthedocs won't be able to navigate to GeosxConfig.hpp or jump to the documentation of one of these macros they encounter in code.

We just need to apply the "fix" for the missing macros and forget about this problem. We may also want to consider splitting the "dependency version" macros into a separate header and not store that one in the repo, since those are likely to change often.

@untereiner
Copy link

untereiner commented Jun 2, 2023

Basically, we want that header to be more or less an exact copy of the one a developer will find in their build directory and included everywhere in GEOS (up to the values that we "fix")

Ok I understand the incompleteness of the hyperlinked documentation. But for me it is a configuration file very dependent of what a developer locally configured and compiled. Maybe it is on the GeosxConfig.hpp.in to be documented (it is a source file) ? Doxygen should be able to deal with the #cmakedefine

@paveltomin paveltomin deleted the pt/remove-geosxconfig-hpp branch June 21, 2023 16:20
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.

3 participants