Skip to content
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

Support import std for CMake #126

Merged
merged 1 commit into from
Apr 24, 2024
Merged

Conversation

huangqinjin
Copy link
Contributor

@huangqinjin huangqinjin commented Apr 17, 2024

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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

@huangqinjin
Copy link
Contributor Author

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

@huangqinjin
Copy link
Contributor Author

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.

@huangqinjin huangqinjin marked this pull request as ready for review April 24, 2024 05:17
@mstorsjo
Copy link
Owner

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.

@mstorsjo mstorsjo merged commit 2055e1b into mstorsjo:master Apr 24, 2024
7 checks passed
@huangqinjin huangqinjin deleted the cmake-import-std branch April 24, 2024 07:58
huangqinjin added a commit to huangqinjin/cxxmodules that referenced this pull request Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants