Skip to content

[vcpkg] Fix '/' incorrectly replaced by '-' on Linux. #24098

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.

Sign up for GitHub

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

Merged
BillyONeal merged 16 commits into from
May 7, 2022
Merged

[vcpkg] Fix '/' incorrectly replaced by '-' on Linux. #24098

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
70ed91f
Fix '/' incorrectly replaced by '-' on Linux.
Apr 12, 2022
99be9d7
Update baseline.
Apr 12, 2022
1b5a3cc
Use MATCHES instead of STREQUAL.
Apr 12, 2022
5554cf5
Update version hash.
Apr 12, 2022
0a11a6f
Add comment for future modifications.
Apr 15, 2022
7fb9d88
Update version hash.
Apr 15, 2022
7f5fdfd
Merge branch 'master' into fix_slash_replaced_by_dash
Apr 22, 2022
dc183ed
Update version-date.
Apr 22, 2022
fa5ab3b
Update baseline.
Apr 22, 2022
f734307
Update ports/vcpkg-cmake/cmake_get_vars/CMakeLists.txt
Nemirtingas May 5, 2022
9a7d735
Merge remote-tracking branch 'vcpkg/master' into fix_slash_replaced_b…
May 5, 2022
eccf03b
Update version.
May 5, 2022
ad50003
Run vcpkg format-manifest ports/vcpkg-cmake/vcpkg.json
May 5, 2022
e3a5da3
Run vcpkg x-add-version vcpkg-cmake
May 5, 2022
50dbba7
Merge remote-tracking branch 'origin/master' into HEAD
BillyONeal May 5, 2022
ef84d24
Merge remote-tracking branch 'origin/master' into HEAD
BillyONeal May 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions ports/vcpkg-cmake/cmake_get_vars/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,12 @@ foreach(incdir IN LISTS CMAKE_C_STANDARD_INCLUDE_DIRECTORIES)
endforeach()

foreach(flag CXX C SHARED_LINKER EXE_LINKER STATIC_LINKER RC)
if(MSVC)
# Transform MSVC /flags to -flags due to bash scripts intepreting /flag as a path.
# This is imperfect because it fails on directories with trailing spaces, but those are exceedingly rare
# When using MSVC, maybe transform /flags to -flags.
# When cross compiling, "/flags" may be an absolute path starting with /, so don't transform.
# Otherwise, transform to improve compatibility with scripts interpreting "/flags" as a path.
if(MSVC AND "${CMAKE_HOST_SYSTEM_NAME}" MATCHES "Windows")
Copy link
Contributor

@dg0yt dg0yt Apr 13, 2022

Choose a reason for hiding this comment

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

That's an unusual setup. Without an extra comment, the AND part will look as if implied by MSVC and might get removed in future cleanup.
(IMO if something is claiming to be MSVC, it should really accept the same parameters as MSVC.)

(And maybe updates to this PR can wait for a low-load time on CI? We have several changes to vcpkg-cmake now, all building for hours, and affecting more common setups.)

Copy link
Contributor Author

@Nemirtingas Nemirtingas Apr 13, 2022

Choose a reason for hiding this comment

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

So, I should add a comment about cross-compilation so it doesn't get removed by someone else in the future ?
MSVC only implies a MSVC compatible compiler, not that its compiling on Windows.

Copy link
Contributor

@dg0yt dg0yt Apr 13, 2022

Choose a reason for hiding this comment

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

MSVC only implies a MSVC compatible compiler, not that its compiling on Windows.

So is your toolchain providing "a MSVC compatible compiler" if it is not fully simulating the Visual C++ cl command-line syntax?

I don't want to say that assuming Windows is right. I'm saying that people might easily accept the assumption that MSVC implies CMAKE_HOST_WIN32.

Copy link
Contributor Author

@Nemirtingas Nemirtingas Apr 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

My toolchain just setup LLVM + Clang + Clang-CL (the CL.exe syntax emulator) for Linux to build Windows binaries on Linux.
Here are my sources:
https://github.com/Nemirtingas/clang-msvc-sdk # The toolchain
https://github.com/Nemirtingas/windowscross_vcpkg/tree/msvc2019_win10.0.18362.0 # The Ubuntu docker image to use the toolchain + vcpkg.

