Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #216 +/- ##
==========================================
- Coverage 97.08% 96.77% -0.31%
==========================================
Files 132 119 -13
Lines 4180 3940 -240
==========================================
- Hits 4058 3813 -245
- Misses 122 127 +5 ☔ View full report in Codecov by Sentry. |
|
Hey Gianmario, Thanks for this, it looks like a great starting point. But there are lots more we should do to this including:
I'll start making some comments and providing references for why we want to do things to help explain. Any questions please let me know and hopefully we can make this a good learning experience for us both! Cheers, |
conanfile.py
Outdated
|
|
||
| def checkCCacheIsSupported(self): | ||
| """ CCache is fully supported on Linux and macOS with gcc and clang. """ | ||
| return (self.settings.os == "Macos" or self.settings.os == "Linux") and \ |
There was a problem hiding this comment.
I think this may not be needed eventually as we should be able to get this working on Windows and with MSVC and Intel compilers too: https://github.com/ccache/ccache/wiki/MS-Visual-Studio
But let's leave it for now and focus on these to begin with.
There was a problem hiding this comment.
I explain why I made this choice; because in the ccache documentation I saw that MSVC has not A support level:
Anyway, as you said let's leave it as it is for now. I am keeping this conversation open, in this way we are not going to forget the point.
There was a problem hiding this comment.
Ah right, I think I have seen this before. Well, let's try it out and see if it's useful on Windows. Hopefully we can tie in some capture of build metrics at some point to shape our decisions.
|
|
So intertingly it looks like CCache support was not actually enabled yet. I can see this by testing with the ccache show stats command as such: Looks like it was because there was a missing option, this was needed but not defined: Easy to miss, so I want to show you how to test the ccache stats so you can confirm things are working, and also the hit rate of the cache which we may also need to tune. I've push up a fix for this, once the fix is in place I see this which suggests it's working: |
Hi Antony, |
cmake/ccache.cmake
Outdated
| cmake_path(NATIVE_PATH CCACHE_BIN ccache_exe) | ||
| file(WRITE ${CMAKE_BINARY_DIR}/launch-cl.cmd | ||
| "@echo off\n" | ||
| "\"${ccache_exe}\" \"${CMAKE_C_COMPILER}\" %*\n" |
There was a problem hiding this comment.
So it looks like we only generate a cmd invoke wrapper for the C compiler, is this correct? I'd presume we need this for every language supported (I may be wrong here but be good to understand the rational)?
There was a problem hiding this comment.
Quoting chapter 35.4.4. Visual Studio Generators:
Using Ccache with the Visual Studio generators requires Ccache 4.6.1 or later. The
CMAKE_<LANG>_COMPILER_LAUNCHER variables don’t support Visual Studio generators, but Visual Studio
project properties can be used in conjunction with a launch script to achieve the same thing. The
CLToolPath and CLToolExe project properties redirect the compile steps to use a custom compiler
executable or script. The properties can be set with the CMAKE_VS_GLOBALS variable (see Section 35.3.3,
“Visual Studio Generators” for another closely related use of this variable).
The same compiler executable is used for C and C++ with these generators. Thus, only one launch
script is needed, unlike the Xcode generator which requires one launch script per language.
That's the reason why I did it in this way. I could easily be wrong because I am a newbie and I am also developing on a Linux machine 😄
There was a problem hiding this comment.
Ha, it may the the correct thing to do but I think it requires a little investigation and testing for each supported language. I can tests on my Windows machine.
cmake/ccache.cmake
Outdated
| set(CMAKE_XCODE_ATTRIBUTE_LDPLUSPLUS ${launchCXX}) | ||
| endif() | ||
| endforeach() | ||
| elseif(${CMAKE_GENERATOR} MATCHES "Visual Studio") |
There was a problem hiding this comment.
I imagine with Visual Studio support we have to limit the languages to language implicitly supported by the MSbuild engine?
There was a problem hiding this comment.
Ah probably yes, sorry but I am missing this one. MSBuild engine supports C and CXX I suppose. The documentation is there and from a quick sight It seems supportin C with MSBuild and CXX with MSBuild++.
There was a problem hiding this comment.
Yes, and Cuda should be good. But I'd expect Objective C/C++ to not work here. Main point here is I think we need to explicit test support for each language (and be we I mean me as I have a Windows Laptop with Visual Studio installed which I can test on)
There was a problem hiding this comment.
I need to install Visual Studio also. Build instructions for Windows are explained there right?
There was a problem hiding this comment.
Hey, that's pretty close (Heisenberg used Conan 1). I should write up a page for Windows instructions too. Let me know if you hit any issues trying this out.
There was a problem hiding this comment.
I am taking a bunch of notes while I am building the library for Windows. I took notes of all the steps that I did because I started from scratch. I can write a build instruction page for Windows. I will do it as soon as we have ccache working.
cmake/ccache.cmake
Outdated
| if(${CMAKE_GENERATOR} MATCHES "Ninja|Makefiles") | ||
| message(STATUS "found generator Ninja|Makefiles") #debug | ||
| foreach(lang IN ITEMS ${MORPHEUS_LANGUAGES}) | ||
| set(CMAKE_${lang}_COMPILER_LAUNCHER ${CCACHE_BIN}) |
There was a problem hiding this comment.
This is a good starting point, but this is where it gets more interesting. That is because we need to be able to pass through settings to ccache. The mechanism for this in ccache is to set environment variables for each parameter that is controllable.
CMake has a very specific way of doing this, you can use the ${CMAKE_COMMAND} variable to invoke a CMake subprocess to launch your process with environment variable propagated: https://cmake.org/cmake/help/latest/manual/cmake.1.html#run-a-command-line-tool. So for example this could be achieved using the ${CMAKE_COMMAND} -E env ${ccacheEnvVariables} ${CCACHE_BIN} command passed to the compiler launcher.
To quote the CMake Professional book's section on Ccache:
#For most projects, the above example is too simplistic. It does not set any Ccache environment variables, it
#relies purely on the user-level configuration of that tool. CMake’s command mode can be used to set the
#relevant environment variables when invoking Ccache on any platform. The following is a more realistic example:
find_program(CCACHE_EXECUTABLE ccache)
if(CCACHE_EXECUTABLE)
set(ccacheEnv CCACHE_SLOPPINESS=pch_defines,time_macros) # NOTE: Ccache 4.2+ required for reliable CUDA support
foreach(lang IN ITEMS C CXX OBJC OBJCXX CUDA)
set(CMAKE_${lang}_COMPILER_LAUNCHER ${CMAKE_COMMAND} -E env ${ccacheEnv} ${CCACHE_EXECUTABLE})
endforeach()
endif()
There was a problem hiding this comment.
Anyway, in summary the next step is to look at the Ccache documentation and see which variable we should be setting and then expose them by CMake variable or parameter to the enable_cache method.
There was a problem hiding this comment.
Good approach. I will try to start with the following that seem useful:
CCACHE_SLOPPINESS=pch_defines,time_macros
CCACHE_PREFIX=icecc (if Icecream executable is present)
CCACHE_LOGFILE=~/.ccache/ccache_logfile.txt (to understand in the detail what ccache is doing)
I guess we will discover the others that we need once we start to build with ccache consistently. What do you think?
There was a problem hiding this comment.
Yes, completely agree! These variable sounds like a good start. I need to spend time time reading through the docs to understand all of the options but we can add incrementally to it as we build our understanding.
CMakeLists.txt
Outdated
| add_subdirectory(examples) | ||
|
|
||
| if (MORPHEUS_BUILD_WITH_CCACHE) | ||
| set(MORPHEUS_LANGUAGES CXX) |
There was a problem hiding this comment.
Actually, the are also parts of Objective C/C++ in the Metal redendering library and I'm also planning for Cuda support in the future.
Perhaps we should move this out of the if statement and define this as a top-level variable for the project.
There was a problem hiding this comment.
Perfect I will move it out of the if statement. The reason why I created a dedicated variable MORPHEUS_LANGUAGES is that unfortunately the project command option LANGUAGES doesn't set any variable:
project(<PROJECT-NAME>
[VERSION <major>[.<minor>[.<patch>[.<tweak>]]]]
[DESCRIPTION <project-description-string>]
[HOMEPAGE_URL <url-string>]
[LANGUAGES <language-name>...])
I will move it out of the if statement and rename it as CCACHE_LANGUAGES and pass it to the function.
|
|
||
| find_program(CCACHE_BIN ccache REQUIRED) | ||
| if(NOT CCACHE_BIN) | ||
| message(STATUS "Morpheus: CCache not found. Rebuilding all the source files.") |
There was a problem hiding this comment.
Actually, these messages are not needed, because of the REQUIRED parameter to find_program. If ccache is not found CMake processing will stop with an error message at that point. So I don't believe these messages could ever be triggered, see docs for find_program with the REQUIRED param: https://cmake.org/cmake/help/latest/command/find_program.html
| message(STATUS "Morpheus: CCache not found. Rebuilding all the source files.") |
There was a problem hiding this comment.
You are right, message will not be triggered because of the REQUIRED parameter. In this case I think that we should remove it, because for ccache in conanfile.py we are exposing an option build_with_ccache. Then it can effectively happen that the message will be triggered right?
For now, I think it is enough to expose a way to set this field. Icecream and other distributed compilation tools require a co-ordinator and other nodes to offload compilation too. I have no idea how we could test this until we have a gid of machines. As long as we can set it then when the time comes we will be able to test it. But to answer your point, yes, I'd go with Icecream as I have previously used this and its simple to set up. When were are in a position to use try this I'd expect to create a Conan package for this (which I have done for a few libraries now). |
Sorry for the newbie question, "to create a Conan package for this" it means that we will contribute to the Conan project |
Yes, exactly that. I have done this for a few libraries already so have some experience with this so if need be I think we can do this. |
|
Hi Antony, But the Windows build it's still tricky! If we keep the LINK: command "C:\PROGRA~1\MICROS~2\2022\ENTERP~1\VC\Tools\MSVC\1438~1.331\bin\Hostx64\x64\link.exe /nologo @CMakeFiles\MorpheusCoreTests.Release.rsp /out:bin\Release\MorpheusCoreTests.exe /implib:lib\Release\MorpheusCoreTests.lib /pdb:bin\Release\MorpheusCoreTests.pdb /version:0.0 /machine:x64 /INCREMENTAL:NO /subsystem:console /MANIFEST:EMBED,ID=1" failed (exit code 1120) with the following output:
enums.tests.cpp.obj : error LNK2019: unresolved external symbol "public: static class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl Catch::StringMaker<class std::basic_string_view<char,struct std::char_traits<char> >,void>::convert(class std::basic_string_view<char,struct std::char_traits<char> >)" (?convert@?$StringMaker@V?$basic_string_view@DU?$char_traits@D@std@@@std@@X@Catch@@SA?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@V?$basic_string_view@DU?$char_traits@D@std@@@4@@Z) referenced in function "class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl Catch::Detail::stringify<class std::basic_string_view<char,struct std::char_traits<char> > >(class std::basic_string_view<char,struct std::char_traits<char> > const &)" (??$stringify@V?$basic_string_view@DU?$char_traits@D@std@@@std@@@Detail@Catch@@YA?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@AEBV?$basic_string_view@DU?$char_traits@D@std@@@3@@Z)So to get a 100% success hit rate for Windows I had to do the following trick:
The limit of this manual solution approach is that everything is working fine until we add/update packages in the Thanks, |
|
Hi Gianmario, Great work mate, absolutely heroic effort to get this working! :) Regarding the Catch2 issues, this is a known issue: conan-io/conan-center-index#19008. It's been causing pain for a while now, and is causing issues on other branches: https://github.com/Twon/Morpheus/actions/runs/7003719483/job/19049991146?pr=237 I should bite the bullet here and raise a PR to fix the Conan package as this has been causing issues for months now. If you don't mind holding off submitting this for a little longer I'll aim to get a fix raised in the next couple of days. BTW, regarding your question on the next topics to address, I hope to come back shortly with a few suggestions here. Cheers, |
No struggle, no learning 😄
Ah! I was not aware that this was a known issue. Let me know if I can help with this. I am looking right now at the Conan recipe for
No worries, take your time. Meanwhile I am trying to educate myself. Thanks. Cheers, |
… into ccache_build_support
|
Quick update to say I have not forgotten about this. I have managed to get the fix to Catch2 merge now: conan-io/conan-center-index#19008 and have a fix for the similar issue with GTest: conan-io/conan-center-index#23854. Once merged this should clear the way for this to go in. |
|
I've now got the fix for GTest merged into to Conan Index Center so nothing is blocking this now. I will start working through this PR this week to understand the changes and provide some feedback. Thanks again for your heroic efforts thus far! |
|
|
||
| Visual Studio 2022 can be downloaded from Microsoft [website](https://visualstudio.microsoft.com/vs/community/) | ||
| A minimum working installation to support the project build will have the following details: | ||
|  |
There was a problem hiding this comment.
This image should be moved to a specific folder for images rather than be left in the root (we want to keep the roots as uncluttered as possible). Let's move this to <project root>/docs/images, and keep all other images there for now.
| @@ -0,0 +1,58 @@ | |||
| ## Visual Studio and Ninja Multi-Config Build Instructions | |||
There was a problem hiding this comment.
Let's move this file to be located at <project root>/docs. The only document in the root should be theREADME.md, which should redirect to all other documentation (ideally within the docs folder, or subdirectories of that.
| enable_ccacche( | ||
| [DEBUG] | ||
| [ICECC]<icecc path> | ||
| [LOGFILE]<ccache logfile path> |
There was a problem hiding this comment.
This is an optional parameter so does not take a logfile path.
| [LOGFILE]<ccache logfile path> | |
| [LOGFILE] |
Two potential courses of action here, rename it to LOGGING or ENABLE_LOGFILE so it's clearly an optional value, or change it to a single value so a path location can be configured dynamically. I tend to prefer the first for simplicity.
2 participants
Hi Antony,
First draft of the CCache support #113.
Thanks in advance for your time.
Cheers,
Gianmario