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

Nemirtingas · 2022-04-12T15:00:17Z

Describe the pull request
This PR makes the '/' to be kept on Linux when cross-compiling using a MSVC compatible compiler (like Clang-CL) instead of behind replaced by '-'.

What does your PR fix?

Fixes [vcpkg] Compile flags are wrongly modified. #24095
Which triplets are supported/not supported? Have you updated the CI baseline?

Same as before, No.

Does your PR follow the maintainer guide?

Yes.

If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Yes.

github-actions

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

Error: Local changes detected for vcpkg-cmake but no changes to version or port version.
-- Version: 2022-04-12
-- Old SHA: 5aee0d861b593c164bb3362288ae9e926b62d8a8
-- New SHA: 7865bb6a8f7c708eaa93ff34e2c5ab0c97c479e3
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***

dg0yt · 2022-04-13T04:21:59Z

ports/vcpkg-cmake/cmake_get_vars/CMakeLists.txt

@@ -143,7 +143,7 @@ foreach(incdir IN LISTS CMAKE_C_STANDARD_INCLUDE_DIRECTORIES)
 endforeach()

 foreach(flag CXX C SHARED_LINKER EXE_LINKER STATIC_LINKER RC)
-    if(MSVC)
+    if(MSVC AND "${CMAKE_HOST_SYSTEM_NAME}" MATCHES "Windows")


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.)

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.

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.

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

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.

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)

dg0yt · 2022-04-13T04:26:08Z

ports/vcpkg-cmake/cmake_get_vars/CMakeLists.txt

@@ -143,7 +143,7 @@ foreach(incdir IN LISTS CMAKE_C_STANDARD_INCLUDE_DIRECTORIES)
 endforeach()

 foreach(flag CXX C SHARED_LINKER EXE_LINKER STATIC_LINKER RC)
-    if(MSVC)
+    if(MSVC AND "${CMAKE_HOST_SYSTEM_NAME}" MATCHES "Windows")
        # 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
        string(REGEX REPLACE "(^| )/" "\\1-" ${flag}_FLAGS "${${flag}_FLAGS}")


@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.

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

Means libtool and other stuff run via configure.

Means MSYS2 runtime, not bash.

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).

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.

Nemirtingas · 2022-04-15T07:42:51Z

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)

I'm happy with the change too. I added the comment and updated the version hash.
This change + adding set(VCPKG_POLICY_SKIP_ARCHITECTURE_CHECK enabled) to my custom triplet fixes my compilation issue.

strega-nil-ms · 2022-04-15T22:48:47Z

/azp run

azure-pipelines · 2022-04-15T22:48:56Z

Azure Pipelines successfully started running 1 pipeline(s).

LilyWangLL · 2022-04-18T03:02:43Z

Depends on #24169

Nemirtingas · 2022-04-18T08:31:20Z

Depends on #24169

Why does this PR depends on #24169 ?

LilyWangLL · 2022-04-18T09:02:11Z

Depends on #24169

Why does this PR depends on #24169 ?

This PR mark zeroc-ice doesn't support staticrt on windows, it should fix the pipeline failed.

Nemirtingas · 2022-04-18T09:11:20Z

Depends on #24169

Why does this PR depends on #24169 ?

This PR mark zeroc-ice doesn't support staticrt on windows, it should fix the pipeline failed.

Oh, ok, that was about the pipeline :).

BillyONeal

This is OK to me assuming CMAKE_HOST_SYSTEM_NAME contains Windows under all our "desktop" and "uwp" triplets and does not under MinGW.

ports/vcpkg-cmake/cmake_get_vars/CMakeLists.txt

LilyWangLL · 2022-05-05T07:26:28Z

Ping @Nemirtingas for response, could you resolve the conflicts?

Co-authored-by: Billy O'Neal <bion@microsoft.com>

…y_dash

Nemirtingas · 2022-05-05T08:02:31Z

Ping @Nemirtingas for response, could you resolve the conflicts?

Done.

# Conflicts: # versions/v-/vcpkg-cmake.json

BillyONeal

Slash dot dash dot slash dot dash dot slash dot com

BillyONeal · 2022-05-07T00:18:05Z

I merged this despite not having a full build for it since there were no 'interesting' merge conflicts outside of the version database.

Thanks for the fix!

github-actions bot requested changes Apr 12, 2022

View reviewed changes

Nemirtingas added 2 commits April 12, 2022 17:04

Fix '/' incorrectly replaced by '-' on Linux.

70ed91f

Update baseline.

99be9d7

Nemirtingas force-pushed the fix_slash_replaced_by_dash branch from f60bcb1 to 99be9d7 Compare April 12, 2022 15:04

github-actions bot approved these changes Apr 12, 2022

View reviewed changes

Nemirtingas added 2 commits April 12, 2022 20:08

Use MATCHES instead of STREQUAL.

1b5a3cc

Update version hash.

5554cf5

github-actions bot approved these changes Apr 12, 2022

View reviewed changes

JackBoosY assigned LilyWangLL Apr 13, 2022