As far as I tested, it understands the CL.exe flags/options and transcribes them to Clang valid flags/options (or ignoring them if not needed like /MP (multi-processor compile)). But it also adds some other options that are not provided by CL.exe.
I have this kind of extra options for clang-cl: (Remember clang-cl is a frontend, it's not a compiler like clang is).

-imsvc /clang_windows_sdk/msvc/include
-imsvc /clang_windows_sdk/winsdk/Include/10.0.18362.0/ucrt
-imsvc /clang_windows_sdk/winsdk/Include/10.0.18362.0/shared
-imsvc /clang_windows_sdk/winsdk/Include/10.0.18362.0/um
-imsvc /clang_windows_sdk/winsdk/Include/10.0.18362.0/winrt

Which pass the include directories to the clang compiler.

-Xclang -ivfsoverlay -Xclang /vcpkg/buildtrees/boost-exception/x86-windows-nemirtingas-rel/winsdk_vfs_overlay.yaml

Which pass a virtual FS overlay to clang for case-insensible include files/libraries.

Thoses options are rewritten by this rule and destroys all the Toolchain logic. (Note the first '/' is replaced by a '-')

-imsvc -clang_windows_sdk/msvc/include
-imsvc -clang_windows_sdk/winsdk/Include/10.0.18362.0/ucrt
-imsvc -clang_windows_sdk/winsdk/Include/10.0.18362.0/shared
-imsvc -clang_windows_sdk/winsdk/Include/10.0.18362.0/um
-imsvc -clang_windows_sdk/winsdk/Include/10.0.18362.0/winrt
-Xclang -ivfsoverlay
-Xclang -vcpkg/buildtrees/boost-exception/x86-windows-nemirtingas-rel/winsdk_vfs_overlay.yaml

Copy link
Contributor Author

@Nemirtingas Nemirtingas Apr 13, 2022

Choose a reason for hiding this comment

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

The condition I added might not be fixing the issue but only my specific issue. If you run LLVM + Clang + Clang-CL on Windows, the bug should be still there because "${CMAKE_HOST_SYSTEM_NAME}" MATCHES "Windows" is true in that case.

Copy link
Contributor

@strega-nil-ms strega-nil-ms Apr 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think the right thing to do here is to just add the comment; something like # for cross-compilation, we don't want to do this in case we have absolute paths starting with `/` (otherwise I'm happy with this change, personally)

# This implementation is imperfect because it fails on directories with trailing spaces,
# but those are rare.
string(REGEX REPLACE "(^| )/" "\\1-" ${flag}_FLAGS "${${flag}_FLAGS}")
Copy link
Contributor

@dg0yt dg0yt Apr 13, 2022

Choose a reason for hiding this comment

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

@ras0219-msft I would be curious about the actual cases which motivated this line. Bash shouldn't treat paths different from other arguments, exept that it may do expansion of * or ~. (In a bash world, you would resolve this with single quotes.)
But MSYS2 has special treatment for converting Unix paths to Windows paths in command line arguments and environment variables. This could probably be resolved by setting enviroment variables to declare the known flags.

Copy link
Contributor

@Neumann-A Neumann-A Apr 13, 2022

Choose a reason for hiding this comment

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

bash scripts

Means libtool and other stuff run via configure. Just try to remove it and see the vcpkg_configure_make world burn ;)

See #12936. Especially 65e71b1 (I aborted the CI run since I already saw it failing) vs 723b9d7

Copy link
Contributor

@dg0yt dg0yt Apr 13, 2022

Choose a reason for hiding this comment

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

Means libtool and other stuff run via configure.

Means MSYS2 runtime, not bash.

Copy link
Contributor

@dg0yt dg0yt Apr 13, 2022

Choose a reason for hiding this comment

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

Illustration

$ /usr/bin/echo.exe /libpath/users
/libpath/users
$ /C/Windows/System32/findstr.exe a /libpath/users
FINDSTR: D:/msys64/libpath/users kann nicht geöffnet werden.
$ MSYS2_ARG_CONV_EXCL=/libpath /C/Windows/System32/findstr.exe a /libpath/users
FINDSTR: /libpath/users kann nicht geöffnet werden.

I see that the MSYS2_ARG_CONV_EXCL doesn't scale to all combinations of tools and parameters, even if scoped via wrappers.
But sometimes it is necessary to understand that the transformation is not in bash itself (cf. bash -> echo) but a MSYS2 service at the border between MSYS2 runtime (bash) and windows runtime (findstr).

Copy link
Contributor

@Neumann-A Neumann-A Apr 13, 2022

Choose a reason for hiding this comment

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

Means MSYS2 runtime, not bash.

libtool is just a bash script. But I really don't care about the details anymore. vcpkg should just build the msys stuff all by itself and no longer have to deal with MSYS2 since it has often enough shown to be instable by removing downloadable stuff.

if(CMAKE_SYSTEM_NAME STREQUAL "WindowsStore")
if("${flag}" STREQUAL "CXX")
Expand Down
2 changes: 1 addition & 1 deletion ports/vcpkg-cmake/vcpkg.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"name": "vcpkg-cmake",
"version-date": "2022-05-05",
"version-date": "2022-05-06",
"license": "MIT"
}
2 changes: 1 addition & 1 deletion versions/baseline.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7317,7 +7317,7 @@
"port-version": 1
},
"vcpkg-cmake": {
"baseline": "2022-05-05",
"baseline": "2022-05-06",
"port-version": 0
},
"vcpkg-cmake-config": {
Expand Down
5 changes: 5 additions & 0 deletions versions/v-/vcpkg-cmake.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
{
"versions": [
{
"git-tree": "46a5686341cf9136053ff01b9f213dbd2dbea8a6",
"version-date": "2022-05-06",
"port-version": 0
},
{
"git-tree": "ecfb2282f599a9c1f3a9ad98eac9d2b35690850f",
"version-date": "2022-05-05",
Expand Down