Fix CMake checks for Linux #8210
Conversation
The LINUX variable is not a thing (at least not by default). Use an explicit CMAKE_SYSTEM_NAME check.
|
|
||
| # The version of clang currently on our Mac bots doesn't seem to support this flag. | ||
| if (CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND (LINUX OR WINDOWS)) | ||
| if (CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND NOT APPLE) |
There was a problem hiding this comment.
Do APPLE is a global but LINUX is not? I guess LINUX must have been at some point in the past?
There was a problem hiding this comment.
I'm not sure if it was in the past, or if some projects define it, or if I just hallucinated it.
| if(LINUX) | ||
| if(CMAKE_SYSTEM_NAME STREQUAL "Linux") | ||
| # Disable interposition and resolve Binaryen symbols locally. | ||
| add_link_flag("-Bsymbolic") |
There was a problem hiding this comment.
Do these two flags were just never being add on linux?
There was a problem hiding this comment.
This flag was never being added. It was sort of a drive-by addition to #7618.
The line above was modified in #8204 just this week, and specifically in a change I added right before submitting the PR; I didn't test that version locally, only on the bots. The error means that -fuse-lld would just never be added, and it would have broken the next LTO release build (because our use of thinlto requires lld).
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
Strange, if I look at the link flags used by binaryen on my linux system i see -Bsymbolic but not -fuse-ld:
[236/236] : && /usr/bin/c++ -fno-omit-frame-pointer -fno-rtti -fPIC -fdiagnostics-color=always -DBUILD_LLVM_DWARF -Wall -Werror -Wextra -Wno-unused-parameter -Wno-dangling-pointer -Wno-implicit-int-fl oat-conversion -Wno-unknown-warning-option -Wswitch -Wimplicit-fallthrough -Wnon-virtual-dtor -O2 -g -DNDEBUG -UNDEBUG -Bsymbolic -Wl,--dependency-file=test/gtest/CMakeFiles/binaryen-unittests.dir/ link.d test/gtest/CMakeFiles/binaryen-unittests.dir/arena.cpp.o test/gtest/CMakeFiles/binaryen-unittests.dir/cast-check.cpp.o test/gtest/CMakeFiles/binaryen-unittests.dir/cfg.cpp.o test/gtest/CMakeFil es/binaryen-unittests.dir/dfa_minimization.cpp.o test/gtest/CMakeFiles/binaryen-unittests.dir/disjoint_sets.cpp.o test/gtest/CMakeFiles/binaryen-unittests.dir/glbs.cpp.o test/gtest/CMakeFiles/binaryen -unittests.dir/interpreter.cpp.o test/gtest/CMakeFiles/binaryen-unittests.dir/intervals.cpp.o test/gtest/CMakeFiles/binaryen-unittests.dir/json.cpp.o test/gtest/CMakeFiles/binaryen-unittests.dir/latti ces.cpp.o test/gtest/CMakeFiles/binaryen-unittests.dir/local-graph.cpp.o test/gtest/CMakeFiles/binaryen-unittests.dir/possible-contents.cpp.o test/gtest/CMakeFiles/binaryen-unittests.dir/principal-typ e.cpp.o test/gtest/CMakeFiles/binaryen-unittests.dir/printing.cpp.o test/gtest/CMakeFiles/binaryen-unittests.dir/public-type-validator.cpp.o test/gtest/CMakeFiles/binaryen-unittests.dir/scc.cpp.o test /gtest/CMakeFiles/binaryen-unittests.dir/stringify.cpp.o test/gtest/CMakeFiles/binaryen-unittests.dir/subtype-exprs.cpp.o test/gtest/CMakeFiles/binaryen-unittests.dir/suffix_tree.cpp.o test/gtest/CMak eFiles/binaryen-unittests.dir/topological-sort.cpp.o test/gtest/CMakeFiles/binaryen-unittests.dir/type-builder.cpp.o test/gtest/CMakeFiles/binaryen-unittests.dir/wat-lexer.cpp.o test/gtest/CMakeFiles/ binaryen-unittests.dir/validator.cpp.o test/gtest/CMakeFiles/binaryen-unittests.dir/source-map.cpp.o -o bin/binaryen-unittests -Wl,-rpath,"\$ORIGIN/../lib" lib/libbinaryen.so lib/libgtest.a lib/li bgtest_main.a lib/libmimalloc.so.2.2 && :
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
Oh, that is because I'm not using clang I guess.
Why are we passing -fuse-ld=lld only when clang is the compiler? Doesn't lld also work if gcc is the compiler? Shouldn't be more like "if lld is available"?
Anyway the LINUX check does seem to work just fine for me.
|
Weird that LINUX is working for you; what version of CMake are you using? I had thought that gcc didn't support the Maybe we should do a check anyway? Or offer a CMake flag to opt out? I guess we can always see if someone complains. It should probably be a separate PR. |
|
Oh, and here's the answer to the LINUX variable question: https://cmake.org/cmake/help/v4.2/variable/LINUX.html It was added in CMake 3.25, and the version of CMake that our Chromium CI bots use is 3.21 IIRC. |
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
Can you mention in the PR description that LINUX requires version X but binaryen support cmake 3.16.3
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
For lld detection it looks like you can do
find_program(LLD_EXECUTABLE NAMES lld lld-link)
if(LLD_EXECUTABLE)
...
2 participants
The LINUX variable only exists in CMake starting in version 3.25
However we support CMake versions down to 3.16 currently.
Use an explicit CMAKE_SYSTEM_NAME check instead.