dg0yt reviewed Apr 13, 2022

View reviewed changes

LilyWangLL added category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Apr 13, 2022

strega-nil-ms added requires:author-response and removed requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Apr 14, 2022

LilyWangLL changed the title ~~Fix '/' incorrectly replaced by '-' on Linux.~~ [vcpkg] Fix '/' incorrectly replaced by '-' on Linux. Apr 15, 2022

Nemirtingas added 2 commits April 15, 2022 09:40

Add comment for future modifications.

0a11a6f

Update version hash.

7fb9d88

github-actions bot approved these changes Apr 15, 2022

View reviewed changes

LilyWangLL added the depends:different-pr This PR or Issue depends on a PR which has been filed label Apr 18, 2022

LilyWangLL removed the depends:different-pr This PR or Issue depends on a PR which has been filed label Apr 20, 2022

Nemirtingas added 3 commits April 22, 2022 09:52

Merge branch 'master' into fix_slash_replaced_by_dash

7f5fdfd

Update version-date.

dc183ed

Update baseline.

fa5ab3b

github-actions bot approved these changes Apr 22, 2022

View reviewed changes

LilyWangLL removed the requires:author-response label Apr 24, 2022

LilyWangLL requested review from strega-nil-ms and BillyONeal April 24, 2022 06:15

BillyONeal approved these changes Apr 26, 2022

View reviewed changes

ports/vcpkg-cmake/cmake_get_vars/CMakeLists.txt Outdated Show resolved Hide resolved

LilyWangLL added the requires:author-response label May 5, 2022

Nemirtingas and others added 5 commits May 5, 2022 09:50

Update ports/vcpkg-cmake/cmake_get_vars/CMakeLists.txt

f734307

Co-authored-by: Billy O'Neal <bion@microsoft.com>

Merge remote-tracking branch 'vcpkg/master' into fix_slash_replaced_b…

9a7d735

…y_dash

Update version.

eccf03b

Run vcpkg format-manifest ports/vcpkg-cmake/vcpkg.json

ad50003

Run vcpkg x-add-version vcpkg-cmake

e3a5da3

github-actions bot approved these changes May 5, 2022

View reviewed changes

LilyWangLL removed the requires:author-response label May 5, 2022

Merge remote-tracking branch 'origin/master' into HEAD

50dbba7

github-actions bot approved these changes May 5, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into HEAD

ef84d24

# Conflicts: # versions/v-/vcpkg-cmake.json

BillyONeal approved these changes May 7, 2022

View reviewed changes

github-actions bot approved these changes May 7, 2022

View reviewed changes

BillyONeal merged commit d5ea966 into microsoft:master May 7, 2022

Nemirtingas deleted the fix_slash_replaced_by_dash branch May 24, 2022 15:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Nemirtingas commented Apr 12, 2022

github-actions bot left a comment

dg0yt Apr 13, 2022

Nemirtingas Apr 13, 2022

dg0yt Apr 13, 2022

Nemirtingas Apr 13, 2022 •

edited

Loading

Nemirtingas Apr 13, 2022

strega-nil-ms Apr 14, 2022 •

edited

Loading

dg0yt Apr 13, 2022

Neumann-A Apr 13, 2022

dg0yt Apr 13, 2022

dg0yt Apr 13, 2022

Neumann-A Apr 13, 2022

Nemirtingas commented Apr 15, 2022 •

edited

Loading

strega-nil-ms commented Apr 15, 2022

azure-pipelines bot commented Apr 15, 2022

LilyWangLL commented Apr 18, 2022

Nemirtingas commented Apr 18, 2022

LilyWangLL commented Apr 18, 2022

Nemirtingas commented Apr 18, 2022

BillyONeal left a comment

LilyWangLL commented May 5, 2022

Nemirtingas commented May 5, 2022

BillyONeal left a comment

BillyONeal commented May 7, 2022

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

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

Conversation

Nemirtingas commented Apr 12, 2022

What does your PR fix?

Which triplets are supported/not supported? Have you updated the CI baseline?

Does your PR follow the maintainer guide?

If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

github-actions bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nemirtingas Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nemirtingas commented Apr 15, 2022 • edited Loading

strega-nil-ms commented Apr 15, 2022

azure-pipelines bot commented Apr 15, 2022

LilyWangLL commented Apr 18, 2022

Nemirtingas commented Apr 18, 2022

LilyWangLL commented Apr 18, 2022

Nemirtingas commented Apr 18, 2022

BillyONeal left a comment

Choose a reason for hiding this comment

LilyWangLL commented May 5, 2022

Nemirtingas commented May 5, 2022

BillyONeal left a comment

Choose a reason for hiding this comment

BillyONeal commented May 7, 2022

If you have added/updated a port: Have you run `./vcpkg x-add-version --all` and committed the result?

Nemirtingas Apr 13, 2022 •

edited

Loading

strega-nil-ms Apr 14, 2022 •

edited

Loading

Nemirtingas commented Apr 15, 2022 •

edited

Loading