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

[many ports] Remove _find_package guards that break *_FOUND #12157

Merged
merged 4 commits into from Aug 2, 2020

Conversation

endrift
Copy link
Contributor

@endrift endrift commented Jun 30, 2020

Describe the pull request

Several ports add guards before calling _find_package to prevent it from being called if a package's libraries and/or includes are in cache. However, *_FOUND variables are not in the cache, and since calling find_package is generally expected to invoke the actual Find*.cmake regardless, such guards are not only unnecessary (I'm not actually sure what the point of them was in the first place), but also break CMake lists using *_FOUND if CMake is ever re-run (and MSVC loooooooooves to rerun CMake every 3 seconds).

  • What does your PR fix? *_FOUND being missing if a package's libraries and/or includes are in cache

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

  • Does your PR follow the maintainer guide? Yes.

@JackBoosY JackBoosY self-assigned this Jun 30, 2020
@JackBoosY
Copy link
Contributor

@vicroms @ras0219 @ras0219-msft Could you please review this PR?

Thanks.

Copy link
Contributor

@ras0219 ras0219 left a comment

Choose a reason for hiding this comment

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

LGTM -- as a bonus, would you consider adding a section to the maintainer's guide about this?

@JackBoosY JackBoosY added category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed requires:author-response labels Jul 1, 2020
@endrift
Copy link
Contributor Author

endrift commented Jul 5, 2020

Wait, why? And why me? I don't see anything in there one way or the other right now. It just seemed to be a bit of a convention that broke things, but an ad hoc convention that wasn't formalized anywhere.

@cenit
Copy link
Contributor

cenit commented Jul 5, 2020

Being the author of many of them, I can just say that during the very long history of #5169 the requirement of calling find_package only if user had not already defined an include dir or library symbol become requested. That huge PR was then split into many different PRs with very long history each, so I cannot link you the exact comment about it.
If no regressions appear because of your PR, I am ok with that of course!

@endrift
Copy link
Contributor Author

endrift commented Jul 10, 2020

Interesting. I was wondering about the genesis of this choice. Not knowing why it was set up this way in the first place made it hard to know if it would break anything.

@endrift
Copy link
Contributor Author

endrift commented Jul 30, 2020

Other than me rebasing it, what's this PR waiting on?

@strega-nil
Copy link
Contributor

@endrift the PR testing to run. Once it runs, I'll merge it.

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Jul 30, 2020
@strega-nil
Copy link
Contributor

Looks like this depends on yasm on osx, so we should wait for #11535 which adds yasm on osx.

@strega-nil strega-nil added the depends:different-pr This PR or Issue depends on a PR which has been filed label Jul 31, 2020
@strega-nil
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@strega-nil strega-nil removed the depends:different-pr This PR or Issue depends on a PR which has been filed label Aug 1, 2020
@strega-nil strega-nil merged commit b46242f into microsoft:master Aug 2, 2020
hellozee pushed a commit to hellozee/vcpkg that referenced this pull request Sep 11, 2020
…t#12157)

* [many ports] Remove _find_package guards that break *_FOUND

* [many ports] Fix incrementing version

Co-authored-by: Jack·Boos·Yu <47264268+JackBoosY@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants