Debug build with Ninja (requires storage of pdb files)

[Knowledge level: 5 minutes of DuckDuckGoing.] Do I understand correctly that the above means that /Zi and /ZI should be made TOO_HARD until support has been implemented? Any other options that should be too hard?

Precompile headers support (detection and storage of pch files)

Similarly: Should /Yc be too hard?

Release build with NMake Makefiles (requires usage of response files)

Does this mean to write a parser similar to args_init_from_gcc_atfile but for an MSVC-specific format?

We are also using ccache for our Linux builds and we do need a solution for MSVC. I've been experimenting for some days now with the similar python based clcache and they also don't support /Yc and /Zi or /ZI. We haven't been using precompiled headers so far, but I switched a group of larger projects from /Zi to /Z7 and I didn't feel any disadvantage so far, so I think it is probably the easiest option to add /Zi, /ZI and /Yc into the TOO_HARD list for a first initial MSVC support.

@cristianadam: Are you still interested in working on this?

@cristianadam: Are you still interested in working on this?

Yes. I will be having some time to work on this next week.

Supporting /Z7 would be super useful.

As suggested by @jrosdahl I created a pull request cristianadam#1 to suggest updating that branch instead of the pull request running in parallel to this one. The pull request essentially forward-ports the changes from this branch to the current ccache master branch. In addition to that some trivial reformatting is taking place to make the clang-format test happy.

What exactly is still missing for this feature to be integrated?
We are already using a fork of this branch productively, so we would be happy if it gets integrated into the master.
If it's blocked by things like the CI tests or something like that, we might be able to fix that.

I've updated the MR on top of the 4.4 release.

I've added /Zi as too hard, this means that if you are using Debug and RelWithDebInfo will get cache misses, and get a correct build.

In order to use ccache with Debug and RelWithDebInfo you need to replace /Zi with /Z7, which means that the debug information is stored in .obj files, and not in the compile pdb file, which is other compiler caching programs (sccache, buildcache) require.

To use is with CMake projects you need to have in your CMakeLists.txt somewhere:

if (MSVC)
  foreach(config DEBUG RELWITHDEBINFO)
    foreach(lang C CXX)
      set(flags_var "CMAKE_${lang}_FLAGS_${config}")
      string(REPLACE "/Zi" "/Z7" ${flags_var} "${${flags_var}}")
    endforeach()
  endforeach()
endif()

Now it works with CMake and Ninja generator in Release, Debug / RelWithDebInfo with /Z7. I updated the top message with what's missing.

Next week I'll be away from keyboard, but when I come back I'll have a look at the missing items.

@jrosdahl how should I proceed wit this MR? @llunak has done some fixed in #952 and #954 which fix things that I haven't covered here.

The unresolved conversations have fixes in other MRs. Should I cherry-pick those fixes into this MR and mark the resolved conversations?

If I decide to merge MSVC support before #956, I think you can leave the current PR mostly as is and then we'll apply follow-up fixes separately.

@jrosdahl wrote:

I think ccache from msvc build is generally broken here.

Note that building ccache itself with MSVC is experimental work-in-progress and nothing that is supported so far.

It was not meant to be harsh. With 'genarally' I wanted to say that with the msvc build the cached outputs always create two newlines, not only when cl is used.
In #943 I had to add a hack to make the builds run green again. But as I said b8c81b7 and 60a6d2d fix the bug.

What I'm debating with myself at the moment is if I should accept even more complexity without a compiler abstraction, especially if the support comes without a test suite.

The important stuff (A level support code) comes with a test suite, so this PR breaking anything else is unlikely, and if it does then that's primarily responsibility of that existing test suite. As for this PR not having tests, well, that's why it's B level, and it can change (and it's more likely to change if it's not made hard by hold this to the same standards as the existing A level code). Going from no MSVC support to MSVC support that maybe doesn't work in some cases is no regression. And in case you misssed it in #447 , I've built pretty much the entire LibreOffice including its bunch of 3rd-party libraries with PR plus my fixes on top.

A related problem is that it's hard for me to maintain support for compilers that either don't work on Linux or are not freely available.

I thought the idea of B level was that you do not have to maintain support for it. Otherwise what's the point of the B level if there's no actual difference?

I would ideally want to be able to have tests for all supported compilers when developing locally, without having to rely too much on CI and especially not on having commercial compilers installed.

