PCH: cmLocalGenerator::AddPchDependencies should not unset the GENERATED property
Introduction
During the implementation of !5308 (merged) (which modifies the GENERATED
source-file property in such a way that it is visible from any directory scope) a problem occurred with cmLocalGenerator.cxx::AddPchDependencies
. It unsets the GENERATED
property of the generated PCH-header, however, unsetting that property will no longer be allowed once the feature from !5308 (merged) lands.
The discussion around that problem is captured by this comment in the merge-request. However, for the sake of completeness I am showing an extended analysis here:
Problem analysis
For simpler explanation I am duplicating the problematic code here:
Source/cmLocalGenerator.cxx
from f6e7d5f3a00eee04834840a9534d19445fd3ab8f:
2607 // Add pchHeader to source files, which will
2608 // be grouped as "Precompile Header File"
2609 auto pchHeader_sf = this->Makefile->GetOrCreateSource(
2610 pchHeader, true, cmSourceFileLocationKind::Known);
2611 std::string err;
2612 pchHeader_sf->ResolveFullPath(&err);
2613
2614 // The pch file is generated, but mark it as not generated
2615 // so that a clean operation will not remove it from disk
2616 pchHeader_sf->SetProperty("GENERATED", "0");
Source/cmSourceFile.cxx
from c7b50349de5e56d7584fa644832170d1e08ef62e
113 bool cmSourceFile::FindFullPath(std::string* error,
114 std::string* cmp0115Warning)
115 {
116 // If the file is generated compute the location without checking on disk.
117 if (this->GetIsGenerated()) {
118 // The file is either already a full path or is relative to the
119 // build directory for the target.
120 this->Location.DirectoryUseBinary();
121 this->FullPath = this->Location.GetFullPath();
122 return true;
123 }
As already mentioned in the introduction above, cmLocalGenerator.cxx::AddPchDependencies
is unsetting the GENERATED
property (in line 2616 of cmLocalGenerator.cxx
). According to the source-comment the intention is to prevent automatic removal of the generated PCH-header with a make clean
(or equivalent) operation.
If that really works or if the PCH-header would otherwise be automatically removed by such a clean operation I cannot really tell.
I was unable to find the code that determines what files to remove by a clean operation and which not. However, I noticed that removing line 2616 of cmLocalGenerator.cxx
made the RunCMake.FileAPI
test fail with the following output:
Unexpected index:
Traceback (most recent call last):
File ".../cmake-source/Tests/RunCMake/FileAPI/codemodel-v2-check.py", line 789, in <module>
check_objects(index["objects"], index["cmake"]["generator"])
File ".../cmake-source/Tests/RunCMake/FileAPI/codemodel-v2-check.py", line 15, in check_objects
check_index_object(o[0], "codemodel", 2, 2, check_object_codemodel(g))
File ".../cmake-source/Tests/RunCMake/FileAPI/check_index.py", line 141, in check_index_object
check(object)
File ".../cmake-source/Tests/RunCMake/FileAPI/codemodel-v2-check.py", line 783, in _check
check_object_codemodel_configuration(c, g, inSource)
File ".../cmake-sourceTests/RunCMake/FileAPI/codemodel-v2-check.py", line 762, in check_object_codemodel_configuration
check_targets(c, g, inSource)
File ".../cmake-source/Tests/RunCMake/FileAPI/codemodel-v2-check.py", line 716, in check_targets
check_list_match(lambda a, e: matches(a["id"], e["id"]),
File ".../cmake-source/Tests/RunCMake/FileAPI/check_index.py", line 57, in check_list_match
check(actual_item, expected_item)
File ".../cmake-source/Tests/RunCMake/FileAPI/codemodel-v2-check.py", line 180, in _check
check_list_match(lambda a, e: matches(a["path"], e["path"]), obj["sources"],
File ".../cmake-source/Tests/RunCMake/FileAPI/check_index.py", line 57, in check_list_match
check(actual_item, expected_item)
File ".../cmake-source/Tests/RunCMake/FileAPI/codemodel-v2-check.py", line 178, in check_source
assert sorted(actual.keys()) == sorted(expected_keys)
AssertionError: ('Source file: .../cmake-build/Tests/RunCMake/FileAPI/codemodel-v2-build/cxx/CMakeFiles/cxx_exe.dir/Debug/cmake_pch.hxx', 'Target ID: cxx_exe::@a56b12a3f5c0529fb296')
That AssertionError
in the last line of the output indicates that something changed with the generated PCH-header. However, it was at least not missing.
Ignoring for a moment that this code definitively has to change for supporting the changes that will be introduced by !5308 (merged), I am not entirely sure that code is currently correct in all situations:
The call to GetOrCreateSource( ..., true, cmSourceFileLocationKind::Known)
in lines 2609 and 2610 of cmLocalGenerator.cxx
assumes that the source file will be created and therefore the GENERATED
property will be automatically set to 1
.
That then guarantees that the call to cmSourceFile::ResolveFullPath(...)
in line 2612 which itself calls cmSourceFile::FindFullPath(...)
will always succeed and fully resolve the path: the check in line 117 of cmSourceFile.cxx
will then evaluate to true
(because the GENERATED
property is set to 1
) and the above shown branch will be taken... and it always succeeds.
And because that was the only reason to set the GENERATED
property, it will be unset in line 2616 of cmLocalGenerator.cxx
again.
However, I cannot see that the original assumption holds.
It could be that GetOrCreateSource(...)
already knows about that source file and therefore will not create it. If that source file was originally created without setting the GENERATED
property, the check in line 117 of cmSourcefile.cxx
will evaluate to false
and instead the long and complicated branch (which I did not show here) will be taken which might possibly not succeed in resolving the full path or even worse, would evaluate to the wrong full path.
Solution ideas
After having looked in detail at the complicated logic of cmSourceFileLocation
and the complicated else
-branch of cmSourceFile::FindFullPath
I suggest to just replace the cmLocalGenerator::GetOrCreateSource
call in line 2609 and 2610 of cmLocalGenerator.cxx
by a call to cmLocalGenerator::GetOrCreateGeneratedSource
. The latter guarantees that the GENERATED
property will be set, even if the source file had already been created. (This will guarantee that the if
-branch in line 117 of cmSourceFile.cxx
will be taken. And that should probably be logically correct, because the PCH-header is always generated.)
This prevents the last problem I described in the former section.
However, this still does not help with the first described problem, the unsetting of the GENERATED
property. That will still no longer be allowed once !5308 (merged) is merged.
Currently, the best solution I see is to just remove the lines 2616 to 2616 and call it a day. (Of course, the RunCMake.FileAPI
test would still need fixing.)
But that would probably prevent the original intention expressed in the source-comment, that is, preventing automatic removal of the PCH-header during cleanup operations.
But if that would not be a problem, there is even a simpler solution to fix all problems at once: Just never setting or unsetting the GENERATED
property.
(This however will take the "long and complicated" path within cmSourceFile::FindFullPath
. I am still not convinced that that will always succeed, but trying it out passed all the tests CMake currently has; even the RunCMake.FileAPI
test.)
Conclusion
We ultimately have the following options to fix all problems (in order of preference):
If the automatic removal-prevention during cleanup operations did never work or is not important then:
-
Either never set the
GENERATED
property, -
or always set the
GENERATED
property but never unset it (and fix/modify theRunCMake.FileAPI
test). -
Otherwise, use approach 2 but implement some extra (and complicated) mechanism which instructs the different generators to not remove that specific file although its
GENERATED
property is set. (Maybe by implementing some additional internal property__CMAKE_NEVER_CLEAN
similar to the__CMAKE_GENERATED_BY_CMAKE
property?)
As the code in question was originally introduced in !4792 (merged) by @cristianadam, maybe he can help us understand the intention and what impact unsetting the GENERATED
property really had.