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

Export CMake Module #236

Merged
merged 1 commit into from
Jul 12, 2022
Merged

Export CMake Module #236

merged 1 commit into from
Jul 12, 2022

Conversation

jh66637
Copy link
Contributor

@jh66637 jh66637 commented Jun 28, 2022

This PR tries to simplify using ExaDG in external projects.
Linking to ExaDG would look like the following:

find_package(EXADG REQUIRED)
target_link_libraries(<target_which_links_to_exadg> EXADG::exadg)
target_include_directories(<target_which_links_to_exadg> ${EXADG_INCLUDE_DIRS})

@nfehn nfehn added the build system cmake, configuration, scripts label Jun 28, 2022
EXADGConfig.cmake.in Outdated Show resolved Hide resolved

@PACKAGE_INIT@

get_filename_component(EXADG_CMAKE_DIR "${CMAKE_CURRENT_LIST_FILE}" PATH)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is EXADG_CMAKE_DIR used?

i do not really understand what EXADGConfig.cmake.in is doing? Maybe you could explain this a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EXADGConfig.cmake.in serves as a template. Running CMake will create EXADGConfig.cmake and EXADGTargets.cmake from it in the build folder. These files are needed to use the FIND_PACKAGE command in other projects.

EXADG_CMAKE_DIR as well as EXADG_INCLUDE_DIRS are available after the FIND_PACKAGE call and can be used in an external project. See e.g. the description of the pull request, where EXADG_INCLUDE_DIRS is used to make the include directories available in the external project. The more we export, the easier it will be to interface. With EXADG_CMAKE_DIR the user has the build directory of EXADG at his fingertips, even though it might not be needed by everybody.

Similar to that, I also export EXADG_VERSION which is currently not set since EXADG currently has no version number. In the future this might be helpful though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me this looks like great work! To get the present PR completed, I would like you to include parts of the above answer as comments in the code.

In CMakeLists.txt, please describe this:

EXADGConfig.cmake.in serves as a template. Running CMake will create EXADGConfig.cmake and EXADGTargets.cmake from it in the build folder. These files are needed to use the FIND_PACKAGE command in other projects.

At the end of CMakeLists.txt, you should write the three lines in the description of this PR how to use ExaDG in other projects. This is very helpful for us because not everyone is familiar with CMake.

find_package(EXADG REQUIRED)
target_link_libraries(<target_which_links_to_exadg> EXADG::exadg)
target_include_directories(<target_which_links_to_exadg> ${EXADG_INCLUDE_DIRS})

In the EXADGConfig.cmake.in file, you should describe the variables that are exported:

EXADG_CMAKE_DIR as well as EXADG_INCLUDE_DIRS are available after the FIND_PACKAGE call and can be used in an external project. Similarly, EXADG_VERSION is also exported (which might be helpful in the future once EXADG has a version number).

As a minor comment, could you remove multiple empty lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done :)

@jh66637 jh66637 force-pushed the cmake_export branch 4 times, most recently from 7fdec00 to 80ac153 Compare June 29, 2022 08:13
@jh66637
Copy link
Contributor Author

jh66637 commented Jun 29, 2022

I am also exporting the ExaDG CMake macros. Currently EXADG_PICKUP_EXE can not be used in external projects since it links against exadg instead EXADG::exadg. If you are interested to use the macros in external projects, I can give it a try this evening. This will need quite a few changes, though. It would be easy to fix if the export is done without the namespace, but I like the export better with the namespace.

@jh66637
Copy link
Contributor Author

jh66637 commented Jun 29, 2022

I had some time during lunch. Making the macros available for external projects was not too bad at all.
The needed changes are in the second commit. If you think this is helpful, I will squash the commits.

While doing the changes, I removed the global INCLUDE_DIRECTORIES and changed it into TARGET_INCLUDE_DIRECTORIES. I don't know if this breaks something locally for you. For the macros to work, this is currently necessary. In general, I think this should be the way to go.

@peterrum
Copy link
Member

@jh66637 You seem to like CMake. Next time I have a question I know whom to contact^^

@nfehn @bergbauer Before merging, can anyone of you try out the new configuration with our internal applications. Thx!

@jh66637
Copy link
Contributor Author

jh66637 commented Jul 5, 2022

While experimenting with a project that is based on ExaDG I realized it is not possible to add kernels with polynomial degrees up to EXADG_DEGREE_MAX just by linking to ExaDG. I don't know why, but it seems each library has to instantiate the FEEvaluation and FEFaceEvaluation classes itself.

I am currently working on a configuration to achieve this in a generic way. I will keep you posted.

@jh66637
Copy link
Contributor Author

jh66637 commented Jul 6, 2022