MSVS has a community edition that is no less free than nvcc or icc. And you can test this on Linux too if you really want to, either clang-cl works anywhere where clang does (although it may need MS SDK headers, https://jake-shadle.github.io/xwin/) or apparently it's possible to use MSVC with Wine (https://github.com/mstorsjo/msvc-wine).

I would personally like to work on a proper compiler abstraction, but that would definitely take some time to finish, so the question is what to do with MSVC support in the meantime. Wait until compiler abstraction support is in place? Merge to branch? Merge to master (maybe under a feature flag) and try to live with the increased messiness?

#956 may make some things cleaner, but it's not blocking for this. 'wc -l src/cache/*' is ~15kLOC and MSVC support is what, 300 out of that maybe? As for messiness, this PR has 8 compiler type checks and my latest source 13, and they are usually simple if blocks. For comparison, master has already ~20 compile_type checks. So it's not like this PR makes it way worse.

In any case, I won't merge this before 4.5 since I would like to get some other changes out before thinking more about this.

You're the maintainer. I just hope you do realize that both asking people to take care of something and making it hard at the same time doesn't really work.

@rvogg wrote:

I think ccache from msvc build is generally broken here.

Note that building ccache itself with MSVC is experimental work-in-progress and nothing that is supported so far.

Works for me.

@cristianadam @rvogg : The only two actually unresolved issues in this PR are the @ file support (and even that is IMO hardly a blocker). Does either of you have (plans for) a follow-up commit to handle that?

@cristianadam @rvogg : The only two actually unresolved issues in this PR are the @ file support (and even that is IMO hardly a blocker). Does either of you have (plans for) a follow-up commit to handle that?

This MR support @file with the CMake's "NMakefile Generator", and in my test it compiled fine.

I haven't tested arguments with spaces, the only example I can think of would be a directory with spaces in it, source fiels with spaces are not very common.

I see this as a bug fix for later, than a requirement of having this MR accepted. I can have a look at this case, sure.

@cristianadam @rvogg : The only two actually unresolved issues in this PR are the @ file support (and even that is IMO hardly a blocker). Does either of you have (plans for) a follow-up commit to handle that?

This MR support @file with the CMake's "NMakefile Generator", and in my test it compiled fine.

I haven't tested arguments with spaces, the only example I can think of would be a directory with spaces in it, source fiels with spaces are not very common.

I see this as a bug fix for later, than a requirement of having this MR accepted. I can have a look at this case, sure.

That is ok for me, I've already prepared the changes: cristianadam#3, but I can also make a new pull request here.

@llunak wrote:

'wc -l src/cache/*' is ~15kLOC and MSVC support is what, 300 out of that maybe? As for messiness, this PR has 8 compiler type checks and my latest source 13, and they are usually simple if blocks. For comparison, master has already ~20 compile_type checks.

Please don't try to trivialize my concerns like this. That is very demotivating for me.

So it's not like this PR makes it way worse.

It's exactly like this PR makes it worse, but only a bit. Yes, just adding a little more messiness just adds a little more messiness, but you can't keep doing that over and over again even if each addition just makes things slightly messier.

I'll merge this after 4.5 is released, which I hope to be able to do in a couple of days.

Please don't try to trivialize my concerns like this. That is very demotivating for me.

Sorry. I didn't know 4.5 was meant to be out soon, so your comments here made it look to me like you were planning to postpone this PR for a long time after some rewrites that could possibly lead to all this needing to be rewritten all over again.

I'll merge this after 4.5 is released, which I hope to be able to do in a couple of days.

I think it might make sense to keep a list of things needed for MSVC support, such as adding tests for this, so that we can sort all that out and not step on each others toes. I don't know where it'd be a good place to keep it, separate issues, wiki page or something else?

Thanks.

Thanks.

Thank you!

Congrats on the merge. Will this be upgrading MSVC to a B on the supported compilers page?

Congrats on the merge. Will this be upgrading MSVC to a B on the supported compilers page?

Yes (as already indicated on said page).

I would very much like to see this Precompile headers support (detection and storage of pch files) it's the only thing we are missing on msvc to use the ccache in everyday development. I'm already using the ccache in Github actions without any problem thx @cristianadam

qmake is using something like this for precompiled headers:

cl -c -FI..\include\pch.h -Yu..\include\pch.h -Fpdebug\TinyOrm_pch.pch ...

And cmake this:

cl.exe /YcE:/<path>/build-TinyOrm-Desktop_Qt_6_3_1_MSVC2022_64bit-Debug/CMakeFiles/TinyOrm.dir/cmake_pch.hxx 
/FpE:/<path>/build-TinyOrm-Desktop_Qt_6_3_1_MSVC2022_64bit-Debug/CMakeFiles/TinyOrm.dir/./cmake_pch.cxx.pch 
/FIE:/<path>/build-TinyOrm-Desktop_Qt_6_3_1_MSVC2022_64bit-Debug/CMakeFiles/TinyOrm.dir/cmake_pch.hxx

The thing is that msvc's pch model is different from gcc's (better actually). They use a dummy source file and also produce an .obj file with code and debug info in addition to the pch file.

This is not good news if it is as you write, if msvc creates .obj for PCH in a different format than gcc so it means a new or customized caching logic will have to be created which doesn't exist now, which means more work. Build cache for dev. is really missing on msvc I have searched and didn't find anything working correctly.

gcc doesn't produce code, only a .pch. Clang can be made to generate an .obj cause they need that for clang-cl mode, I think by now it's -fpch-codegen -fpch-debuginfo. Though it's also a bit special since you need to "compile" the .pch file after generating it, if my notes are still correct:

clang++ -x c++-header header.h -o header.pch -fpch-codegen -fpch-debuginfo
clang++ -c header.pch -o shared.o

Thank you for this. This cuts the github action time for our Windows CI more than in half in a 'perfect hit rate' situation, from a typical runtime of ~1:15 to about 25 minutes (15 minutes fixed cost to execute tests, 1 hour of setup + build time down to 10 minutes). Many PRs don't change any c++ code so will see maximum benefit from this. That even includes folding a separate job into the same job to reuse build artifacts, so freeing up a whole runner slot per PR.