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

[vcpkg] Add vcpkg_check_features #6958

Merged
merged 4 commits into from
Jun 28, 2019

Conversation

myd7349
Copy link
Contributor

@myd7349 myd7349 commented Jun 19, 2019

When working on #4979, I realized this pattern has been repeated too much:

if(<feature> IN_LIST FEATURES)
    set(<var> ON)
else()
   set(<var> OFF)
endif()

vcpkg_config_cmake(
    ...
    OPTIONS
        -D<var>=${var}
)

So it is time to create a little shiny macro/function for this.

czmq has a lot of optional libraries:

if("draft" IN_LIST FEATURES)
    set(ENABLE_DRAFTS ON)
else()
    set(ENABLE_DRAFTS OFF)
endif()

if("httpd" IN_LIST FEATURES)
    set(CZMQ_WITH_LIBMICROHTTPD ON)
else()
    set(CZMQ_WITH_LIBMICROHTTPD OFF)
endif()

if("tool" IN_LIST FEATURES)
    set(BUILD_TOOLS ON)
else()
    set(BUILD_TOOLS OFF)
endif()

if("curl" IN_LIST FEATURES)
    set(CZMQ_WITH_LIBCURL ON)
else()
    set(CZMQ_WITH_LIBCURL OFF)
endif()

if("lz4" IN_LIST FEATURES)
    set(CZMQ_WITH_LZ4 ON)
else()
    set(CZMQ_WITH_LZ4 OFF)
endif()

if("uuid" IN_LIST FEATURES AND
   VCPKG_CMAKE_SYSTEM_NAME STREQUAL "Linux")
    set(CZMQ_WITH_UUID ON)
else()
    set(CZMQ_WITH_UUID OFF)
endif()

vcpkg_configure_cmake(
    SOURCE_PATH ${SOURCE_PATH}
    PREFER_NINJA
    OPTIONS
        -DENABLE_DRAFTS=${ENABLE_DRAFTS}
        -DCZMQ_WITH_LIBMICROHTTPD=${CZMQ_WITH_LIBMICROHTTPD}
        -DCZMQ_WITH_UUID=${CZMQ_WITH_UUID}
        -DCZMQ_WITH_LIBCURL=${CZMQ_WITH_LIBCURL}
        -DCZMQ_WITH_LZ4=${CZMQ_WITH_LZ4}
)

This patch provides a macro(vcpkg_check_feature) and a function(vcpkg_check_features). With the help of this new module, it can be simplified as:

vcpkg_check_feature(draft ENABLE_DRAFTS)
vcpkg_check_feature(httpd CZMQ_WITH_LIBMICROHTTPD)
vcpkg_check_feature(tool BUILD_TOOLS)
vcpkg_check_feature(lz4 CZMQ_WITH_LZ4)
vcpkg_check_feature(curl CZMQ_WITH_LIBCURL)

if("uuid" IN_LIST FEATURES AND
   VCPKG_CMAKE_SYSTEM_NAME STREQUAL "Linux")
    set(CZMQ_WITH_UUID ON)
else()
    set(CZMQ_WITH_UUID OFF)
endif()

vcpkg_configure_cmake(
    SOURCE_PATH ${SOURCE_PATH}
    PREFER_NINJA
    OPTIONS
        -DENABLE_DRAFTS=${ENABLE_DRAFTS}
        -DCZMQ_WITH_LIBCURL=${CZMQ_WITH_LIBCURL}
        -DCZMQ_WITH_LIBMICROHTTPD=${CZMQ_WITH_LIBMICROHTTPD}
        -DCZMQ_WITH_LZ4=${CZMQ_WITH_LZ4}
        -DCZMQ_WITH_UUID=${CZMQ_WITH_UUID}
)

or

vcpkg_check_features(
    draft ENABLE_DRAFTS
    httpd CZMQ_WITH_LIBMICROHTTPD
    tool BUILD_TOOLS
    lz4 CZMQ_WITH_LZ4
    curl CZMQ_WITH_LIBCURL
)

if("uuid" IN_LIST FEATURES AND
   VCPKG_CMAKE_SYSTEM_NAME STREQUAL "Linux")
    set(CZMQ_WITH_UUID ON)
else()
    set(CZMQ_WITH_UUID OFF)
endif()

vcpkg_configure_cmake(
    SOURCE_PATH ${SOURCE_PATH}
    PREFER_NINJA
    OPTIONS
        -DCZMQ_WITH_UUID=${CZMQ_WITH_UUID}
        ${FEATURE_OPTIONS}
)

@myd7349
Copy link
Contributor Author

myd7349 commented Jun 20, 2019

I still have the following concerns:

  1. Is defining two new functions in one .cmake a good pattern?
  2. Is it easy to distinguish between vcpkg_check_feature and vcpkg_check_features?
  3. Is vcpkg_check_feature(s) a good name?
  4. Does the syntax of vcpkg_check_features look weired?

@Rastaban
Copy link
Contributor

I'm not sure having two separate macros is needed. I lean toward just vcpkg_check_features (unless others decide it looks weird) because it can still be called with one set of arguments.

@myd7349
Copy link
Contributor Author

myd7349 commented Jun 23, 2019

@Rastaban Thanks! That sounds resonable to me.

@myd7349
Copy link
Contributor Author

myd7349 commented Jun 24, 2019

TODO: Take this form into consideration:

if(<option> MATCHES "ON")
endif()

https://github.com/microsoft/mimalloc/blob/91222691cb21a6fb7a9760a264ff1183a1237fd4/CMakeLists.txt#L45
That is, we should allow the user to choose between ON/OFF, 1/0, TRUE/FALSE.
#7011

@cbezault
Copy link
Contributor

Agreed with @Rastaban, drop the singular version.

@myd7349
Copy link
Contributor Author

myd7349 commented Jun 27, 2019

@cbezault Thanks for your reply!

@myd7349 myd7349 changed the title [vcpkg] Add vcpkg_check_feature, vcpkg_check_features [vcpkg] Add vcpkg_check_features Jun 27, 2019
## )
## ```
##
## `vcpkg_check_feature` accepts a list of (feature, output_variable) pairs.
Copy link
Contributor

Choose a reason for hiding this comment

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

vcpkg_check_features

@cbezault
Copy link
Contributor

Could you modify one port to use this new feature and add it to this PR?

@myd7349
Copy link
Contributor Author

myd7349 commented Jun 28, 2019

Could you modify one port to use this new feature and add it to this PR?

Thanks! Done.

@cbezault cbezault merged commit b4675fd into microsoft:master Jun 28, 2019
@myd7349 myd7349 deleted the vcpkg-check-features branch June 28, 2019 22:20
@myd7349
Copy link
Contributor Author

myd7349 commented Jun 29, 2019

Oops! It seems that I made a mistake here:

if("non-posix" IN_LIST FEATURES)
    set(ENABLE_POSIX_API OFF) # Not `ON` here
else()
    set(ENABLE_POSIX_API ON)
endif()

can not be replaced with:

vcpkg_check_features(non-posix ENABLE_POSIX_API)

I will fix it.

@myd7349
Copy link
Contributor Author

myd7349 commented Jun 29, 2019

#7091

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants