[vcpkg baseline][kf5solid] Fix usage

[JackBoosY]

Kf5solid requires libmount in Liunx, but doesn't add find_dependency(libmount) in its config.cmake file.

Fix that.

[JackBoosY]

cc @wrobelda, it's a upstream bug.

[wrobelda]

@JackBoosY LGTM. The #20938 has that fixed already upstream, so it will require rebasing once yours get accepted.

[JackBoosY]

@wrobelda Since there are so many changes in #20938. can we merge this PR first?

[wrobelda]

@wrobelda Since there are so many changes in #20938. can we merge this PR first?

Yes, absolutely. That's the right thing to do.

[JackBoosY]

We have encountered a recursive dependency. The test of this PR depends on #21341, @BillyONeal please merge this PR or #21341 without the pipeline test passes.

[Neumann-A]

I still think you need to add -DCMAKE_DISABLE_FIND_PACKAGE_LibMount=ON for the portfile because otherwise binary caching is screwed. (Otherwise update cmake to 3.22 and add CMAKE_REQUIRE_FIND_PACKAGE_<PackageName>)

[BillyONeal]

I still think you need to add -DCMAKE_DISABLE_FIND_PACKAGE_LibMount=ON for the portfile because otherwise binary caching is screwed. (Otherwise update cmake to 3.22 and add CMAKE_REQUIRE_FIND_PACKAGE_<PackageName>)

Either that or patch REQUIRED into all the libmount places used by upstream?

[JackBoosY]

I still think you need to add -DCMAKE_DISABLE_FIND_PACKAGE_LibMount=ON for the portfile because otherwise binary caching is screwed. (Otherwise update cmake to 3.22 and add CMAKE_REQUIRE_FIND_PACKAGE_<PackageName>)

Either that or patch REQUIRED into all the libmount places used by upstream?

We should only add this for every port that uses libmount.

[wrobelda]

I still think you need to add -DCMAKE_DISABLE_FIND_PACKAGE_LibMount=ON for the portfile because otherwise binary caching is screwed. (Otherwise update cmake to 3.22 and add CMAKE_REQUIRE_FIND_PACKAGE_<PackageName>)

@Neumann-A, @BillyONeal Can you please elaborate why is this needed at all? And wouldn't -DCMAKE_DISABLE_FIND_PACKAGE_LibMount=ON disable finding LibMount entirelyl, in which case the KF5SolidConfig.cmake.in patch is pointless? What am I missing here?

[BillyONeal]

I still think you need to add -DCMAKE_DISABLE_FIND_PACKAGE_LibMount=ON for the portfile because otherwise binary caching is screwed. (Otherwise update cmake to 3.22 and add CMAKE_REQUIRE_FIND_PACKAGE_<PackageName>)

@Neumann-A, @BillyONeal Can you please elaborate why is this needed at all? And wouldn't -DCMAKE_DISABLE_FIND_PACKAGE_LibMount=ON disable finding LibMount entirelyl, in which case the KF5SolidConfig.cmake.in patch is pointless? What am I missing here?

Optional dependencies are bad, because key cause order dependencies. That is:

apt install libmount
vcpkg install kf5solid

and

vcpkg install kf5solid
apt install libmount

should produce the same results, because vcpkg is going to binary cache them as if they are the same and users expect them to be the same.

[wrobelda]

should produce the same results, because vcpkg is going to binary cache them as if they are the same and users expect them to be the same.

OK, I get that, but:

DCMAKE_DISABLE_FIND_PACKAGE_LibMount=ON disable finding LibMount entirely, in which case the KF5SolidConfig.cmake.in patch is pointless? What am I missing here?

What's the point of even adding that patch, if kf5solid now will never link against libmount? My point is that a feature should be added to this port to optionally allow that linkage, instead of permanently disabling it.

[dg0yt]

My point is that a feature should be added to this port to optionally allow that linkage, instead of permanently disabling it.

The feature must make the relevant find_package REQUIRED. And/or you add a libmount port as dependency.

[dg0yt]

My point is that a feature should be added to this port to optionally allow that linkage, instead of permanently disabling it.

The feature must make the relevant find_package REQUIRED. And/or you add a libmount port as dependency.

More accurately:

• With the feature enabled, find_package(LibMount REQUIRED) must succeed.
• With the feature disabled, LibMount_FOUND or other side effects of find_package(LibMount) must not have any effect on the build.
  -DCMAKE_DISABLE_FIND_PACKAGE_LibMount=ON is an approach to achieve this, but it also affects dependencies.

[JackBoosY]

My point is that a feature should be added to this port to optionally allow that linkage, instead of permanently disabling it.

The feature must make the relevant find_package REQUIRED. And/or you add a libmount port as dependency.

More accurately:

• With the feature enabled, find_package(LibMount REQUIRED) must succeed.
• With the feature disabled, LibMount_FOUND or other side effects of find_package(LibMount) must not have any effect on the build.
  -DCMAKE_DISABLE_FIND_PACKAGE_LibMount=ON is an approach to achieve this, but it also affects dependencies.

Agreed.

[wrobelda]

More accurately:

• With the feature enabled, find_package(LibMount REQUIRED) must succeed.

OK, I get that, but that PR effectively disabled libmount dependency, which will catch some developers using it off guard. And patching KF5SolidConfig.cmake.in was redundant, too – and potentially even more confusing. So while I understand this was breaking binary caching on CI/CD infra, this PR effectively will break things for devs.

With the feature enabled, find_package(LibMount REQUIRED) must succeed.

There's plenty of other kf5 ports that have such non-REQUIRED find_package dependencies. What's the ultimate solution here?

EDIT: in a way, what I am saying is that without something like -DCMAKE_FIND_PACKAGE_LibMount_MAKE_REQUIRE in CMake, in order to inline all my kf5 ports with what you did here in this PR, I'd have to disable each and every optional find_package(), otherwise they have a potential of breaking CI/CD binary caching, too.

[JackBoosY]

I think that all ports that depend on libmount should add the keyword REQUIRED in find_package(libmount) to prevent inconsistencies with user expectations (users rarely check logs). And in *config.cmake.in, add the find_dependency of the dependency according to the option.

In order to avoid conflicts with the Find method provided by cmake, we should explicitly declare to disable it in portfile.cmake.

[wrobelda]

I think that all ports that depend on libmount should add the keyword REQUIRED in find_package(libmount) to prevent inconsistencies with user expectations (users rarely check logs).

Yeah, but my point is that we are being selective about libmount only. This fixes this specific binary-caching CI/CD issue for you, the vcpkg team, but there are tons of other OPTIONAL find_dependency usages in kf5 and other ports (for the dependencies that are not provided by vcpkg itself) , which will break binary caching for other users, i.e. other teams of devs and/or their CI/CDs.

That having said, we can't possibly be patching all usages of OPTIONAL find_package(xxx) into REQUIRED, only to be able to add them as features with DCMAKE_DISABLE_FIND_PACKAGE_xxx switch. Which is why something like DCMAKE_FIND_PACKAGE_xxx_MAKE_REQUIRE would be useful.

[JackBoosY]

which will break binary caching for other users, i.e. other teams of devs and/or their CI/CDs.

In my opinion, this is just planting an untimely bomb, and explicit declarations are always safer than implicit declarations.
If downstream want to use this feature, they should ensure that libmount exists and turn on the corresponding option.

Edit: This is my personal opinion, not related to other members.

[dg0yt]

Well, the point of packaging is reproducibility. This is in conflict with developers trying to configure the build by auto-discovery. CMAKE_DISABLE_FIND_PACKAGE_xxx covers disabling features quite well, but we also need to cover reliably enabling features. CMAKE_REQUIRE_FIND_PACKAGE_<PackageName> is coming in CMake 3.22. (But freebsd is still at 3.20 in vcpkg.)

[wrobelda]

Well, the point of packaging is reproducibility. This is in conflict with developers trying to configure the build by auto-discovery. CMAKE_DISABLE_FIND_PACKAGE_xxx covers disabling features quite well, but we also need to cover reliably enabling features. CMAKE_REQUIRE_FIND_PACKAGE_<PackageName> is coming in CMake 3.22. (But freebsd is still at 3.20 in vcpkg.)

I agree. Sounds like something to be brought up with upstream. Also, I missed that CMAKE_REQUIRE_FIND_PACKAGE actually does what I suggested, so going forward we should be able to mitigate those issues.

@JackBoosY, I just posted a LibMount PR that will fix libmount usage across the board: #21491

[Neumann-A]

the problem is even bigger than that. A lot of ports require X libraries on windows and a lot of the x features are behind additional dependencies and optional find_package. So linux binary caching currently is screwed any way.

[cenit]

wasn't there also a PR by @ras0219-msft with a wrap around find_package to make all calls required?

[cenit]

that highlighted how impossible was for now to really enforce this rule