| View Issue Details [ Jump to Notes ] | [ Print ] | ||||||||
| ID | Project | Category | View Status | Date Submitted | Last Update | ||||
| 0014240 | CMake | CMake | public | 2013-06-21 02:52 | 2014-03-05 09:58 | ||||
| Reporter | mh | ||||||||
| Assigned To | Peter Kuemmel | ||||||||
| Priority | normal | Severity | feature | Reproducibility | N/A | ||||
| Status | closed | Resolution | fixed | ||||||
| Platform | PC | OS | Win, Linux, OSX | OS Version | |||||
| Product Version | CMake 2.8.11.1 | ||||||||
| Target Version | CMake 3.0 | Fixed in Version | |||||||
| Summary | 0014240: Ninja: Support new, faster header dependency handling | ||||||||
| Description | A newer, supposedly faster way of header dependency handling was added to Ninja in version 1.3.0. (see https://groups.google.com/d/msg/ninja-build/v8FTKNcY87Q/noQq1N2m8FoJ [^] and http://martine.github.io/ninja/manual.html#ref_headers [^]). It would be great if CMake would support that. | ||||||||
| Tags | No tags attached. | ||||||||
| Attached Files |  ninja_deps.patch [^] (9,754 bytes) 2013-10-10 04:14 [Show Content]
ninja_deps_with_clang.patch [^] (9,779 bytes) 2013-10-10 04:48 [Show Content]
fix_Sln_test.diff [^] (1,813 bytes) 2013-10-17 15:48 [Show Content]
| ||||||||
| Relationships | |
| Relationships |
| Notes | |
|
(0034089) Edwin Jacques (reporter) 2013-10-10 04:14 |
I made a patch for this feature. It passes the regression suite. Seems to work fine for me (I generate Eclipse CDT4 Kepler project files with Ninja backend). |
|
(0034090) Edwin Jacques (reporter) 2013-10-10 04:18 |
Not sure what policy you wanted in place for backward compatibility, but since Ninja support is still listed as "experimental" it seems to make sense that 1.3 be required. I almost added some code to specify that into the ninja files, but I hate adding magic numbers to code. |
|
(0034091) Edwin Jacques (reporter) 2013-10-10 04:49 |
Enhancement to support clang as well. Tested with clang to build cmake code via ninja. Also passes all regression tests. |
|
(0034099) Bill Hoffman (manager) 2013-10-10 13:46 |
Does this remove the need for the cmake depend helper program on windows? Has this been tested on windows? |
|
(0034103) Edwin Jacques (reporter) 2013-10-11 09:40 |
I am not as familiar with use of cmake on Windows. This feature I enabled for the ninja build should allow include dependencies discovered during C/C++ compilation to be saved in ninja's dependency database instead of individual dependency files per source file. I don't know anything about the depend helper program that cmake uses. Maybe the depend helper can be enhanced to use the ninja dependency database in some/all cases? Should that work be in a separate enhancement request? I have not yet tested on Windows. I added the code I think it needs to work. I will have to set up a Visual Studio 2012 environment to test that my enhancement works on Windows, and that all cmake regression tests pass. I will be traveling, so I will not get to this for a few days. Thank you. |
|
(0034109) Peter Kuemmel (developer) 2013-10-12 07:01 edited on: 2013-10-13 06:40 |
This patch doesn't remove the cmcldeps usage. Therefore all hits of 'grep cldeps -i -r *' in the source tree have to be removed. Ninja 1.4 only supports English MSVC when parsing /showInclude: https://github.com/martine/ninja/issues/613 [^] A work-in-progress branch is here (based on the clang patch) https://github.com/syntheticpp/CMake/commits/ninja-remove-cmcldeps [^] It misses 1. the figuring out of the /showInclude prefix, and 2. a ninja version which supports such a prefix variable. And currently cmcldeps is also used to generate dependencies for .rc files, so maybe we can't remove it anyway. |
|
(0034133) Edwin Jacques (reporter) 2013-10-16 23:56 |
So, should we be bothering to try to remove cmcldeps? I finally could generate a ninja build of cmake and build it. Some of the regression tests related to dependencies fail. I suspect that it is because cmcldeps and "ninja deps" are not compatible. "ninja deps" feature actually causes the dependency files to be deleted after the compile command is invoked and dependencies are loaded into the ninja dependency database. If cmcldeps is trying to read the file, it will be gone. Personally, I think this is a problem with ninja. I don't know why ninja bothers removing the file, since it doesn't hurt. Actually, the unlink probably slows down the ninja build a little. I will try to see if changing ninja will allow cmake to work with the "ninja deps" feature in Visual C++. For now, would it be acceptable to only have support for this feature for clang and gcc? Clang and gcc pass the regression tests. At least users of cmake/ninja on most Unix platforms would work. Alternatively, we could leave the implementation as is and note that a compatible version of ninja is required for the feature to work properly on Windows with cl.exe compiler (and even with that, it will only work well for English code due to ninja limitations). Should I make a new patch that removes msvc support for this feature, or leave it as is? I'll let you know how my testing goes with a modified ninja. Thank you. |
|
(0034134) Edwin Jacques (reporter) 2013-10-17 00:47 |
As I expected, the dependency tests (ctest -R Dep) pass when I change ninja to leave the dependency file around. It does not appear to alter the functionality of ninja significantly. It was easy to change the ninja regression tests to make ninja tests pass. I will ask on the ninje developer list whether the dep files can be left by default and removed if an option is specified. Thank you, --Edwin |
|
(0034135) Edwin Jacques (reporter) 2013-10-17 01:39 |
Hmm... I seem to still have a couple of failures even after hacking ninja on Windows. I'm not quite sure how they are related: The following tests FAILED: 147 - MFC (Failed) 210 - CMakeLib.testVisualStudioSlnParser (Failed) Errors while running CTest The MFC test is failing with the following error: 147: FAILED: C:\PROGRA~2\WI3CF2~1\8.0\bin\x86\rc.exe /showIncludes -D_AFXDLL / foCMakeFiles\mfc1.dir\mfc1.rc.res C:\JACQUE\cmake_syntheticpp_Retail\Tests\MFC\m fcShared-prefix\src\mfcShared\mfc1.rc 147: C:\JACQUE\cmake_syntheticpp_Retail\Tests\MFC\mfcShared-prefix\src\mfcShared \mfc1.rc(266) : error RC2104 : undefined keyword or key name: DIALOG The SlnParser is test may be counting files wrong (need to check): 210: cmVisualStudioSlnParser returned bad number of projects (50 instead of 49) I am more concerned about the first failure. I'd appreciate some advice on that. Indeed, I can not find where DIALOG should be getting defined. |
|
(0034136) Peter Kuemmel (developer) 2013-10-17 03:47 |
Which code have you tested? My patch completely removes cmcldeps, but I later realized we need it for rc compiling (the failed MFC test). The .sln parser test lists cmcldeps, which is gone, maybe it breaks because of this. But the dependency test should work also with deps = msvc because principally it is not important how a dependency is found. |
|
(0034139) Edwin Jacques (reporter) 2013-10-17 11:30 |
What I did was take the code in the ninja-remove-cmcldeps branch and build it. This is the code where the Deps tests failed. The Deps tests passed when I hacked ninja to leave the Deps files around. I will look further into the failure. I am now supposing that the test is looking for the dependency file as a success criteria, and it is missing. If that's the case, the test criteria has to be changed (it should expect deps file to be missing if building with ninja and compiler is msvc). I will test it with an unmodified ninja. I can also modify the SlnParser test so that the dependency on cmcldeps is removed. Questions for you: Why does the MFC test require cmcldeps? Can you explain what problem it is solving? Perhaps we can leverage ninja deps to provide equivalent functionality somehow. Is it acceptable to leave cmcldeps step for rc compilation to avoid the failure? I will try the latter approach first. Finally, is the localized /showIncludes problem really a cmake problem or a ninja problem? Can we get approval for the "ninja deps" feature and release-note the deficiency in ninja? I would like to get this change staged for release. Also, if the problem is a ninja problem, is it OK to copy code from CMake into ninja, or are there some legal considerations? |
|
(0034140) Peter Kuemmel (developer) 2013-10-17 12:04 |
The MFC test requires cmcldeps for the .rc file processing. Because the rc compiler doesn't support something like /showIncludes we currently first "compile" the .rc file with cl.exe to generate the dependency file, and then we call the real rc compiler. The patch in ninja-remove-cmcldeps is ATM not correct because it completely removes cmcldeps (my fault), but we need it for the rc files, so finally we will still have cmcldeps (which should also fix the .sln test, I assume we don't have to touch SlnParser). For localized /showIncludes we have to fix Ninja and CMake. CMake has to extract the localized string and pass it to Ninja which has to strip it when parsing the output of cl.exe. Because Ninja support is marked as experimental I assume we could just drop the old cmcldeps usage, and require the ninja version which supports the localized cl output parsing 1.5/1.4.1. AFAIK cmake's build server uses ninja master, so when we have our ninja patches upstream we could also stage our patches. I will update the ninja-remove-cmcldeps soon. |
|
(0034141) Edwin Jacques (reporter) 2013-10-17 15:46 |
I am sorry, I did not realize that you were already well on your way to modifying both ninja and cmake. I downloaded your ninja mods here: https://github.com/syntheticpp/ninja/tree/localized-showinclude/src. [^] I found that the ninja_test did not pass. All it needed was for the new method signatures to have the default value of deps_prefix set to "". That way all the old tests pass. Then I modified one test and added another to make sure I could specify a non-default prefix string. There was one other bug in the ninja_test where is was doing a gratuitous ToLower of a directory name that causes tests to fail if the temp directory had a capital letter in it. The Deps tests are passing now without modification. The Sln test needed to have the cmcldeps ProjectSection removed from Tests\CMakeLib\testVisualStudioSlnParser_data\valid.sln-file I will attach a diff for that. The MFC test is still failing, and I don't know why. I suppose if we can fix that, then this change can be promoted? Or will we have to wait for Ninja 1.5 to be released? Or can we just put a release note that someone should build ninja from master to use this "experimental" feature? |
|
(0034142) Peter Kuemmel (developer) 2013-10-17 16:02 |
"The MFC test is still failing": I have to restore the RC handling in the patch. And I would not use a default parameter for the prefix parameter, because then one can't oversee that this function needs a prefix when calling, just pass a "" in the test. |
|
(0034143) Edwin Jacques (reporter) 2013-10-17 16:08 |
Sorry, read your comments after my post. Is it OK to have some of the C++ dependencies in .d files and others in the ninja database? Would it be better to leave cmcldeps out, and add logic to cmake to write ninja build rules that explicitly run cl on the rc file the same way that cmcldeps, so it's not hidden? Then even the .rc deps would be in the ninja dependency database. IMHO, that seems more straightforward than cmcldeps, but I am not really a cmake or ninja expert. Please let me know if there is anything else I can do to help in this effort. For now, the new version of ninja is working well for me on Linux which is all I need for now. Knowing that this fix is in the pipeline will help managers with their decision-making. Thank you. --Edwin |
|
(0034146) Edwin Jacques (reporter) 2013-10-17 16:28 |
I updated the ninja diff in the comment to just pass in the "". Personally, I prefer that too. When making the initial patch, I was burned repeatedly by default arguments, so I removed them everywhere they got in my way. :) |
|
(0034150) Peter Kuemmel (developer) 2013-10-18 07:49 |
I've updated the cmake patch. Edwin, could you check it the MFC test now passes? |
|
(0034155) Peter Kuemmel (developer) 2013-10-18 10:12 |
Empty build of clang on Linux now takes 100ms instead of 160ms. |
|
(0034157) Edwin Jacques (reporter) 2013-10-18 11:02 |
Thanks! I just tried the MFC test and it failed. Looks like it needs a space after cmcldeps.exe. I thought I'd report the problem before i tried to fix it. 147: FAILED: "C:/JACQUE/cmake_syntheticpp_Retail/bin/cmcldeps.exeRC C:\JACQUE\cm ake_syntheticpp_Retail\Tests\MFC\mfcShared-prefix\src\mfcShared\mfc1.rc "CMakeFi les/mfc1.dir/mfc1.rc.res.d" CMakeFiles\mfc1.dir\mfc1.rc.res "Note: including fil e: " "C:/Program Files (x86)/Microsoft Visual Studio 11.0/VC/bin/cl.exe" C:\PROG RA~2\WI3CF2~1\8.0\bin\x86\rc.exe -D_AFXDLL /foCMakeFiles\mfc1.dir\mfc1.rc.res C:\JACQUE\cmake_syntheticpp_Retail\Tests\MFC\mfcShared-prefix\src\mfcShared\mfc1 .rcninja: fatal: ReadFile: The handle is invalid. |
|
(0034158) Peter Kuemmel (developer) 2013-10-18 11:22 |
Ah, yes, I touched the code after testing ;) Fixed it. |
|
(0034159) Edwin Jacques (reporter) 2013-10-18 11:49 |
That worked. I am running all of the tests now (takes 30m or so on my laptop). Will let you know how it goes. Thanks! |
|
(0034160) Edwin Jacques (reporter) 2013-10-18 12:31 |
Tests pass! I also downloaded and built the new ninja and its tests and that worked. Thanks for the help. May I suggest one more tweak? In the RC_COMPILER rule, if you add "deps = gcc", then it will add the .rc dependencies into the Ninja global dependency database. I tested this by changing the modified rules.ninja files in Tests\VSResource and running the ninja command. Ninja reads the .d files cmcldeps makes, puts it into the database and removes the file. It looks a bit odd, but it should be faster. What do you think? rule RC_COMPILER depfile = $DEP_FILE deps = gcc command = "C:/JACQUE/cmake_syntheticpp_Retail/bin/cmcldeps.exe" RC $in "$DEP_FILE" $out "Note: including file: " "C:/Program Files (x86)/Microsoft Visual Studio 11.0/VC/bin/cl.exe" C:\PROGRA~2\WI3CF2~1\8.0\bin\x86\rc.exe $FLAGS $DEFINES /fo$out $in description = Building RC object $out |
|
(0034161) Peter Kuemmel (developer) 2013-10-18 12:51 |
Yes, good idea. I assume also the the ninja keyword deps_pefix will change. |
|
(0034162) Edwin Jacques (reporter) 2013-10-18 14:14 |
I think this three char addition does it. I did a full rebuild and ran the MFC test. Works fine. It looks odd since the Windows RC build is using gcc dependency file format, but that's exactly what cmcldeps does, so it makes sense. I'm am re-running regression tests and can let you know when everything passes. diff --git a/Source/cmNinjaTargetGenerator.cxx b/Source/cmNinjaTargetGenerator.c xx index c1d061b..9a67a47 100644 --- a/Source/cmNinjaTargetGenerator.cxx +++ b/Source/cmNinjaTargetGenerator.cxx @@ -374,7 +374,7 @@ cmNinjaTargetGenerator { if (!mf->GetIsSourceFileTryCompile() && lang == "RC") { - deptype = ""; + deptype = "gcc"; depfile = "$DEP_FILE"; const std::string cl = mf->GetDefinition("CMAKE_C_COMPILER") ? mf->GetSafeDefinition("CMAKE_C_COMPILER") : |
|
(0034163) Edwin Jacques (reporter) 2013-10-18 14:48 |
All tests pass. |
|
(0034165) Edwin Jacques (reporter) 2013-10-18 16:37 |
I have two failed tests on SUSE Linux 11 SP 2. ctest -R complex . . . The following tests FAILED: 119 - complex (Failed) 120 - complexOneConfig (Failed) Errors while running CTest The error I see from both tests is similar: 120: /c4_working/cmake/cmake_synthenicpp/Tests/ComplexOneConfig/Executable/complex.cxx:65:3: error: #error Per-configuration directory-level definition not inherited. 119: /c4_working/cmake/cmake_synthenicpp/Tests/Complex/Executable/complex.cxx:65:3: error: #error Per-configuration directory-level definition not inherited. Is that just some other problem? If so, happen to know what ticket? |
|
(0034166) Peter Kuemmel (developer) 2013-10-18 16:58 |
OK, changed the ninja keyword to msvc_deps_prefix, and added deps = gcc for the RC rule. Do you know you can pass -j to ctest? It could speedup the tests a lot. When a test fails run ctest with -V; it shows you much more details. |
|
(0034167) Edwin Jacques (reporter) 2013-10-18 17:40 |
I did not know about -j, got my test time down to 1.5 minutes on Linux (my dev VM is way faster than my laptop, which now takes 7 minuts). Thanks! I have been using the -V a lot to see why the test is failing. All tests pass on Windows, just have those "complex" tests failing on Linux. I'll have to look to see if there's a bug filed for that yet. I look forward to these enhancements getting released! In the meantime, I will use my local build. The incremental build speed is great, and I love being able to use ninja to know exactly the dependencies of every source file quickly: c:\JACQUE\cmake_syntheticpp_Retail>ninja -t deps Source\CMakeFiles\CMakeLib.dir\ cmArchiveWrite.cxx.obj Source\CMakeFiles\CMakeLib.dir\cmArchiveWrite.cxx.obj: #deps 21, deps mtime 4038 22957 (VALID) ..\cmake_syntheticpp\Utilities\cm_libarchive.h ..\cmake_syntheticpp\Utilities\cmlibarchive\libarchive\archive.h ..\cmake_syntheticpp\Utilities\cmlibarchive\libarchive\archive_entry.h ..\cmake_syntheticpp\source\cmArchiveWrite.h ..\cmake_syntheticpp\source\cmStandardIncludes.h ..\cmake_syntheticpp\source\cmSystemTools.h Source\cmConfigure.h Source\cmsys\Configure.h Source\cmsys\Configure.hxx Source\cmsys\Directory.hxx Source\cmsys\IOStream.hxx Source\cmsys\Process.h Source\cmsys\String.hxx Source\cmsys\SystemTools.hxx Source\cmsys\ios\iosfwd Source\cmsys\ios\iostream Source\cmsys\stl\map Source\cmsys\stl\string Source\cmsys\stl\string.hxx Source\cmsys\stl\vector Utilities\cmThirdParty.h |
|
(0034168) Peter Kuemmel (developer) 2013-10-18 17:53 |
I successfully compiled reactos with tons of rc files and a 12MB build.ninja. I think the patch is quite complete. Now we only have wait for the merge of the ninja patch. Many thanks for your help! |
|
(0034170) Edwin Jacques (reporter) 2013-10-18 21:29 |
Glad to help. Hey, should I bother reporting the issue with the "complex" tests failing? I can't reproduce in the normal release branch for cmake. I also can't reproduce in master. |
|
(0034171) Peter Kuemmel (developer) 2013-10-19 03:40 |
Ninja patch was merged, so I merged to next: http://public.kitware.com/pipermail/cmake-commits/2013-October/016528.html [^] Lets wait and see what happens on the dash board: http://open.cdash.org/index.php?project=CMake [^] This will also show if the "complex" tests are a issue for us. |
|
(0034172) Edwin Jacques (reporter) 2013-10-19 12:57 |
I built the "next" branch on my Linux system and all regression tests pass! Now I should be able to get started leveraging cmake for my own builds. Thanks again! |
|
(0034173) Edwin Jacques (reporter) 2013-10-19 12:58 |
BTW, since it's in next, should we set the "target version" for this feature? |
|
(0034266) Peter Kuemmel (developer) 2013-10-28 04:23 |
Merged to next after some more fixes: http://cmake.org/gitweb?p=cmake.git;a=commit;h=eeb4aece1cf564c10d1c4e7409907ca6c92462ed [^] |
|
(0035297) Robert Maynard (manager) 2014-03-05 09:58 |
Closing resolved issues that have not been updated in more than 4 months |
| Notes |
| Issue History | |||
| Date Modified | Username | Field | Change |
| 2013-06-21 02:52 | mh | New Issue | |
| 2013-06-21 08:24 | Brad King | Assigned To | => Peter Kuemmel |
| 2013-06-21 08:24 | Brad King | Status | new => assigned |
| 2013-10-10 04:14 | Edwin Jacques | Note Added: 0034089 | |
| 2013-10-10 04:14 | Edwin Jacques | File Added: ninja_deps.patch | |
| 2013-10-10 04:18 | Edwin Jacques | Note Added: 0034090 | |
| 2013-10-10 04:48 | Edwin Jacques | File Added: ninja_deps_with_clang.patch | |
| 2013-10-10 04:49 | Edwin Jacques | Note Added: 0034091 | |
| 2013-10-10 13:46 | Bill Hoffman | Note Added: 0034099 | |
| 2013-10-11 09:40 | Edwin Jacques | Note Added: 0034103 | |
| 2013-10-12 07:01 | Peter Kuemmel | Note Added: 0034109 | |
| 2013-10-12 10:08 | Peter Kuemmel | Note Edited: 0034109 | |
| 2013-10-12 10:20 | Peter Kuemmel | Note Edited: 0034109 | |
| 2013-10-13 06:40 | Peter Kuemmel | Note Edited: 0034109 | |
| 2013-10-16 23:56 | Edwin Jacques | Note Added: 0034133 | |
| 2013-10-17 00:47 | Edwin Jacques | Note Added: 0034134 | |
| 2013-10-17 01:39 | Edwin Jacques | Note Added: 0034135 | |
| 2013-10-17 03:47 | Peter Kuemmel | Note Added: 0034136 | |
| 2013-10-17 11:30 | Edwin Jacques | Note Added: 0034139 | |
| 2013-10-17 12:04 | Peter Kuemmel | Note Added: 0034140 | |
| 2013-10-17 15:46 | Edwin Jacques | Note Added: 0034141 | |
| 2013-10-17 15:48 | Edwin Jacques | File Added: fix_Sln_test.diff | |
| 2013-10-17 16:02 | Peter Kuemmel | Note Added: 0034142 | |
| 2013-10-17 16:08 | Edwin Jacques | Note Added: 0034143 | |
| 2013-10-17 16:28 | Edwin Jacques | Note Added: 0034146 | |
| 2013-10-18 07:49 | Peter Kuemmel | Note Added: 0034150 | |
| 2013-10-18 10:12 | Peter Kuemmel | Note Added: 0034155 | |
| 2013-10-18 11:02 | Edwin Jacques | Note Added: 0034157 | |
| 2013-10-18 11:22 | Peter Kuemmel | Note Added: 0034158 | |
| 2013-10-18 11:49 | Edwin Jacques | Note Added: 0034159 | |
| 2013-10-18 12:31 | Edwin Jacques | Note Added: 0034160 | |
| 2013-10-18 12:51 | Peter Kuemmel | Note Added: 0034161 | |
| 2013-10-18 14:14 | Edwin Jacques | Note Added: 0034162 | |
| 2013-10-18 14:48 | Edwin Jacques | Note Added: 0034163 | |
| 2013-10-18 16:37 | Edwin Jacques | Note Added: 0034165 | |
| 2013-10-18 16:58 | Peter Kuemmel | Note Added: 0034166 | |
| 2013-10-18 17:40 | Edwin Jacques | Note Added: 0034167 | |
| 2013-10-18 17:53 | Peter Kuemmel | Note Added: 0034168 | |
| 2013-10-18 21:29 | Edwin Jacques | Note Added: 0034170 | |
| 2013-10-19 03:40 | Peter Kuemmel | Note Added: 0034171 | |
| 2013-10-19 12:57 | Edwin Jacques | Note Added: 0034172 | |
| 2013-10-19 12:58 | Edwin Jacques | Note Added: 0034173 | |
| 2013-10-21 09:04 | Robert Maynard | Target Version | => CMake 3.0 |
| 2013-10-28 04:23 | Peter Kuemmel | Note Added: 0034266 | |
| 2013-10-28 04:23 | Peter Kuemmel | Status | assigned => resolved |
| 2013-10-28 04:23 | Peter Kuemmel | Resolution | open => fixed |
| 2014-03-05 09:58 | Robert Maynard | Note Added: 0035297 | |
| 2014-03-05 09:58 | Robert Maynard | Status | resolved => closed |
| Issue History |
| Copyright © 2000 - 2018 MantisBT Team |