|
Notes |
|
|
(0037245)
|
|
Brad King
|
|
2014-11-21 09:43
|
|
|
FWIW, CMake never intended to support wrong-case specification of file names. Such projects would not work on case-sensitive filesystems. Still, this looks like a regression since it worked in 3.0. If we are going to disallow this, it would need to be done with a policy. |
|
|
|
(0037249)
|
|
Ben Boeckel
|
|
2014-11-21 12:57
|
|
Unable to reproduce on OS X. Testing on Windows bisects down to:
commit d38423ecc4105d8339b7461130b964f3c69e8847
Author: Stephen Kelly <steveire@gmail.com>
Date: Mon Mar 17 17:49:38 2014 +0100
cmTarget: Add a method to obtain list of filenames for sources.
as being the culprit. |
|
|
|
(0037250)
|
|
Ben Boeckel
|
|
2014-11-21 13:05
|
|
|
A simple revert doesn't work and isn't simple. Source files now depend on the configuration, so I'm going to try avoiding the std::string function as a fix. |
|
|
|
(0037251)
|
|
Ben Boeckel
|
|
2014-11-21 13:20
|
|
Looks like the bad commit was partially reverted at:
commit 92e2fbe103c6ffc3af1189d0450493488c65baaa
Author: Stephen Kelly <steveire@gmail.com>
Date: Sat Apr 5 11:41:58 2014 +0200
cmGeneratorTarget: Trace cmSourceFile objects instead of strings.
This reverses the decision in commit d38423ec (cmTarget: Add a
method to obtain list of filenames for sources., 2014-03-17). The
cmSourceFile based API is preferred because that avoids creation of
many cmSourceFileLocation objects for matching strings, and the
result is cached by cmTarget.
but the bad behavior stuck around. Inspecting history around this commit. |
|
|
|
(0037252)
|
|
Ben Boeckel
|
|
2014-11-21 14:01
|
|
So one comparing the reverse diff of the original breaking commit and the partial revert shows this:
# Reverse of d38423ecc4105d8339b7461130b964f3c69e8847
// Make sure this file is in the target.
- this->Target->AddSource(name);
+ this->Target->AddSourceFile(sf);
# From 92e2fbe103c6ffc3af1189d0450493488c65baaa
// Make sure this file is in the target.
- this->Target->AddSource(name);
+ this->Target->AddSource(sf->GetFullPath());
Which has since been changed by:
commit 4f1c71fdd25f33bd0cdeb2b705723db02f8fddf4
Author: Stephen Kelly <steveire@gmail.com>
Date: Wed Apr 9 01:32:14 2014 +0200
cmTarget: Add all sources traced from custom commands at once.
The AddSource method accepts one file and tries to avoiding adding
it to the sources-list of the target if it already exists. This
involves creating many cmSourceFileLocation objects for matching
on existing files, which is an expensive operation.
Avoid the searching algorithm by appending the new sources as one
group. Generate-time processing of source files will ensure
uniqueness.
Add a new AddTracedSources for this purpose. The existing
AddSources method must process the input for policy CMP0049, but
as these source filenames come from cmSourceFile::GetFullPath(),
we can forego that extra processing.
into:
- // Make sure this file is in the target.
- this->Target->AddSource(sf->GetFullPath());
+ // Make sure this file is in the target at the end.
+ this->NewSources.push_back(sf->GetFullPath());
and now calls cmTarget::AddTracedSources. It seems that AddSourceFile has been removed because now source files are tracked by strings to accommodate generator expressions properly. There might be an easier fix in getting cmTarget::GetSourceFiles(std::vector<cmSourceFile*>) to not call down to the std::vector<std::string> version, but there's a lot of code in there I'm not intimately familiar with. I admit that this whole trail might be only related and not the actual problem though, but it is certainly the most promising I can get from it. |
|
|
|
(0037254)
|
|
Stephen Kelly
|
|
2014-11-22 05:48
|
|
Getting back to
commit d38423ecc4105d8339b7461130b964f3c69e8847
Author: Stephen Kelly <steveire@gmail.com>
Date: Mon Mar 17 17:49:38 2014 +0100
cmTarget: Add a method to obtain list of filenames for sources.
What is it about that commit which causes the test to fail? Is there some case handling in the old code path which is not in the new code path? |
|
|
|
(0037255)
|
|
Ben Boeckel
|
|
2014-11-22 11:09
|
|
|
My best guess is the difference between AddSource(fname) and AddSourceFile(sf) because we don't do case-insensitive name comparisons…for the path components on case-insensitive file systems (e.g., I don't know how things work with NTFS-on-Linux). I can try and break down that commit on Monday. |
|
|
|
(0037269)
|
|
Ben Boeckel
|
|
2014-11-24 14:37
|
|
|
I was unable to get it to work by reverting hunks from that patch by anything short of reverting the entire patch. This includes using cmSourceFile for the API and using GetFullPath for string-y parts or using cmMakefile::GetSource to get a cmSourceFile* from a string. I tried making the queues have cmSourceFile* which using strings elsewhere, using cmTarget::AddSourceFile while using strings elsewhere, and turning strings back into cmSourceFile* for use around the cmTarget APIs. |
|
|
|
(0037270)
|
|
Ben Boeckel
|
|
2014-11-24 14:48
|
|
If we can't get this fixed, my proposal for a policy (which won't avoid a backwards-incompat break, but at least makes noises about the issue):
Add a policy which warns when source file properties are set on files which are not used during the build. This should catch improper logic for source files which have properties but aren't part of a target (probably a logic error) and this case.
This should warn in the OLD case and error in the NEW case; no behavior change for either side. The open question I have is whether to do this *just* for CMake's built-in properties or also do so for user properties (though a query on it should prevent the warning). The downside would be that CMake would have to store a cmBacktrace for each source file property to indicate where it was set. |
|
|
|
(0037271)
|
|
Stephen Kelly
|
|
2014-11-25 08:02
|
|
It should be possible to fix this by finding out how the old codepath accepted the code. Somewhere in CMake there must be a case-insensitive comparison, or use of some WIN32 filesystem API, or some point where the wrong-case name is converted to a correct-case name. I don't have the Windows access to track this down. If we find that, we might be able to find the right place to do a similar conversion for a policy implementation.
In the old codepath, the sources of a target are stored as cmSourceFile*s early during configure-time, and in the new codepath they are stored as strings and converted to cmSourceFile* at generate-time. The API for getting the cmSourceFile from the string should be the same (cmMakefile::GetOrCreateSource), so I don't really see why the behavior is any different. Maybe the cmSourceFile created while executing set_source_files_properties is not found during generate time by GetOrCreateSource? |
|
|
|
(0037281)
|
|
Ben Boeckel
|
|
2014-11-25 14:23
|
|
So with this setup:
on disk:
- NeedDef.c
name give to set_source_files_properties:
- needdef.c
name given to the target:
- needdef.c
it worked in 3.0. With master, however, the *generate* step now asks for a cmSourceFile by its full, on-disk cased, path (this is probably why OS X works: we don't find the proper case there) and doesn't find the one with the properties set on it because cmSourceFileLocation::Matches uses the user-given casing for the file when searching. This is also why if the property name and the target name didn't match in case, it failed to work even in 3.0.
Now bisecting with this new test case. |
|
|
|
(0037282)
|
|
Stephen Kelly
|
|
2014-11-25 14:42
|
|
Ah, so being a full path is relevant?
So the CMake 3.0 and earlier behavior with
SET(FileWarningAsError warningaserror.cpp)
SET(Files main.cpp ${FileWarningAsError})
SET_SOURCE_FILES_PROPERTIES(${FileWarningAsError} PROPERTIES COMPILE_FLAGS "/WX")
ADD_EXECUTABLE(CaseInsensitive ${Files})
is different to
SET(FileWarningAsError ${CMAKE_CURRENT_SOURCE_DIR}/warningaserror.cpp)
SET(Files main.cpp ${FileWarningAsError})
SET_SOURCE_FILES_PROPERTIES(${FileWarningAsError} PROPERTIES COMPILE_FLAGS "/WX")
ADD_EXECUTABLE(CaseInsensitive ${Files})
? |
|
|
|
(0037283)
|
|
Ben Boeckel
|
|
2014-11-25 15:15
|
|
So that new test bisects down to the same commit :/ . The problem seems to be that the cmSourceFileLocation stores the user-specified casing, not the actual casing on-disk and previously, using cmSourceFile would boil down to asking about the on-disk casing via GetFullPath whereas now with the strings, we create sources according to the user-specified casing, but then query on the actual casing.
Testing with a full path given in the CMakeLists.txt doesn't change anything about the results. |
|
|
|
(0037284)
|
|
Ben Boeckel
|
|
2014-11-25 15:53
|
|
Here is a diff which fixes the original broken commit (d38423ecc4105d8339b7461130b964f3c69e8847). It no longer cleanly applies (since cmTarget::GetSourceFiles has been rewritten):
========
diff --git a/Source/cmGeneratorTarget.cxx b/Source/cmGeneratorTarget.cxx
index 834f9fd..0296877 100644
--- a/Source/cmGeneratorTarget.cxx
+++ b/Source/cmGeneratorTarget.cxx
@@ -710,7 +710,7 @@ void cmTargetTraceDependencies::FollowName(std::string const& name)
{
this->CurrentEntry->Depends.push_back(sf);
}
- this->QueueSource(sf->GetFullPath());
+ this->QueueSource(name);
}
}
diff --git a/Source/cmTarget.cxx b/Source/cmTarget.cxx
index 0866d10..5e14fe0 100644
--- a/Source/cmTarget.cxx
+++ b/Source/cmTarget.cxx
@@ -551,7 +551,7 @@ void cmTarget::GetSourceFiles(std::vector<std::string> &files) const
si = sourceFiles.begin();
si != sourceFiles.end(); ++si)
{
- files.push_back((*si)->GetFullPath());
+ files.push_back((*si)->GetLocation().GetName());
}
}
========
I expect Mantis to screw up the formatting, but I don't see a way to attach a file? For anyone who sees this before it expires:
http://paste.fedoraproject.org/154051/48802141/ [^] |
|
|
|
(0037302)
|
|
Brad King
|
|
2014-11-26 14:24
|
|
As stated in 0015259:0037245, using the wrong case for sources was never intended to work or documented. Now that we've dug into this, it appears to have worked only due to a subtle idiosyncrasy of the old implementation. After the refactoring needed to enable the target SOURCES features in 3.1 it does not appear to be possible to reproduce the old behavior easily. I don't want to add a bunch of complicated/subtle logic to the new implementation that may introduce other bugs just to support this old bug.
It is easy for projects to fix their CMake code to use the correct case when naming sources, and the fixed code will still work just as well in older CMake versions.
IMO the cost of trying to support this old bug with compatibility code in CMake is too high to be worthwhile compared to just fixing the individual projects with such typos in their code. |
|
|
|
(0037303)
|
|
Brad King
|
|
2014-11-26 14:43
|
|
|
|
|
(0037304)
|
|
Brad King
|
|
2014-11-26 16:12
|
|
Re 0015259:0037302: On second thought, it may be possible to make this continue to work. Although the code paths need to do lookups that they didn't do before, we can make those lookups work despite case differences.
We already normalize the case of the directory component of file locations. Now we just need to compare the names in a case-insensitive way when working on operating systems using case-insensitive filesystems:
diff --git a/Source/cmSourceFileLocation.cxx b/Source/cmSourceFileLocation.cxx
index 1c2454e..004fd1f 100644
--- a/Source/cmSourceFileLocation.cxx
+++ b/Source/cmSourceFileLocation.cxx
@@ -216,7 +216,8 @@ bool cmSourceFileLocation::Matches(cmSourceFileLocation const& loc)
// Both extensions are similarly ambiguous. Since only the old fixed set
// of extensions will be tried, the names must match at this point to be
// the same file.
- if(this->Name.size() != loc.Name.size() || this->Name != loc.Name)
+ if(this->Name.size() != loc.Name.size() ||
+ !cmSystemTools::ComparePath(this->Name, loc.Name))
{
return false;
}
|
|
|
|
(0037305)
|
|
Brad King
|
|
2014-11-26 17:11
|
|
|
|
|
(0037322)
|
|
Brad King
|
|
2014-12-01 09:13
|
|
I've queued the fix for inclusion in the next 3.1 release candidate.
|
|
|
|
(0038680)
|
|
Robert Maynard
|
|
2015-05-04 09:05
|
|
|
Closing resolved issues that have not been updated in more than 4 months. |
|
DummyProject.7z (760) 2014-11-21 08:10