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.

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

[mpi/msmpi] Add cmake wrapper to fix bug getting MPI_${LANG}_ADDITIONAL_INCLUDE_DIRS when calling FindMPI.cmake on Windows #24746

Merged
merged 18 commits into from
May 26, 2022
Merged

[mpi/msmpi] Add cmake wrapper to fix bug getting MPI_${LANG}_ADDITIONAL_INCLUDE_DIRS when calling FindMPI.cmake on Windows #24746

merged 18 commits into from
May 26, 2022

Conversation

JackBoosY
Copy link
Contributor

When calling find_package(MPI REQUIRED), FindMPI.cmake provides wrong value of MPI_${LANG}_ADDITIONAL_INCLUDE_DIRS.
This can be reproduced by installing hdf5:

CMake Error in CMakeLists.txt:
  Target "libdeps" INTERFACE_INCLUDE_DIRECTORIES property contains path:

    "D:/buildtrees/highfive/src/v2.3-43259b649d.clean"

  which is prefixed in the source directory.

This is because FindMPI.cmake tried to get ENV{MSMPI_INC} value, but it's empty. so get_filename_component will use the current working directory:
https://github.com/Kitware/CMake/blob/943210856ae433708a7e12866e94f6dcaacc3248/Modules/FindMPI.cmake#L936-L940

        if(NOT MPI_${LANG}_ADDITIONAL_INCLUDE_DIRS)
          get_filename_component(MPI_MSMPI_INC_DIR "$ENV{MSMPI_INC}" REALPATH)
          set(MPI_${LANG}_ADDITIONAL_INCLUDE_DIRS "${MPI_MSMPI_INC_DIR}" CACHE STRING "MPI ${LANG} additional include directories" FORCE)
          unset(MPI_MSMPI_INC_DIR)
        endif()

If FindMPI.cmake does not declare to set this environment variable before calling it, it should be a cmake bug.

Fix this.

JackBoosY added 2 commits May 17, 2022 02:21
@JackBoosY JackBoosY added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels May 17, 2022
@JackBoosY JackBoosY mentioned this pull request May 17, 2022
Closed
github-actions[bot]
github-actions bot approved these changes May 17, 2022
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Use the version scheme "version" instead of "version-string" in port "mpi".

@JackBoosY JackBoosY changed the title [mpi] Add cmake wrapper to fix bug getting MPI_${LANG}_ADDITIONAL_INCLUDE_DIRS when calling FindMPI.cmake on Windows [mpi/msmpi] Add cmake wrapper to fix bug getting MPI_${LANG}_ADDITIONAL_INCLUDE_DIRS when calling FindMPI.cmake on Windows May 19, 2022
github-actions[bot]
github-actions bot approved these changes May 19, 2022
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Use the version scheme "version" instead of "version-string" in port "mpi".

JackBoosY added 2 commits May 19, 2022 00:05
github-actions[bot]
github-actions bot approved these changes May 19, 2022
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Use the version scheme "version" instead of "version-string" in port "mpi".

@JackBoosY
Copy link
Contributor Author

@Neumann-A @dg0yt @cenit Should be okay now?

@LilyWangLL LilyWangLL added the info:reviewed Pull Request changes follow basic guidelines label May 23, 2022
dg0yt
dg0yt suggested changes May 23, 2022
View reviewed changes
ports/mpi/portfile.cmake Outdated Show resolved Hide resolved
ports/msmpi/vcpkg-cmake-wrapper.cmake Outdated Show resolved Hide resolved
ports/msmpi/vcpkg-cmake-wrapper.cmake Outdated Show resolved Hide resolved
ras0219-msft
ras0219-msft requested changes May 23, 2022
View reviewed changes
ports/msmpi/portfile.cmake Outdated Show resolved Hide resolved
ports/mpi/portfile.cmake Outdated Show resolved Hide resolved
github-actions[bot]
github-actions bot approved these changes May 24, 2022
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Use the version scheme "version" instead of "version-string" in port "mpi".

github-actions[bot]
github-actions bot approved these changes May 24, 2022
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Use the version scheme "version" instead of "version-string" in port "mpi".

github-actions[bot]
github-actions bot approved these changes May 24, 2022
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Use the version scheme "version" instead of "version-string" in port "mpi".

JackBoosY added 2 commits May 24, 2022 00:20
github-actions[bot]
github-actions bot approved these changes May 24, 2022
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Use the version scheme "version" instead of "version-string" in port "mpi".

github-actions[bot]
github-actions bot approved these changes May 24, 2022
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Use the version scheme "version" instead of "version-string" in port "mpi".

ras0219-msft
ras0219-msft requested changes May 24, 2022
View reviewed changes
ports/msmpi/portfile.cmake Outdated Show resolved Hide resolved
@ras0219-msft ras0219-msft removed the info:reviewed Pull Request changes follow basic guidelines label May 24, 2022
github-actions[bot]
github-actions bot approved these changes May 25, 2022
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Use the version scheme "version" instead of "version-string" in port "mpi".

@JackBoosY JackBoosY requested a review from ras0219-msft May 26, 2022 06:05
@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label May 26, 2022
@ras0219-msft ras0219-msft merged commit 75536e7 into microsoft:master May 26, 2022
@ras0219-msft
Copy link
Contributor

LGTM, thanks everyone!

@JackBoosY JackBoosY deleted the dev/jack/fix-msmpi-findmpi branch May 27, 2022 02:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Neumann-A Neumann-A Neumann-A left review comments

@cenit cenit cenit left review comments

@dg0yt dg0yt dg0yt requested changes

@LilyWangLL LilyWangLL LilyWangLL approved these changes

@github-actions github-actions[bot] github-actions[bot] approved these changes

@BillyONeal BillyONeal Awaiting requested review from BillyONeal

@ras0219-msft ras0219-msft Awaiting requested review from ras0219-msft

Assignees

@LilyWangLL LilyWangLL

Labels
category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@JackBoosY @ras0219-msft @cenit @dg0yt @Neumann-A @LilyWangLL