partial c++20 module support#2516
Conversation
8f46985 to
930732c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2516 +/- ##
==========================================
+ Coverage 73.27% 73.39% +0.12%
==========================================
Files 68 68
Lines 36907 37219 +312
==========================================
+ Hits 27042 27316 +274
- Misses 9865 9903 +38 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mathstuf
left a comment
There was a problem hiding this comment.
The code looks good to me. I'll probably try to test it some time next week.
I can help out with GCC module map file parsing too (it will need to actually be parsed due to the $root meta-descriptor). However, the output path is also given by it…that may prove more difficult to "know".
Thanks! It was my first time contributing to sccache so I'm not too familiar with the process. I will fix up the clippy issues + windows test issues :p |
489600b to
71e9190
Compare
|
It looks like BMI-only compilations are not properly considered: This is from running CMake's export SCCACHE_SERVER_PORT=23448 # unique to avoid ambient re-usage
export CXX="$(which sccache) $(which clang++)"
ctest -R CXXModules |
|
You can ignore the tests failing about JSON argument list mismatches; it is not robust against mutli-argument compilers like here. If you do the "symlink trick" ( |
|
Hmm, i think this is just me forgetting to change it somewhere. I will fix |
|
Okay so it seems i think we should always treat it as an object even if it really isnt... |
c0a57b6 to
8841d54
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
could you please add an integration test ? ie high level which integrates into cmake and uses C++ module. |
There are dozens of test cases within each CTest-level test. These are the relevant ones, thanks! Can you inspect the cache state ( |
Yes! |
This comment was marked as outdated.
This comment was marked as outdated.
|
https://github.com/mozilla/sccache/actions/runs/20413181927/job/58653103431 test results look good! |
|
are you sure ? |
|
That seems right no? the first time we invoke the compiler not cached yet then we invoke again and its cached. There are only 2 files to compile, so i expected it to be 4 requests 2 hit 2 misses. |
|
oh yeah, my bad, i thought we were doing a reset |
I just copied the other cmake test example, but I can reset the stats if that makes it easier to follow |
|
Gah, module compilation in the tests doesn't really happen without a setting. For
|
src/compiler/gcc.rs
Outdated
| } | ||
|
|
||
| if !module_only_flag { | ||
| if let Some(module_output_path) = module_output_path { |
There was a problem hiding this comment.
could you please move this into a function?
There was a problem hiding this comment.
I can, however this function is ~550 lines, I think it would be better if in a followup PR we break up this function rather than doing a half job here by breaking up just this one conditional.
There was a problem hiding this comment.
i still want this to be a new function, sorry
Ah, I see. Okay I will try again i think i need to setup my env to have support for import std. |
let
pkgs = import <nixpkgs> {
overlays = [
(final: prev: {
llvmPackages_21 = prev.llvmPackages_21 // {
libcxx = prev.llvmPackages_21.libcxx.overrideAttrs (old: {
postInstall = (old.postInstall or "") + ''
substituteInPlace $out/lib/libc++.modules.json \
--replace-fail '"../share' "\"$out/share"
'';
});
};
})
];
};
llvm = pkgs.llvmPackages_21;
wrappedClangScanDeps = pkgs.writeShellScriptBin "clang-scan-deps" ''
newargs=()
inject_next=false
for arg in "$@"; do
if $inject_next; then
newargs+=("$arg")
newargs+=("-isystem" "${llvm.libcxx.dev}/include/c++/v1")
inject_next=false
elif [[ "$arg" == "--" ]]; then
newargs+=("$arg")
inject_next=true
else
newargs+=("$arg")
fi
done
exec ${llvm.clang-tools}/bin/clang-scan-deps "''${newargs[@]}"
'';
sccache = "/home/troy/github/troy/sccache/result/bin/sccache";
clangWrapper = pkgs.writeShellScriptBin "clang" ''
exec ${sccache} "${llvm.libcxxClang}/bin/clang" "''${@}"
'';
clangxxWrapper = pkgs.writeShellScriptBin "clang++" ''
exec ${sccache} "${llvm.libcxxClang}/bin/clang++" "''${@}"
'';
in
pkgs.mkShell {
hardeningDisable = [ "all" ];
buildInputs = [
pkgs.cmake
pkgs.mold
pkgs.ninja
pkgs.openssl
llvm.libcxxClang
llvm.clang-tools
wrappedClangScanDeps
];
shellHook = ''
export CC="${clangWrapper}/bin/clang"
export CXX="${clangxxWrapper}/bin/clang++"
export LD_LIBRARY_PATH="${llvm.libcxx.out}/lib:${pkgs.openssl.out}/lib:''${LD_LIBRARY_PATH:-}"
export NIX_CFLAGS_COMPILE="-B${llvm.libcxx.out}/lib -isystem ${llvm.libcxx.dev}/include/c++/v1"
export PATH="${wrappedClangScanDeps}/bin:''${PATH:-}"
'';
}A huge pain... |
|
There's no rush on my account at least; take your time. |
|
Hi @TroyKomodo, Do you have any updates? Looks like we have conflicts. |
64a1a01 to
864b5b0
Compare
Merging this PR will degrade performance by 3.49%
Performance Changes
Comparing Footnotes
|
|
Okay! So i finally have gotten the time to look through this PR again. I think, this automatically works with dist mode since we add the PCMs to the Could someone please provide me with a way to test this or someone test this for me, using the integration test in the PR. Integration tests fail because this branch is from a fork. |
cf216c5 to
10fc4f6
Compare
why do you think that ? |
mathstuf
left a comment
There was a problem hiding this comment.
Seems fine as a stepping stone towards actual support. Just one minor request.
Initially it spoke about missing an auth token when uploading. I think that was fixed in some other PR, after a rebase i think you (or me) resolved a merge incorrectly and it included additional ci jobs which were removed/changed which is why CI was failing before my last commit. |
|
Thanks a lot for implementing this feature. |
Adds partial support for c++20 modules when using clang.
Also detects when modules are used in MSVC / GCC and disable cache for those calls.
fixes #2216
partially addresses #2095
the clang c++20 module implementation (as well as msvc) is rather easy to parse and understand so adding support for it is fairly trivial. GCC however is a mess. An entirely different approach will be needed to get GCC module support which is far out side of the scope for this initial PR.
I will submit future PRs to get MSVC / GCC.
Test with nix