Use a target in FindDiscount (modern cmake) rather than variables
ClosedPublic

Authored by svuorela on May 2 2018, 8:11 PM.

Diff Detail

Repository
R223 Okular
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
svuorela created this revision.May 2 2018, 8:11 PM
Restricted Application added a project: Okular. · View Herald TranscriptMay 2 2018, 8:11 PM
Restricted Application added a subscriber: Okular. · View Herald Transcript
svuorela requested review of this revision.May 2 2018, 8:11 PM
aacid added a subscriber: aacid.May 5 2018, 9:55 AM

It's more lines of code than before, what's the improvement?

  1. The current code only works if discount is installed into existing locations (discount_INCLUDE_DIR is never used), and as a user of the FindPackage, you don't have to care about the include directory. It comes from the target.
  2. The whole thing with targets vs variables: if you link against a double colon target, it fails at cmake time if something is wrong. If something is wrong with the variable, it fails at link time. E.g. trying to link discard::Lib fails at cmake time, linking ${discard_LIBRARIES} gives a link error.

Using targets is much less error prone, and in general encouraged.

aacid added a comment.May 7 2018, 11:01 PM

If noone disagrees i guess you can commit this in one week or so, i don't really have enough knowledge to discuss if this is better or worse.

apol accepted this revision.May 8 2018, 1:28 AM
apol added a subscriber: apol.

It's actually fixing the build when discount is in its own prefix.

Also I agree that it's better using targets.

This revision is now accepted and ready to land.May 8 2018, 1:28 AM
This revision was automatically updated to reflect the committed changes.