View Issue Details [ Jump to Notes ] [ Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0015259CMakeCMakepublic2014-11-21 08:102015-05-04 09:05
ReporterAlexander Markert 
Assigned ToBrad King 
PrioritynormalSeverityminorReproducibilityalways
StatusclosedResolutionfixed 
Platformx86_64OSWindowsOS VersionWindows 7
Product Version 
Target VersionCMake 3.1Fixed in VersionCMake 3.1 
Summary0015259: Source file properties are lost, if the name of the file is not specified case sensitive.
DescriptionIf you add a source file and the file name is not specified case sensitive, then CMake corrects the name - that is ok.

But if you specify additional properties for the file via SET_SOURCE_FILES_PROPERTIES, the settings are lost!

This is only true for CMake 3.1.0, CMake 3.0.2 worked fine.
Steps To ReproduceAdd a source file with a name, which is not case sensitive and set an additional propery via SET_SOURCE_FILES_PROPERTIES.

Example of CMakeLists.txt:
# Name is actually WarningsAsError.cpp, but warningaserror.cpp is used
SET(FileWarningAsError warningaserror.cpp)
SET(Files main.cpp ${FileWarningAsError})
# option /WX is lost
SET_SOURCE_FILES_PROPERTIES(${FileWarningAsError} PROPERTIES COMPILE_FLAGS "/WX")
ADD_EXECUTABLE(CaseInsensitive ${Files})
Additional InformationPlease find attached a configuration file and a batch file, which generated the given projects with CMake 3.1.0 and CMake 3.0.2.
The paths to CMake have to be adjusted in CallCMake.bat.
TagsNo tags attached.
Attached Files7z file icon DummyProject.7z [^] (760 bytes) 2014-11-21 08:10

 Relationships

  Notes
(0037245)
Brad King (manager)
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 (developer)
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 (developer)
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 (developer)
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 (developer)
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 (developer)
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 (developer)
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 (developer)
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 (developer)
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 (developer)
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 (developer)
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 (developer)
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 (developer)
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 (developer)
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 (manager)
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 (manager)
2014-11-26 14:43

 I've updated the 3.1 release notes to explicitly mention that projects depending on the old behavior should be fixed:

 Help: Add 3.1 release note about source file name casing
 http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=ea1afd84 [^]
(0037304)
Brad King (manager)
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 (manager)
2014-11-26 17:11

 Re 0015259:0037304: I've committed the patch here:

 Fix lookup of source names after conversion to their actual case
 http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=84d124e8 [^]

and merged for testing.
(0037322)
Brad King (manager)
2014-12-01 09:13

 I've queued the fix for inclusion in the next 3.1 release candidate.
(0038680)
Robert Maynard (manager)
2015-05-04 09:05

 Closing resolved issues that have not been updated in more than 4 months.

 Issue History
Date Modified Username Field Change
2014-11-21 08:10 Alexander Markert New Issue
2014-11-21 08:10 Alexander Markert File Added: DummyProject.7z
2014-11-21 09:35 Brad King Assigned To => Ben Boeckel
2014-11-21 09:35 Brad King Status new => assigned
2014-11-21 09:35 Brad King Target Version => CMake 3.1
2014-11-21 09:43 Brad King Note Added: 0037245
2014-11-21 12:57 Ben Boeckel Assigned To Ben Boeckel => Stephen Kelly
2014-11-21 12:57 Ben Boeckel Note Added: 0037249
2014-11-21 13:05 Ben Boeckel Note Added: 0037250
2014-11-21 13:05 Ben Boeckel Status assigned => acknowledged
2014-11-21 13:20 Ben Boeckel Note Added: 0037251
2014-11-21 14:01 Ben Boeckel Note Added: 0037252
2014-11-22 05:48 Stephen Kelly Note Added: 0037254
2014-11-22 11:09 Ben Boeckel Note Added: 0037255
2014-11-24 14:37 Ben Boeckel Note Added: 0037269
2014-11-24 14:48 Ben Boeckel Note Added: 0037270
2014-11-25 08:02 Stephen Kelly Note Added: 0037271
2014-11-25 14:23 Ben Boeckel Note Added: 0037281
2014-11-25 14:42 Stephen Kelly Note Added: 0037282
2014-11-25 15:15 Ben Boeckel Note Added: 0037283
2014-11-25 15:53 Ben Boeckel Note Added: 0037284
2014-11-26 14:24 Brad King Note Added: 0037302
2014-11-26 14:24 Brad King Assigned To Stephen Kelly =>
2014-11-26 14:24 Brad King Status acknowledged => resolved
2014-11-26 14:24 Brad King Resolution open => won't fix
2014-11-26 14:24 Brad King Target Version CMake 3.1 =>
2014-11-26 14:43 Brad King Note Added: 0037303
2014-11-26 16:09 Brad King Status resolved => acknowledged
2014-11-26 16:09 Brad King Resolution won't fix => open
2014-11-26 16:12 Brad King Note Added: 0037304
2014-11-26 16:13 Brad King Target Version => CMake 3.1
2014-11-26 17:11 Brad King Note Added: 0037305
2014-11-26 17:18 Brad King Note View State: 0037304: private
2014-11-26 17:18 Brad King Note View State: 0037304: public
2014-12-01 09:13 Brad King Note Added: 0037322
2014-12-01 09:13 Brad King Assigned To => Brad King
2014-12-01 09:13 Brad King Status acknowledged => resolved
2014-12-01 09:13 Brad King Resolution open => fixed
2014-12-01 09:13 Brad King Fixed in Version => CMake 3.1
2015-05-04 09:05 Robert Maynard Note Added: 0038680
2015-05-04 09:05 Robert Maynard Status resolved => closed

Copyright © 2000 - 2018 MantisBT Team