[vcpkg_check_features] Set output variable explicitly and allow reverse-logic check

[vicroms]

The aim of these PR is to make the following changes to vcpkg_check_features():

• Require an output variable to be explicitly set
  The current implementation silently sets the variable FEATURE_OPTIONS, this breaks the pattern used in other functions like vcpkg_from_github() or vcpkg_extract_source_archive_ex(), where out variables are set explicitly. A new parameter OUT_FEATURE_OPTIONS is added to replace the FEATURE_OPTIONS variable.

• Do not create variables for each feature
  Stop creating output variables for each feature, this makes it so that OUT_FEATURE_OPTIONS is the only output variable produced by this function.

• Allow inverted-logic features.
  Allow features that have opt-out behavior in their build-systems, e.g.: cpprestsdk[websockets,brotli], to use vcpkg_check_features() with inverted logic.

In order to accommodate these changes the usage of the function has been changed to:

vcpkg_check_features(
  OUT_FEATURE_OPTIONS <output_variable> 
  [FEATURES]
    <feature1> <option_name1>
    [<feature2> <option_name2>
     ...]
  [INVERTED_FEATURES
    <feature3> <option_name3>
    [<feature4> <option_name4>]
     ...]
)

Ports already using vcpkg_check_features() need to be updated to follow this new usage:

• czmq
• darknet
• librdkafka
• mimalloc
• mongo-c-driver
• oniguruma
• opencv4
• paho-mqttpp3
• pcl
• xsimd
• xtensor
• zeromq

[Neumann-A]

Is there a simple reason why <uppercasefeature>_ENABLED and <uppercasefeature>_DISABLED are not simply passed by vcpkg to cmake? VCPKG probably has something in place to generate the FEATURES list and knows about all available features. Having to write it out in another function is just duplication of information.
see build.cpp line 411 and 412. E.g. FEATURES and ALL_FEATURES.
So in CMake it could simply defined as:

foreach(feature ${ALL_FEATURES})
 string(TOUPPER "${feature}" upfeat)
 if(${feature} IN LIST ${FEATURES})
   set(${upfeat}_ENABLED 1)
   set(${upfeat}_DISABLED 0)
 else()
   set(${upfeat}_ENABLED 0)
   set(${upfeat}_DISABLED 1)
 endif()
endforeach()

and should be put somewhere before the portfile is included.

vcpkg_check_features() thus looks superflous and there is no benefit of having it. I still have to do the mapping from feature to cmake option and it might even be more obvious using the syntax:
-DSOME_STRANGE_PORTFILE_SPECIFIC_CMAKE_OPTION=${FEATURE_DISABLED} instead of just passing ${port_OPTIONS}

[vicroms]

Is there a simple reason why <uppercasefeature>_ENABLED and <uppercasefeature>_DISABLED are not simply passed by vcpkg to cmake?

vcpkg_check_features() thus looks superflous and there is no benefit of having it. I still have to do the mapping from feature to cmake option and it might even be more obvious using the syntax:
-DSOME_STRANGE_PORTFILE_SPECIFIC_CMAKE_OPTION=${FEATURE_DISABLED} instead of just passing ${port_OPTIONS}

The main motivation behind this PR is to get rid of automatically-set variables.

We strive to design portfile functions with zero or minimal side-effects. Although, sometimes it is not possible (e.g.: vcpkg_configure_cmake() smuggles the generator to vcpkg_install_cmake()).

The same is true for vcpkg features, we try to limit the behind-the-scenes "magic" that is done by vcpkg, for that reason, adding feature_ENABLED and feature_DISABLED macros would contribute negatively to this goal.

Using the -DLIBRARY_SPECIFIC_OPTION=${FEATURE_DISABLED} pattern does not solve the problem of having to map each feature to a CMake option manually, but having the definition in vcpkg_check_features() match the library specific option from the start does.

vcpkg_check_feature(
  OUT_FEATURE_OPTIONS FEATURE_OPTIONS
    "feature" LIBRARY_SPECIFIC_OPTION
    "feature2" LIBRARY_SPECIFIC_OPTION2
)

[Neumann-A]

behind-the-scenes "magic"

that just referes to undocumented behavior. If the variables are documented there is no issue with that. The function just is documented and thus it seems like less behind-the-scenes "magic". The same could be said about all the VCPKG_POLICY_ variables. They are just missing documentation.

Netherless you probably want to add an optional APPEND option and an optional NEGATE option instead of an INVERTED_FEATURES list. APPEND to append to OUT_FEATURE_OPTIONS which allows chaining of multiple vcpkg_check_feature calls into one OUT_FEATURE_OPTIONS variable which is probably necessary if you do not want to have big duplicated features checks for different build targets.

[ras0219-msft]

We did consider a NEGATE option, but we felt that having the user declare the two categories was clearer and easier to read (assuming you don't have over a page of options). Because this is setting cmake -D options, order isn't relevant.

I really like the idea of APPEND, though we can add that switch later once we see a port that can benefit from it.

[cenit]

This is different from the previous meaning!!!

[vicroms]

You're right, it was a mistake on my part.
Unfortunately, this means that this port cannot use vcpkg_check_features() now.

[cenit]

Ok, no problem, the right dependencies are more important

[Neumann-A]

Unfortunately, this means that this port cannot use vcpkg_check_features() now.

Why? Shouldn't the feature list now contain cuda, opencv and opencv-cuda?

I assume the correct line should read darknet[opencv], darknet[cuda], opencv[cuda]

[cenit]

No.
Darknet[cuda] enables cuda for darknet, darknet[opencv-cuda] requires a cuda-enabled opencv. They are independent features. If you want both, you have to ask for darknet[cuda,opencv-cuda]

[Neumann-A]

Ah I see
Build-Depends: darknet[opencv], darknet[cuda], opencv[cuda]
resolves to
Build-Depends: opencv[ffmpeg], cuda, opencv[cuda]
but you want to have:
Build-Depends: darknet[opencv], opencv[cuda]
resolving to
Build-Depends: opencv[ffmpeg], opencv[cuda]

[cenit]

I’d just revert them the way they were originally. I also had another pr floating, for the cudnn feature. I think I will just restore features there merging with master, then wait for merging.