mstorsjo / msvc-wine

Scripts for setting up and running MSVC in Wine on Linux
Other
683 stars 83 forks source link

Support `import std` for CMake #126

Closed huangqinjin closed 7 months ago

huangqinjin commented 7 months ago

https://gitlab.kitware.com/cmake/cmake/-/merge_requests/9337 The coming cmake 3.30 supports import std for clang and MSVC.

For MSVC, it need to locate modules.json which is in ${VCToolsInstallDir}/modules (VCToolsInstallDir = VC/Tools/MSVC/$VER). https://gitlab.kitware.com/cmake/cmake/-/blob/ca449572ef8b1c6066e88879795900aea9727834/Modules/Compiler/MSVC-CXX-CXXImportStd.cmake#L2-11

  find_file(_msvc_modules_json_file
    NAME modules.json
    HINTS
      "$ENV{VCToolsInstallDir}/modules"
    PATHS
      "$ENV{INCLUDE}"
      "${CMAKE_CXX_COMPILER}/../../.."
    PATH_SUFFIXES
      ../modules
    NO_CACHE)

To support import std without those environment variables presets, the specific directory layout is required. There are two ways going on:

  1. Copy ${VCToolsInstallDir}/modules/* to installation root. There are only 3 files in the modules folder: modules.json, std.ixx and std.compat.ixx. So I think it is acceptable.
  2. Create symlink modules under installation root to ${VCToolsInstallDir}/modules. This results in a different layout compared to the original MSVC tools layout.

VCToolsInstallDir layout:

  ${VCToolsInstallDir}/bin/Hostx64/x64/cl.exe
  ${VCToolsInstallDir}/modules/modules.json

Proposed msvc-wine installation layout:

  ${root}/bin/x64/cl.exe
  ${root}/modules/modules.json

This requires to add "${CMAKE_CXX_COMPILER}/../.." to above cmake code block. I prefer this way. If we agree, I am going to send a MR to CMake. But if they reject, we have to go with 1.

The test is not enabled currently since the new feature is experimental and is gated by CMAKE_EXPERIMENTAL_CXX_IMPORT_STD which is a UUID string and may be updated time to time, the value is documented at https://gitlab.kitware.com/cmake/cmake/-/blob/ca449572ef8b1c6066e88879795900aea9727834/Help/dev/experimental.rst.

mstorsjo commented 7 months ago

Thanks for looking into this!

For MSVC, it need to locate modules.json which is in ${VCToolsInstallDir}/modules (VCToolsInstallDir = VC/Tools/MSVC/$VER). https://gitlab.kitware.com/cmake/cmake/-/blob/ca449572ef8b1c6066e88879795900aea9727834/Modules/Compiler/MSVC-CXX-CXXImportStd.cmake#L2-11

  find_file(_msvc_modules_json_file
    NAME modules.json
    HINTS
      "$ENV{VCToolsInstallDir}/modules"
    PATHS
      "$ENV{INCLUDE}"
      "${CMAKE_CXX_COMPILER}/../../.."
    PATH_SUFFIXES
      ../modules
    NO_CACHE)

Hmm, I'm trying to see if I understand this correctly...

So this looks for the path ../modules relative to either $ENV{INCLUDE} or ${CMAKE_CXX_COMPILER}/../../... In the case of $ENV{INCLUDE}, this contains a path like ${VCToolsInstallDir}/include, so this resolves to ${VCToolsInstallDir}/modules, and then locating modules.json within that directory. That makes sense.

And in the case of ${CMAKE_CXX_COMPILER}/../../.., this evaluates on top of a compiler path like ${VCToolsInstallDir}/bin/Hostx64/x64/cl.exe, so this becomes ${VCToolsInstallDir}/bin/Hostx64/x64/cl.exe/../../.., which is equal to ${VCToolsInstallDir}/bin, and then ../modules on top of that.

(I was thrown off for a bit with using .. on top of a file name, but I guess this gets evaluated by some path simplification logic somewhere, before trying to evaluate such a path. Or does that work in general?)

To support import std without those environment variables presets, the specific directory layout is required. There are two ways going on:

  1. Copy ${VCToolsInstallDir}/modules/* to installation root. There are only 3 files in the modules folder: modules.json, std.ixx and std.compat.ixx. So I think it is acceptable.

Hmm, how would this setup work? If this is evaluated with ${CMAKE_CXX_COMPILER}/../../.., with the compiler set to ${root}/bin/x64/cl.exe, we end up with ${root} here, and if applying ../modules on top of that, CMake would look in a directory modules next to the MSVC installation directory.

So I don't quite see how this approach would work?

  1. Create symlink modules under installation root to ${VCToolsInstallDir}/modules. This results in a different layout compared to the original MSVC tools layout.

VCToolsInstallDir layout:

${VCToolsInstallDir}/bin/Hostx64/x64/cl.exe
${VCToolsInstallDir}/modules/modules.json

Proposed msvc-wine installation layout:

${root}/bin/x64/cl.exe
${root}/modules/modules.json

This requires to add "${CMAKE_CXX_COMPILER}/../.." to above cmake code block. I prefer this way. If we agree, I am going to send a MR to CMake. But if they reject, we have to go with 1.

This looks like a quite reasonable approach - hopefully they will accept this addition. (It might be good to have comments in the cmake file explaining which file layouts these expect to operate on.)

The test is not enabled currently since the new feature is experimental and is gated by CMAKE_EXPERIMENTAL_CXX_IMPORT_STD which is a UUID string and may be updated time to time, the value is documented at https://gitlab.kitware.com/cmake/cmake/-/blob/ca449572ef8b1c6066e88879795900aea9727834/Help/dev/experimental.rst.

Thanks for looking into it!

huangqinjin commented 7 months ago

I was thrown off for a bit with using .. on top of a file name, but I guess this gets evaluated by some path simplification logic somewhere, before trying to evaluate such a path. Or does that work in general?

Yes, cmake paths are purely syntactic and do not access underlying filesystem, cmake has their own logic to do path simplification. That does not work in general (cd not-exist/.. complains No such file or directory).

To support import std without those environment variables presets, the specific directory layout is required. There are two ways going on:

  1. Copy ${VCToolsInstallDir}/modules/* to installation root. There are only 3 files in the modules folder: modules.json, std.ixx and std.compat.ixx. So I think it is acceptable.

Hmm, how would this setup work?

The key is that cmake find_* commands will first search ${some_path}/${suffix} for each suffix in PATH_SUFFIXES, and then also search without any suffix (i.e. ${some_path} itself). So cmake will found modules.json under ${root}.

mstorsjo commented 7 months ago

To support import std without those environment variables presets, the specific directory layout is required. There are two ways going on:

  1. Copy ${VCToolsInstallDir}/modules/* to installation root. There are only 3 files in the modules folder: modules.json, std.ixx and std.compat.ixx. So I think it is acceptable.

Hmm, how would this setup work?

The key is that cmake find_* commands will first search ${some_path}/${suffix} for each suffix in PATH_SUFFIXES, and then also search without any suffix (i.e. ${some_path} itself). So cmake will found modules.json under ${root}.

Oh, I see - that explains it! That was a bit non-obvious.

huangqinjin commented 7 months ago

https://gitlab.kitware.com/cmake/cmake/-/merge_requests/9442 provided a detection variable for import std. Updated accordingly.

huangqinjin commented 7 months ago

Created https://gitlab.kitware.com/cmake/cmake/-/merge_requests/9450 to support proposed installation layout.

huangqinjin commented 7 months ago

Created https://gitlab.kitware.com/cmake/cmake/-/merge_requests/9450 to support proposed installation layout.

The MR has been accepted by CMake. Using latest nightly build cmake binary from https://cmake.org/files/dev/cmake-3.29.20240423-g5ccb695-linux-x86_64.tar.gz, the import std test can run without error by adding

-DCMAKE_EXPERIMENTAL_CXX_IMPORT_STD=0e5b6991-d74f-4b3d-a41c-cf096e0b2508

to CMAKE_ARGS in test-cmake.sh. So this PR is ready.

mstorsjo commented 7 months ago

Created https://gitlab.kitware.com/cmake/cmake/-/merge_requests/9450 to support proposed installation layout.

The MR has been accepted by CMake. Using latest nightly build cmake binary from https://cmake.org/files/dev/cmake-3.29.20240423-g5ccb695-linux-x86_64.tar.gz, the import std test can run without error by adding

-DCMAKE_EXPERIMENTAL_CXX_IMPORT_STD=0e5b6991-d74f-4b3d-a41c-cf096e0b2508

to CMAKE_ARGS in test-cmake.sh. So this PR is ready.

Thanks! This looks good to me, so let's merge it.