It seems not to be related to the linkage, but is probably related to Issue #241 (comment)

@kronbichler
Copy link
Contributor

ExaDG I realized it is not possible to add kernels with polynomial degrees up to EXADG_DEGREE_MAX just by linking to ExaDG.

I think the problem is indeed the different definitions coming from different libraries - it seems that the linker/runtime loader of dynamic libraries picks the version from ExaDG when it makes this check https://github.com/dealii/dealii/blob/cfebfb5e1114f93755136675369d0c6b89829304/include/deal.II/matrix_free/fe_evaluation.h#L8643-L8645 here (which goes into internal::FEEvaluationFactory<dim, VectorizedArrayType>::fast_evaluation_supported), whereas the code picked up here https://github.com/dealii/dealii/blob/cfebfb5e1114f93755136675369d0c6b89829304/include/deal.II/matrix_free/fe_evaluation.h#L8790-L8799 picks up the code from libdeal_II.so, which might have a different values and hence create a problem. From within ExaDG, we likely never have a problem because the libexadg.so library always comes first from the symbols, but from external projects things are not so clear. Overall, this process has turned out to be more brittle than I had hoped for 2 years ago when we introduced it. I'm wondering if we should choose a different approach:

  • Recommend users to add -D DEAL_II_CXX_FLAGS="-DFE_EVAL_FACTORY_DEGREE_MAX=9" or whatever value is desired to receive fast routines to the compilation of deal.II.
  • In
    dealii::Patterns::Integer(1,EXADG_DEGREE_MAX),
    and similar places, allow any non-negative degree, but in case we are outside of the FE_EVAL_FACTORY_DEGREE_MAX, i.e., FEEvaluation::fast_evaluation_supported(degree, degree+1) returns false, print a warning at the place where we parse the polynomial degree that the user will not get optimal-performance because it is not a pre-compiled degree of deal.II, and instruct them to re-compile deal.II if the performance is desired?

What do you think?

@peterrum
Copy link
Member

peterrum commented Jul 6, 2022

Recommend users to add -D DEAL_II_CXX_FLAGS="-DFE_EVAL_FACTORY_DEGREE_MAX=9" or whatever value is desired to receive fast routines to the compilation of deal.II.

Sounds reasonable. See also my comment here: dealii/dealii#13591 (comment)

@peterrum
Copy link
Member

peterrum commented Jul 6, 2022

Regarding the check, one can write a new pattern that replaces

https://github.com/dealii/dealii/blob/cfebfb5e1114f93755136675369d0c6b89829304/source/base/patterns.cc#L234-L235

by the FEEvaluation::fast_evaluation_supported(degree, degree+1) call. This way nothing would change to the user and the warning would come already during parsing of the parameter.

@nfehn
Copy link
Member

nfehn commented Jul 6, 2022

I suggest to shift this discussion first to #241. Otherwise, things just become more complicated for ExaDG users.

@jh66637
Copy link
Contributor Author

jh66637 commented Jul 12, 2022

I updated this after #244. On my machine everything works as expected .

@bergbauer
Copy link
Member

Works perfectly fine for my application! Thanks!

What is the intention of a namespace like EXADG:: in general? Is it useful if several targets are exported?

@jh66637
Copy link
Contributor Author

jh66637 commented Jul 12, 2022

Kind of. The namespace itself is mostly to avoid name clashes with other libraries which might be used. However, having multiple targets, the chances of name clashes increases. Also, having multiple targets, it is clear where those come from.

As you said, it would be nice to split ExaDG into multiple targets, then it is possible to only link to the required part of ExaDG. This is nice, because changing something in e.g. EXADG::incompressible_nse would not affect EXADG::compressible_nse and only the incompressible lib has to be rebuilt and relinked.

@nfehn
Copy link
Member

nfehn commented Jul 12, 2022

@jh66637 is this PR ready to be merged from your side, or would you like to still work on it?

@bergbauer
Copy link
Member

As you said, it would be nice to split ExaDG into multiple targets, then it is possible to only link to the required part of ExaDG. This is nice, because changing something in e.g. EXADG::incompressible_nse would not affect EXADG::compressible_nse and only the incompressible lib has to be rebuilt and relinked.

Should we discuss this in an issue? WDYT? @nfehn

@nfehn
Copy link
Member

nfehn commented Jul 12, 2022

I think that we should not split ExaDG in different parts right now. For me, this feels like over-engineering right now.

@jh66637
Copy link
Contributor Author

jh66637 commented Jul 12, 2022

PR is finished from my side :)

@nfehn nfehn merged commit 81b6383 into exadg:master Jul 12, 2022
@jh66637 jh66637 deleted the cmake_export branch July 12, 2022 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build system cmake, configuration, scripts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants