Skip to content

Multiple find package pdi #610

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.

Sign up for GitHub

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

Jump to bottom
Open
benoitmartin88 wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Multiple find package pdi #610

benoitmartin88 wants to merge 24 commits into from

Conversation

@benoitmartin88
Copy link
Member

@benoitmartin88 benoitmartin88 commented Dec 2, 2025 •
edited
Loading

List of things to check before making a PR

Before merging your code, please check the following:

  • you have added a line describing your changes to the Changelog;
  • you have added unit tests for any new or improved feature;
  • In case you updated dependencies, you have checked pdi/docs/CheckList.md
  • you have checked your code format:
    • you have checked that you respect all conventions specified in CONTRIBUTING.md;
    • you have checked that the indentation and formatting conforms to the .clang-format;
    • you have documented with doxygen any new or changed function / class;
  • you have correctly updated the copyright headers:
    • your institution is in the copyright header of every file you (substantially) modified;
    • you have checked that the end-year of the copyright there is the current one;
  • you have updated the AUTHORS file:
    • you have added yourself to the AUTHORS file;
    • if this is a new contribution, you have added it to the AUTHORS file;
  • you have added everything to the user documentation:
    • any new CMake configuration option;
    • any change in the yaml config;
    • any change to the public or plugin API;
    • any other new or changed user-facing feature;
    • any change to the dependencies;
  • you have correctly linked your MR to one or more issues:
    • your MR solves an identified issue;
    • your commit contain the Fix #issue keyword to autoclose the issue when merged.

@benoitmartin88 benoitmartin88 self-assigned this Dec 2, 2025
@tpadioleau
Copy link
Contributor

tpadioleau commented Dec 2, 2025

Thank you @benoitmartin88 I think it should fix CExA-project/ddc#964. A naive question that comes to me is, are these global targets (PDI_C, PDI_f90, ...) necessary at all ? I think there are two sources of problems for consumers:

  • they have global scope,
  • they are defined outside of the PDI namespace.

@benoitmartin88 benoitmartin88 marked this pull request as ready for review December 5, 2025 07:31
@benoitmartin88
Copy link
Member Author

benoitmartin88 commented Dec 5, 2025

@jbigot Could you take a look ?

Yushan-Wang
Yushan-Wang previously approved these changes Dec 5, 2025
View reviewed changes
jmorice91
jmorice91 reviewed Dec 8, 2025
View reviewed changes
Yushan-Wang
Yushan-Wang reviewed Dec 10, 2025
View reviewed changes
@tpadioleau
Copy link
Contributor

tpadioleau commented Dec 14, 2025

Do you think it is wortk keeping the message(WARNING "...") ? It displays

CMake Warning at opt/pdi/share/pdi/cmake/PDIConfig.cmake:144 (message):
  Target PDI_f90 is already defined.  Skipping.

How should the user react seeing this message ?

@benoitmartin88
Copy link
Member Author

benoitmartin88 commented Dec 15, 2025

You are right, this warning might be a little too aggressive. I'm going to dial it down to "normal" cmake message.

jbigot
jbigot reviewed Dec 16, 2025
View reviewed changes
bin/build_and_run_all_tests
cd "$(mktemp -d pdibuild.XXXXX)"
TEST_DIR="${PWD}"

INSTALL_DIR="${INSTALL_DIR:-/tmp/pdi_install_dir}"
Copy link
Member

@jbigot jbigot Dec 15, 2025

Choose a reason for hiding this comment

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

Suggested change
INSTALL_DIR="${INSTALL_DIR:-/tmp/pdi_install_dir}"
INSTALL_DIR="${INSTALL_DIR:-"$(mktemp --tmpdir -d pdi_install_dir_XXXXXXX)"}"

bin/build_and_run_all_tests
-DBUILD_USER_CODE_PLUGIN=ON"

DYLD_LIBRARY_PATH=${INSTALL_DIR}/lib:$DYLD_LIBRARY_PATH
CMAKE_TEST_FLAGS="-DTEST_FORTRAN=OFF -DTEST_PYTHON=OFF"
Copy link
Member

@jbigot jbigot Dec 15, 2025

Choose a reason for hiding this comment

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

Why do we need to explicitly set DYLD_LIBRARY_PATH? Shouldn't pdirun be enough?

If we do not build neither Fortran nor Python, why do we need to specify that we don't test them again?

bin/build_and_run_all_tests
Comment on lines +139 to +140
cd /tmp
cd "$(mktemp -d pdicmaketestdir.XXXXX)"
Copy link
Member

@jbigot jbigot Dec 16, 2025

Choose a reason for hiding this comment

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

Suggested change
cd /tmp
cd "$(mktemp -d pdicmaketestdir.XXXXX)"
cd "$(mktemp --tmpdir -d pdicmaketestdir.XXXXXXX)"

pdi/cmake/PDIConfig.cmake.in
add_executable(PDI::pdirun IMPORTED)
set_target_properties(PDI::pdirun PROPERTIES IMPORTED_LOCATION "${PDI_pdirun_EXECUTABLE}")
else()
message("Target PDI::pdirun is already defined. Skipping.")
Copy link
Member

@jbigot jbigot Dec 16, 2025

Choose a reason for hiding this comment

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

We should handle the QUIET parameter here

pdi/cmake/PDIConfig.cmake.in
add_library(PDI::PDI_Fortran ALIAS PDI_f90)
add_library(PDI::pdi_f90 ALIAS PDI_f90)
else()
message("Target PDI_f90 is already defined. Skipping.")
Copy link
Member

@jbigot jbigot Dec 16, 2025

Choose a reason for hiding this comment

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

We should handle the QUIET parameter here

pdi/cmake/PDIConfig.cmake.in
target_link_libraries(PDI_C INTERFACE PDI::PDI_C)
add_library(PDI::pdi ALIAS PDI_C)
else()
message("Target PDI_C is already defined. Skipping.")
Copy link
Member

@jbigot jbigot Dec 16, 2025

Choose a reason for hiding this comment

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

this should be made a lower priority message and QUIET should be honored

pdi/cmake/PDIConfig.cmake.in
Comment on lines +133 to +138
# Note: temporary fix for oldest supported cmake. target will have global scope with cmake version < 3.18
if (CMAKE_MAJOR_VERSION VERSION_EQUAL "3" AND CMAKE_MINOR_VERSION VERSION_GREATER_EQUAL "18")
add_library(PDI_f90 INTERFACE IMPORTED)
else()
add_library(PDI_f90 INTERFACE)
endif()
Copy link
Member

@jbigot jbigot Dec 16, 2025

Choose a reason for hiding this comment

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

Suggested change
# Note: temporary fix for oldest supported cmake. target will have global scope with cmake version < 3.18
if (CMAKE_MAJOR_VERSION VERSION_EQUAL "3" AND CMAKE_MINOR_VERSION VERSION_GREATER_EQUAL "18")
add_library(PDI_f90 INTERFACE IMPORTED)
else()
add_library(PDI_f90 INTERFACE)
endif()
add_library(PDI_f90 INTERFACE IMPORTED)

pdi/CHANGELOG.md
#### Removed

#### Fixed
* Fix multiple consecutive calls to find_package(PDI)
Copy link
Member

@jbigot jbigot Dec 16, 2025

Choose a reason for hiding this comment

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

Suggested change
* Fix multiple consecutive calls to find_package(PDI)
* Support multiple reentrant calls to find_package(PDI)

tests/test_cmake/CMakeLists.txt
Comment on lines +39 to +46
# check subdirectory scope
# Note: temporary deactivate due to cmake version <3.18 bug with target alias
#message(STATUS "+ add_subdirectory(PDI)")
#add_subdirectory(subdir)
#assert(NOT DEFINED PDI_FOUND)
#assert(NOT TARGET PDI_C)
#assert(NOT TARGET PDI_f90)
#assert(NOT TARGET PDI_pysupport)
Copy link
Member

@jbigot jbigot Dec 16, 2025

Choose a reason for hiding this comment

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

We should move to 3.22 as a minimum. Also this can be conditionnally deactivated base on cmake version

bin/build_and_run_all_tests
make install
cd /tmp
cd "$(mktemp -d pdicmaketestdir.XXXXX)"
${INSTALL_DIR}/bin/pdirun cmake ${CMAKE_TEST_FLAGS} ${SRCDIR}/tests/test_cmake
Copy link
Member

@jbigot jbigot Dec 16, 2025

Choose a reason for hiding this comment

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

Another option would be to call this from a bash script called through ctest. This would make it a normal test. Do you think this is feasible?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jbigot jbigot jbigot left review comments

@Yushan-Wang Yushan-Wang Yushan-Wang left review comments

@jmorice91 jmorice91 jmorice91 left review comments

@JAuriac JAuriac Awaiting requested review from JAuriac

At least 2 approving reviews are required to merge this pull request.

Assignees

@benoitmartin88 benoitmartin88

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

find_package(PDI) cannot be called twice

6 participants

@benoitmartin88 @tpadioleau @jbigot @Yushan-Wang @jmorice91 @JAuriac