Jens-G opened a new pull request, #3909:
URL: https://github.com/apache/thrift/pull/3909
`build/cmake/DefinePlatformSpecifc.cmake` means to hand `/MP` to MSVC so
that a target's sources compile in parallel. It never has.
```cmake
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()
```
The left-hand side is quoted but not dereferenced, so what gets compared is
the literal string `CMAKE_CXX_COMPILER_ID` against `MSVC`. The top-level
`CMakeLists.txt` requires CMake 3.16, which puts CMP0054 on NEW, and under NEW
a quoted `if()` argument is never expanded as a variable. The condition is
false for every compiler, and `/MP` has never reached a command line.
Reproduced with CMake 3.22:
```
set(CMAKE_CXX_COMPILER_ID "MSVC")
if("CMAKE_CXX_COMPILER_ID" STREQUAL "MSVC") -> false
if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC") -> true
CMP0054 = NEW
```
The quoting predates c5a5f79d7, which separated the MSVC and Clang-Cl
settings, so this is not a regression from that change. Keeping the
dereferenced `CMAKE_<LANG>_COMPILER_ID` rather than switching to `if(MSVC)`
preserves the intent of the comment above it: clang-cl reports `Clang`, so it
still does not get `/MP`.
### Deliberately out of scope
The two conditions below this one carry the same bug. Repairing them changes
behaviour well beyond build speed, so they are left alone here:
- the branch guarding `/W3` and the two `utf-8` charset flags 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 an error limit 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.
### Testing
No unit test applies to a compiler flag. The evidence is the build log:
`/MP` appears nowhere in the current AppVeyor output, and the MSVC job's two
largest targets — `thrift-compiler.exe` at 4 min 22 s and `thrift_compiler.lib`
at about 4 min — are where the difference should show.
### Related
**THRIFT-6324** cuts the AppVeyor matrix and adds `--parallel` to `cmake
--build`. Kept separate on purpose: `/m` works at project level and `/MP` at
source level, the two multiply, and on a small build worker they can
oversubscribe. Landing them apart gives a clean A/B on the job timings.
---
- [x] Did you create an [Apache
Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? —
THRIFT-6325
- [x] If a ticket exists: Does your pull request title follow the pattern
"THRIFT-NNNN: describe my issue"?
- [x] Did you squash your changes to a single commit?
- [x] Did you do your best to avoid breaking changes?
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
