-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
[ryu] Fix toolchain selection on *nixes. Fix macOS build. Add support for Mingw-w64 on Windows #31037
Conversation
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
e91af15
to
d39033e
Compare
974344a
to
e2ea376
Compare
ports/ryu/portfile.cmake
Outdated
endif () | ||
else () | ||
message(FATAL_ERROR "${TARGET_TRIPLET} is not supported!") | ||
endif () | ||
if (VCPKG_DETECTED_CMAKE_SYSTEM_PROCESSOR STREQUAL x86) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what it's worth this if/elseif ladder looks wrong to me because it's inspecting CMAKE_SYSTEM_PROCESSOR
which is information about the host, not the target, but this PR isn't meaningfully changing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that vcpkg uses triplets instead of CPUs in its command line, so I chose to display the triplet to avoid confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On my x64 host, I checked the cmake-get-vars-x86-mingw-dynamic-rel.cmake.log file and found the following lines:
set(VCPKG_DETECTED_CMAKE_SYSTEM_PROCESSOR "i686")
set(VCPKG_DETECTED_CMAKE_HOST_SYSTEM_PROCESSOR "x86_64")
Based on this, I believe that VCPKG_DETECTED_CMAKE_SYSTEM_PROCESSOR refers to the target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't just push the quote adds because I wasn't 100% positive on them. Can you please either apply those or just let me know they're OK and I'll push that change for ya?
ports/ryu/portfile.cmake
Outdated
get_filename_component(DIR ${VCPKG_DETECTED_CMAKE_C_COMPILER} DIRECTORY) | ||
# Bazel expects Mingw-w64 to be installed in MSYS2 (pacman -S mingw-w64-x86_64-toolchain). | ||
# From BAZEL_SH it finds MSYS2 root, adds "mingw64" to the root and uses this path as the location of Mingw-w64. | ||
# It is also possible to use non-MSYS2 binaries with Bazel if they are installed to a directory | ||
# whose name ends with "mingw64", such as c:\mingw64 or c:\TDM-GCC-64\mingw64. | ||
string(REGEX REPLACE "/mingw64/bin$" "" MSYS2_ROOT ${DIR}) | ||
string(REPLACE / \\ MSYS2_ROOT ${MSYS2_ROOT}) | ||
set(ENV{BAZEL_SH} ${MSYS2_ROOT}\\usr\\bin\\bash.exe) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_filename_component(DIR ${VCPKG_DETECTED_CMAKE_C_COMPILER} DIRECTORY) | |
# Bazel expects Mingw-w64 to be installed in MSYS2 (pacman -S mingw-w64-x86_64-toolchain). | |
# From BAZEL_SH it finds MSYS2 root, adds "mingw64" to the root and uses this path as the location of Mingw-w64. | |
# It is also possible to use non-MSYS2 binaries with Bazel if they are installed to a directory | |
# whose name ends with "mingw64", such as c:\mingw64 or c:\TDM-GCC-64\mingw64. | |
string(REGEX REPLACE "/mingw64/bin$" "" MSYS2_ROOT ${DIR}) | |
string(REPLACE / \\ MSYS2_ROOT ${MSYS2_ROOT}) | |
set(ENV{BAZEL_SH} ${MSYS2_ROOT}\\usr\\bin\\bash.exe) | |
get_filename_component(DIR "${VCPKG_DETECTED_CMAKE_C_COMPILER}" DIRECTORY) | |
# Bazel expects Mingw-w64 to be installed in MSYS2 (pacman -S mingw-w64-x86_64-toolchain). | |
# From BAZEL_SH it finds MSYS2 root, adds "mingw64" to the root and uses this path as the location of Mingw-w64. | |
# It is also possible to use non-MSYS2 binaries with Bazel if they are installed to a directory | |
# whose name ends with "mingw64", such as c:\mingw64 or c:\TDM-GCC-64\mingw64. | |
string(REGEX REPLACE "/mingw64/bin$" "" MSYS2_ROOT "${DIR}") | |
string(REPLACE / \\ MSYS2_ROOT "${MSYS2_ROOT}") | |
set(ENV{BAZEL_SH} "${MSYS2_ROOT}\\usr\\bin\\bash.exe") |
ports/ryu/portfile.cmake
Outdated
vcpkg_execute_build_process( | ||
COMMAND ${BAZEL} --batch build ${BAZEL_CPU} ${CONLY_OPTS_RELEASE} ${LINK_OPTS_RELEASE} --verbose_failures --strategy=CppCompile=standalone //ryu //ryu:ryu_printf | ||
COMMAND ${BAZEL} --batch build ${BAZEL_COMPILER} ${BAZEL_CPU} ${CONLY_OPTS_RELEASE} ${LINK_OPTS_RELEASE} --verbose_failures --strategy=CppCompile=standalone //ryu //ryu:ryu_printf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
COMMAND ${BAZEL} --batch build ${BAZEL_COMPILER} ${BAZEL_CPU} ${CONLY_OPTS_RELEASE} ${LINK_OPTS_RELEASE} --verbose_failures --strategy=CppCompile=standalone //ryu //ryu:ryu_printf | |
COMMAND ${BAZEL} --batch build "${BAZEL_COMPILER}" ${BAZEL_CPU} ${CONLY_OPTS_RELEASE} ${LINK_OPTS_RELEASE} --verbose_failures --strategy=CppCompile=standalone //ryu //ryu:ryu_printf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto I was wrong sorry
ports/ryu/portfile.cmake
Outdated
@@ -65,12 +91,12 @@ else () | |||
endif () | |||
|
|||
vcpkg_execute_build_process( | |||
COMMAND ${BAZEL} --batch build ${BAZEL_CPU} ${CONLY_OPTS_DEBUG} ${LINK_OPTS_DEBUG} --verbose_failures --strategy=CppCompile=standalone //ryu //ryu:ryu_printf | |||
COMMAND ${BAZEL} --batch build ${BAZEL_COMPILER} ${BAZEL_CPU} ${CONLY_OPTS_DEBUG} ${LINK_OPTS_DEBUG} --verbose_failures --strategy=CppCompile=standalone //ryu //ryu:ryu_printf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
COMMAND ${BAZEL} --batch build ${BAZEL_COMPILER} ${BAZEL_CPU} ${CONLY_OPTS_DEBUG} ${LINK_OPTS_DEBUG} --verbose_failures --strategy=CppCompile=standalone //ryu //ryu:ryu_printf | |
COMMAND ${BAZEL} --batch build "${BAZEL_COMPILER}" ${BAZEL_CPU} ${CONLY_OPTS_DEBUG} ${LINK_OPTS_DEBUG} --verbose_failures --strategy=CppCompile=standalone //ryu //ryu:ryu_printf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, my mistake. I thought this was the file path to a compiler, and didn't realize that it could sometimes be empty string. This suggestion was wrong.
ports/ryu/portfile.cmake
Outdated
WORKING_DIRECTORY ${SOURCE_PATH} | ||
LOGNAME build-${TARGET_TRIPLET}-rel | ||
) | ||
|
||
if (CMAKE_HOST_WIN32) | ||
if (CMAKE_STATIC_LIBRARY_SUFFIX STREQUAL .lib) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm torn on whether this should be:
if (CMAKE_STATIC_LIBRARY_SUFFIX STREQUAL .lib) | |
if ("${CMAKE_STATIC_LIBRARY_SUFFIX}" STREQUAL ".lib") |
I think what you originally wrote is OK though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that all the double quotes in the code are unnecessary, so I removed them (where it possible). In my opinion, the lines with or without double quotes have the same meaning. However, if you prefer to keep the quotes for readability, it is acceptable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that all the double quotes in the code are unnecessary, so I removed them (where it possible). In my opinion, the lines with or without double quotes have the same meaning. However, if you prefer to keep the quotes for readability, it is acceptable.
In all cases where the variable is referred to by ${}
, the double quotes do have meaning. Without them, spaces in the original variable are replaced with ;
s (they are interpreted as different parameters to the CMake function in question).
The only case that is borderline is one like this where the variable is referred to by name rather than with ${}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from space:
All commands are equal, especially if
commands. Here, it is not necessary to turn variable names (CMAKE_STATIC_LIBRARY_SUFFIX
is okay) into quoted strings, but you should turn all expressions and literals into quoted strings.
- When some
${foo}
evaluates to an empty string,if(foo STREQUAL ${foo})
becomes invalidif(foo STREQUAL)
. - We had cases where unexpectedly a variable named
1
was defined to mean0
(or the other way?), giving unexpected results onif( ... EQUAL ...)
. So any literal which can be a variable name must be quoted ("1"
,"0"
).
So I would really apply quotes for .lib
here, unambiguously marking it as a plain literal expression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your advice! I don't fully agree with it, but as I mentioned earlier, using or not using double quotes doesn't make a difference to me. Therefore, I've added double quotes where it is possible. In my personal opinion, not using quotes was better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You didn't need to add quotes on things that don't name variables. Do you want me to just fix that up for you?
ports/ryu/vcpkg.json
Outdated
"description": "Ryu generates the shortest decimal representation of a floating point number that maintains round-trip safety.", | ||
"homepage": "https://github.com/ulfjack/ryu", | ||
"supports": "linux | osx | windows", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this supports does nothing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am facing an issue here where I have been able to successfully compile x86-mingw-static and x86-mingw-dynamic triplets on Linux, but not on Windows.
The following expression:
linux | osx | (linux & mingw) | (mingw & x64) | (windows & !mingw)
is wrong as it does not allow the compilation of x86 mingw targets on Linux.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(linux & mingw)
is never true.
Users can still build unsupported targets with --allow-unsupported
.
But maybe it is easier to omit the supports
expression where it cannot accurately catch the unsupported configurations.
(freebsd? android? ios? mingw & arm64?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We know for certain that the mingw x86 target is not supported on Windows.
!(mingw & x86 & windows)
is too restrictive, as it also prohibits mingw x86 targets on Linux.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, it cannot be expressed that it is supported for cross-builds only. Let supports
be less restrictive, and catch it in the portfile. Similar to what is needed elsewhere for "at least GCC 10" or "at least "C++20" etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the supports field. Non-x64 Windows mingw targets are already captured by the portfile.
192ae1c
to
b20c163
Compare
This looks good to me but after removals from ci.baseline.txt the build has to actually pass first before it can merge. Thanks again! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make it work also with release-only triplets.
ports/ryu/portfile.cmake
Outdated
string(REGEX REPLACE "[ ]+-" ";-" ${flags} ${${flags}}) | ||
foreach (OPT IN LISTS ${flags}) | ||
string(REGEX REPLACE "[ ]+((\\\")?)-" ";\\1-" ${flags} ${${flags}}) | ||
foreach (opt IN LISTS ${flags}) | ||
string(REGEX REPLACE "^([^ ]+)[ ]+\"?([^\"]+)\"?$" \\1\\2 opt ${opt}) | ||
string(REGEX REPLACE "^\"([^\"]+)\"$" \\1 opt ${opt}) | ||
if (${opts}) | ||
string(REGEX REPLACE "^([^ ]+)[ ]+\"?([^\"]+)\"?$" \\1\\2 OPT ${OPT}) | ||
set(${opts} ${${opts}};${switch}=${OPT}) | ||
set(${opts} ${${opts}};${switch}=${opt}) | ||
else () | ||
set(${opts} ${switch}=${OPT}) | ||
set(${opts} ${switch}=${opt}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know which transformation is really desired here, but in my experience parsing of flags from CMake should always start with separate_arguments
. Cf. https://github.com/microsoft/vcpkg/pull/30381/files.
ports/ryu/portfile.cmake
Outdated
string(REPLACE "/" "\\" MSYS2_ROOT "${MSYS2_ROOT}") | ||
set(ENV{BAZEL_SH} "${MSYS2_ROOT}\\usr\\bin\\bash.exe") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my tests, no conversion from CMake paths to Windows path is needed here.
@dg0yt Thanks! |
… for Mingw-w64 on Windows
@dg0yt Since you've reviewed this can you confirm you're happy with it? @sbrajchuk Thanks for your submission! |
ports/ryu/portfile.cmake
Outdated
if ("${VCPKG_BUILD_TYPE}" STREQUAL "" OR "${VCPKG_BUILD_TYPE}" STREQUAL "release") | ||
bazel_build("release") | ||
endif () | ||
|
||
if (CMAKE_HOST_WIN32) | ||
file(INSTALL ${SOURCE_PATH}/bazel-bin/ryu/ryu.lib DESTINATION ${CURRENT_PACKAGES_DIR}/debug/lib/) | ||
file(INSTALL ${SOURCE_PATH}/bazel-bin/ryu/ryu_printf.lib DESTINATION ${CURRENT_PACKAGES_DIR}/debug/lib/) | ||
else () | ||
file(INSTALL ${SOURCE_PATH}/bazel-bin/ryu/libryu.a DESTINATION ${CURRENT_PACKAGES_DIR}/debug/lib/) | ||
file(INSTALL ${SOURCE_PATH}/bazel-bin/ryu/libryu_printf.a DESTINATION ${CURRENT_PACKAGES_DIR}/debug/lib/) | ||
if ("${VCPKG_BUILD_TYPE}" STREQUAL "" OR "${VCPKG_BUILD_TYPE}" STREQUAL "debug") | ||
bazel_build("debug") | ||
endif () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the only supported single-config type is "release", so transformed to a frequent pattern, this would be:
bazel_build("release")
if(NOT VCPKG_BUILD_TYPE)
bazel_build("debug")
endif()
./vcpkg x-add-version --all
and committing the result.