-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
[tiff] Fix find_package in cmake wrapper #18473
Conversation
I will update this PR to set CMP0057 to NEW, for using |
I don't think this is the right solution, because it results in very strange behavior: Instead, I think the right approach is to unset |
More generally, I think we need to move to a different mechanism than |
Yes.
Without this PR, simage[tiff] wouldn't build at all. One might argue that this is also a valid and desired outcome:
From that POV, simage[tiff] must opt out of tiff's jpeg feature. PS I see @ras0219-msft just posted another comment which seems to support this POV. |
Uppdate simage to 1.8.1 ? They changed all find_package call to |
@dg0yt So I suggest you to update simage to 1.8.1 first. |
@JackBoosY I'm not going to start with simage. I don't use it. I don't test with windows triplets outside CI. It is not clear to me what is the desired state for image format support:
|
This work is complete. However, there should be public guidelines for cmake wrappers. And there is #18631. |
PR #18645 upgrades the simage to 1.8.1. |
Ping @ras0219 @ras0219-msft for respose. |
Ping @ras0219-msft for review this again. |
@ras0219-msft In the light of writing down best practices for wrappers, there are two issues I want to point out.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!
After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 6b24f0fa76d171c5756f01f407682f7274cfe238 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/t-/tiff.json b/versions/t-/tiff.json
index 5c84ed3..29367b7 100644
--- a/versions/t-/tiff.json
+++ b/versions/t-/tiff.json
@@ -1,7 +1,7 @@
{
"versions": [
{
- "git-tree": "aab7feb7ca21fc2343fff20bf27d92a95d0b5e69",
+ "git-tree": "f147253c84ad1d911393d8b5496f30574a07c780",
"version": "4.3.0",
"port-version": 2
},
Ping for review. |
I think I can accept this. |
This makes sense to me but I want @ras0219 / @ras0219-msft to OK it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a solid improvement, though it can't fully resolve the issue until we implement some non-transitive blocking capabilities for find_package
.
[tiff] Fix find_package in cmake wrapper (microsoft#18473)
What does your PR fix?
REQUIRED
when finding feature dependencies of tiff, properly respect the options given tofind_package(TIFF)
.Requires at least CMake 3.3 for project mode (as do some other wrappers).
Follow-up from [meson] Update meson to 0.58.1 #18393 (comment) - CC @Neumann-A
if ("name" IN_LIST FEATURES)
if(@<feature_var@>)
.Which triplets are supported/not supported? Have you updated the CI baseline?
all / no.
Does your PR follow the maintainer guide?
yes
If you have added/updated a port: Have you run
./vcpkg x-add-version --all
and committed the result?yes