Jens Geyer created THRIFT-6325:
----------------------------------
Summary: Let the MSVC builds use /MP again
Key: THRIFT-6325
URL: https://issues.apache.org/jira/browse/THRIFT-6325
Project: Thrift
Issue Type: Bug
Components: Build Process
Reporter: Jens Geyer
h3. Problem
{{build/cmake/DefinePlatformSpecifc.cmake}} means to hand {{/MP}} to MSVC so
that the sources of a target compile in parallel:
{code}
foreach(lang IN ITEMS C CXX)
if("CMAKE_${lang}_COMPILER_ID" STREQUAL "MSVC")
# These flags are not supported (or needed) with Clang-Cl on Windows:
set(CMAKE_${lang}_FLAGS "${CMAKE_${lang}_FLAGS} /MP") # parallel build
endif()
{code}
The left-hand side is quoted but never dereferenced, so the comparison is
between the literal string {{CMAKE_CXX_COMPILER_ID}} and {{MSVC}}. The
top-level {{CMakeLists.txt}} asks for {{cmake_minimum_required(VERSION 3.16)}},
which puts CMP0054 on NEW, and under NEW a quoted {{if()}} argument is never
expanded as a variable. The condition is therefore false for every compiler,
and {{/MP}} has never reached a command line.
Reproduced with CMake 3.22:
{noformat}
set(CMAKE_CXX_COMPILER_ID "MSVC")
if("CMAKE_CXX_COMPILER_ID" STREQUAL "MSVC") -> false
if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC") -> true
CMP0054 = NEW
{noformat}
The quoting predates the split of the MSVC and Clang-Cl settings; it is not a
regression from that change.
h3. Change
Dereference the variable in the {{/MP}} condition. One character pair, no
behavioural change other than MSVC compiling a target's sources in parallel
again.
h3. Deliberately out of scope
The two conditions below it carry the same bug, and repairing them changes
behaviour well beyond build speed:
* the {{/W3}} and {{/source-charset:utf-8}} and {{/execution-charset:utf-8}}
branch also calls {{add_definitions("-DUNICODE -D_UNICODE")}}, which switches
the TCHAR semantics of every Windows build;
* the {{elseif}} branches for Clang and GNU would newly apply {{-Wall -Wextra}}
and {{-fmax-errors=1}} / {{-ferror-limit=1}} across the whole Linux and macOS
CI, most likely surfacing a large number of warnings at once.
Each deserves its own ticket and its own review. This one is scoped to {{/MP}}.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
