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

Preliminary CMake configure support on Windows (#228) #666

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

leha-bot
Copy link
Contributor

@leha-bot leha-bot commented Aug 3, 2024

The first step on proper compilation on Windows: the CMake configuring.

Some notes and restrictions of this PR:

  1. I used the vcpkg as a source of 3rd-party libraries. (I also fixed Windows build for the libev by this vcpkg PR specially for getting userver to configure by CMake). You have to install all dependencies which has been requested by userver's configure script (via "vcpkg search <depname>" and "vcpkg install <depname>");
  2. While I fixed the CMake configuring, the master has been moved up, and my fixes for Python tests were irrelevant. I will send it later (as part of Make all the tests pass on native Windows platform #229);
  3. As tests were not fixed, for proper configuring you have to disable them via -DUSERVER_FEATURE_TESTSUITE=OFF;
  4. Some non-MSVC flags were moved under the "userver_target_cxx_compile_options_if_supported()";
  5. uboost coro should be disabled if it was in your CMake cache via -DUSERVER_FEATURE_UBOOST_CORO=OFF.

Related to #228.

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/

Copy link

github-actions bot commented Aug 3, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@leha-bot
Copy link
Contributor Author

leha-bot commented Aug 3, 2024

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/

@leha-bot leha-bot changed the title Feat/cmake configure on windows Preliminary CMake configure support on Windows (#228) Aug 3, 2024
@@ -32,12 +32,18 @@ list(REMOVE_ITEM SOURCES ${UNIT_TEST_SOURCES} ${BENCH_SOURCES} ${INTERNAL_SOURCE
set(CMAKE_THREAD_PREFER_PTHREAD ON)
set(THREADS_PREFER_PTHREAD_FLAG ON)
find_package(Threads REQUIRED)
if (WIN32)
set(userver_boost_stacktrace_component stacktrace_windbg)
Copy link
Member

Choose a reason for hiding this comment

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

Just find stacktrace_windbg in OPTIONAL_COMPONENTS

target_compile_options(userver-internal-compile-options INTERFACE
"-Wall" "-Wextra" "-Wpedantic"
)
if (MSVC)
Copy link
Member

Choose a reason for hiding this comment

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

Please no MSVC for now, Clang on Windows is enough :)
We are not ready for MSVC's bugs and lack of certain intrinsics

@@ -206,7 +206,7 @@ _userver_directory_install(COMPONENT core
)

target_include_directories(${PROJECT_NAME} SYSTEM BEFORE PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/libc_include_fixes>
$<$<NOT:$<CXX_COMPILER_ID:MSVC>>:$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/libc_include_fixes>>
Copy link
Member

Choose a reason for hiding this comment

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

Probably NOT WIN32?

@@ -120,7 +120,7 @@ add_subdirectory(${USERVER_THIRD_PARTY_DIRS}/http-parser http-parser)
target_link_libraries(${PROJECT_NAME} PRIVATE userver-http-parser userver-llhttp)

set(USERVER_UBOOST_CORO_DEFAULT ON)
if(CMAKE_SYSTEM_NAME MATCHES "Darwin" AND CMAKE_SYSTEM_PROCESSOR MATCHES "arm64")
if(CMAKE_SYSTEM_NAME MATCHES "Darwin" AND CMAKE_SYSTEM_PROCESSOR MATCHES "arm64" OR CMAKE_SYSTEM_NAME MATCHES "Windows")
Copy link
Member

Choose a reason for hiding this comment

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

Use either CMAKE_SYSTEM_NAME MATCHES "Windows" or WIN32 consistently

find_program(venv_python
NAMES python python3
PATHS "${venv_dir}/bin" "${venv_dir}/Scripts"
REQUIRED NO_DEFAULT_PATH
Copy link
Member

Choose a reason for hiding this comment

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

We currently support cmake 3.14+, but REQUIRED appeared in cmake 3.18

@@ -95,7 +95,12 @@ function(userver_venv_setup)

set(venv_dir "${parent_directory}/${venv_name}")
set(venv_bin_dir "${venv_dir}/bin")
set("${python_output_var}" "${venv_bin_dir}/python" PARENT_SCOPE)
find_program(venv_python
NAMES python python3
Copy link
Member

Choose a reason for hiding this comment

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

We'd use NO_CACHE, but it appeared in 3.21

It's easier to check WIN32 and either use bin or Scripts based on that

Always using either python or python3 is OK as long as both are present on Windows, choose something that works

@leha-bot leha-bot closed this Aug 6, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Aug 6, 2024
@apolukhin apolukhin reopened this Sep 5, 2024
@userver-framework userver-framework unlocked this conversation Sep 5, 2024
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