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

[cudnn] Fix port: install find module, add usage #17644

Merged
merged 3 commits into from
May 18, 2021

Conversation

jacobkahn
Copy link
Contributor

  • What does your PR fix?

#17331 removed the installation of FindCUDNN.cmake, which breaks the port for downstream projects that have find_dependency calls that might require a FindCUDNN module. cc @BillyONeal

Add back the installation of the module in addition to a usage.

  • Which triplets are supported/not supported? Have you updated the CI baseline?

No change

Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Yes

@@ -70,3 +70,6 @@ elseif(VCPKG_TARGET_IS_WINDOWS)
else()
message(FATAL_ERROR "Please install CUDNN using your system package manager (the same way you installed CUDA). For example: apt install libcudnn8-dev.")
endif()

file(INSTALL "${CURRENT_PORT_DIR}/FindCUDNN.cmake" DESTINATION ${CURRENT_PACKAGES_DIR}/share/${PORT})
Copy link
Member

Choose a reason for hiding this comment

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

I think we're supposed to install Xxx-config.cmake or XxxConfig rather than FindXxx? (Hoping a better cmake expert than I can point to policy/rationale but I've heard this before)

Copy link
Contributor

Choose a reason for hiding this comment

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

well, at the beginning modules were preferred for non-cmake-built projects, hoping that upstream (in the project or in cmake itself) they could find a final home after some development here

imho, this is still the best approach (and still applied in many other ports, see ffmpeg for example)

Copy link
Member

Choose a reason for hiding this comment

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

I see. In that case pay no attention to the Billy behind the curtain… 😂

@JackBoosY JackBoosY added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label May 4, 2021
@NancyLi1013 NancyLi1013 added the info:reviewed Pull Request changes follow basic guidelines label May 7, 2021
@dan-shaw dan-shaw merged commit a0bed69 into microsoft:master May 18, 2021
@cenit cenit mentioned this pull request May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants