Conversation
8f46985 to
930732c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2516 +/- ##
==========================================
+ Coverage 73.16% 73.39% +0.22%
==========================================
Files 68 68
Lines 36743 37208 +465
==========================================
+ Hits 26884 27309 +425
- Misses 9859 9899 +40 ☔ 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... |
0aec08a to
64a1a01
Compare
Look at |
|
Isn't this needed both for caching and for distributed compilation? The inputs are needed for caching to check timestamps/sizes/hashes and for distribution to ship over the files. Note hashing the preprocessed output is insufficient. The outputs are needed for caching to stash in the cache and for distribution so ship back. For distribution, we might need to massage the command line, adding -fmodule-file for each input and output module to redirect them to out place in the filesystem. |
IMO, we can think about distributed compiling support in additional PR |
sccache [1] combines ccache and distcc, and promises [2] to support modules early. Support it via a more general --compiler-cache option, replacing the current --ccache. [1] https://github.com/mozilla/sccache [2] mozilla/sccache#2516
sccache [1] combines ccache and distcc, and promises [2] to support modules early. Support it via a more general --compiler-cache option, replacing the current --ccache. The test workflow is adjusted for the new option name. [1] https://github.com/mozilla/sccache [2] mozilla/sccache#2516
|
Note that an initial investigation to at least drop "unsupported" errors and/or TODO comments in places would be useful for further work. It could also help avoid confusion by anyone trying modules and distributed builds in the interim. |
I am still traveling so I haven't had time to try out the distributed compiler. I wish there were better tests than to setup a docker container and try run it. I wasn't even aware the distributed compiler was a thing until someone pointed it out in this PRs comments. Since all the unit tests passed... If I don't do the full impl, I will at minimum add unsupported errors for modules when using distributed compiler mode. Sorry I haven't had time yet I should be fully settled down by Wednesday. |
|
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 not alter performance
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.
| msvc_flag!("openmp:experimental", PassThrough), | ||
| msvc_flag!("permissive", PassThrough), | ||
| msvc_flag!("permissive-", PassThrough), | ||
| msvc_take_arg!("reference", OsString, Separated, TooHard), |
There was a problem hiding this comment.
The MSVC flags are very similar to Clang's. -interface and -internalPartition just need hashed like any other.
Can issues be opened and referenced from the comments for GCC and MSVC support?
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