|
Notes |
|
|
(0016770)
|
|
Alex Neundorf
|
2009-06-28 16:45
(edited on: 2009-10-08 11:45) |
|
The attached patch implements the changed behaviour, together with a new policy to control the behaviour.
Old behaviour:
set(foo bar)
message(STATUS "foo (1): ${foo}")
set(foo blub CACHE STRING "some variable")
message(STATUS "foo (2): ${foo}")
Running cmake the first time gives:
foo (1): bar
foo (2): blub
Running cmake the second time gives:
foo (1): bar
foo (2): bar
With the new behaviour it gives:
Running cmake the first time gives:
foo (1): bar
foo (2): blub
Running cmake the second time gives:
foo (1): bar
foo (2): blub
which is IMO easier to understand and more consistent. If the value of foo was modified in the cache, e.g. to "dumdidum", foo (2) will show "dumdidum", not "blub" (while with the OLD behaviour it would still be "bar").
|
|
|
|
(0017322)
|
|
Alex Neundorf
|
|
2009-09-06 04:46
|
|
I uploaded an updated version of the patch, the policy is now 0015.
A test is still missing. I guess I would need a run-cmake-twice test...
Alex |
|
|
|
(0017324)
|
|
Alex Neundorf
|
|
2009-09-06 09:09
|
|
The attached file cmSetCommand.h.documentation.patch documents the behaviour in more detail.
Alex |
|
|
|
(0017326)
|
|
Brad King
|
|
2009-09-07 08:21
|
|
|
Ugh, I had forgotten about this patch and was unaware of the issue tracker entry until now. I'm interested in the patch. I've committed it on a local branch in my git repo and will test it on some real projects when I get a chance. |
|
|
|
(0017327)
|
|
Brad King
|
|
2009-09-07 09:53
|
|
I like the change now that I see it in action. The new behavior makes sense. It turns set(CACHE) into direct interaction with the user. The policy warning won't show up except in cases that would have had surprising behavior before.
However, the patch needs some work.
1.) The documentation should refer to CMake "2.8.0", not "2.7.0". When the patch is finally ready for commit, the date of policy addition (2,7,20090906) should be changed to the day of commit (or the day after commit to be really safe).
2.) The policy check should be centralized in cmMakefile::AddCacheDefinition. The short-circuit code in cmSetCommand and cmOptionCommand should be removed so that it falls through to the central place.
3.) The policy implementation for WARN should fall through to OLD, not NEW.
The OLD behavior should be preserved unless the policy is set to NEW.
4.) The policy implementation should handle the two REQUIRE cases. Although
they won't be used yet, it is easier to implement and test them now while
you understand the change in behavior.
Thanks! |
|
|
|
(0017371)
|
|
Alex Neundorf
|
|
2009-09-10 14:21
|
|
Not sure about REQUIRE_IF_USED and REQUIRE_ALWAYS:
does it mean that with REQUIRE_IF_USED it should print a useful error message if that case is hit and error out, and with REQUIRE_ALWAYS it should just always error out if the policy is not set to NEW, independent whether the policy would change the behaviour ?
http://www.cmake.org/Wiki/CMake_2.6_Notes [^]
After looking at the code again now I remember why I did it that way, instead of centrally in AddCacheDefinition(): the way the code in cmSetCommand()/cmOptionCommand() and AddCacheDefinition() works together is not too straight forward, so I chose the way which seemed least intrusive to me.
Alex |
|
|
|
(0017372)
|
|
Brad King
|
|
2009-09-10 14:39
|
|
By the time REQUIRE_ALWAYS is used then the policy implementation is removed too (the entire switch statement). Any attempt to set the policy to OLD (or unset it) will error out in the cmPolicies class. The convention we've been using is just to make REQUIRE_IF_USED and REQUIRE_ALWAYS be the same case (no break) so that compilers do not warn about not all enum values being handled by a switch.
I'd like to clean up the interaction of AddCacheDefinition and its callers, so I'll take care of this after all. |
|
|
|
(0017373)
|
|
Brad King
|
|
2009-09-10 17:00
|
|
Create CMake Policy CMP0015 to fix set(CACHE)
/cvsroot/CMake/CMake/Source/cmMakefile.cxx,v <-- Source/cmMakefile.cxx
new revision: 1.519; previous revision: 1.518
/cvsroot/CMake/CMake/Source/cmMakefile.h,v <-- Source/cmMakefile.h
new revision: 1.259; previous revision: 1.258
/cvsroot/CMake/CMake/Source/cmOptionCommand.cxx,v <-- Source/cmOptionCommand.cxx
new revision: 1.25; previous revision: 1.24
/cvsroot/CMake/CMake/Source/cmPolicies.cxx,v <-- Source/cmPolicies.cxx
new revision: 1.43; previous revision: 1.42
/cvsroot/CMake/CMake/Source/cmPolicies.h,v <-- Source/cmPolicies.h
new revision: 1.26; previous revision: 1.25
/cvsroot/CMake/CMake/Source/cmSetCommand.cxx,v <-- Source/cmSetCommand.cxx
new revision: 1.36; previous revision: 1.35
/cvsroot/CMake/CMake/Source/cmSetCommand.h,v <-- Source/cmSetCommand.h
new revision: 1.23; previous revision: 1.22 |
|
|
|
(0018029)
|
|
Brad King
|
|
2009-10-08 11:34
|
|
Unfortunately policy CMP0015's NEW behavior breaks a valid use case.
# CMakeLists.txt
option(BUILD_SHARED_LIBS "..." ON)
add_library(mylib ...)
set(BUILD_SHARED_LIBS OFF) # we want only mylib to be shared
add_subdirectory(ThirdParty)
# ThirdParty/CMakeLists.txt
option(BUILD_SHARED_LIBS "..." ON)
# uh, oh, with NEW behavior this dir uses shared libs!!!
For reference, this is the discussion that led to the erasure of local variables in the first place:
http://www.cmake.org/pipermail/cmake/2007-March/013204.html [^]
This was the commit:
Author: Ken Martin <ken.martin@kitware.com>
Date: Thu Mar 15 17:48:15 2007 +0000
BUG: change in how set cache overrides local definitions. Should mainly be a NOP change for most cases
diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx
index 1679568..78b44a4 100644
--- a/Source/cmMakefile.cxx
+++ b/Source/cmMakefile.cxx
@@ -1241,7 +1241,8 @@ void cmMakefile::AddCacheDefinition(const char* name, const char* value,
}
this->GetCacheManager()->AddCacheEntry(name, val, doc, type);
- this->AddDefinition(name, val);
+ // if there was a definition then remove it
+ this->Definitions.erase( DefinitionMap::key_type(name));
} |
|
|
|
(0018030)
|
|
Brad King
|
|
2009-10-08 11:44
|
|
From the current CMP0015 documentation, describing OLD behavior:
the code
set(x 1)
set(before ${x})
set(x 2 CACHE STRING "X")
set(after ${x})
message(STATUS "${before},${after}")
would print "1,2" on the first run and "1,1" on future runs.
I think a better fix is to make the NEW behavior separate local and cache settings completely. Essentially it would remove the .erase() line added by the above patch. This would:
(1) Fix inconsistency in the before/after example.
It would print "1,1" on the first run and all future runs.
(2) Address the original use case from the 2007 mailing list thread.
Since no local variable would ever be set the cache would always be used.
(3) Allow the BUILD_SHARED_LIBS use case in the previous note.
The result in the example would be the same for OLD and NEW behavior.
We could implement this new policy CMP0015 by adding a check in cmMakefile::AddCacheDefinition. If the value just stored in the cache (first run) is different from the value of the local variable it would erase in OLD behavior, then trigger the policy. OLD behavior is to proceed with the erasure of the local. NEW behavior is to leave the local untouched. |
|
|
|
(0018046)
|
|
Brad King
|
|
2009-10-08 14:58
|
|
Since the new version of the policy I propose changes AddCacheDefinition in stead of just the set and option commands, it affects all calls to it. Implementing the change will require careful thought about whether each call should make the local or cache value visible.
For now it is best to remove the policy altogether and leave this issue open. That way when we do a better fix it will be fewer changes in behavior for users.
Remove CMake Policy CMP0015 until it is revised
/cvsroot/CMake/CMake/Source/cmMakefile.cxx,v <-- Source/cmMakefile.cxx
new revision: 1.528; previous revision: 1.527
/cvsroot/CMake/CMake/Source/cmMakefile.h,v <-- Source/cmMakefile.h
new revision: 1.263; previous revision: 1.262
/cvsroot/CMake/CMake/Source/cmOptionCommand.cxx,v <-- Source/cmOptionCommand.cxx
new revision: 1.28; previous revision: 1.27
/cvsroot/CMake/CMake/Source/cmPolicies.cxx,v <-- Source/cmPolicies.cxx
new revision: 1.49; previous revision: 1.48
/cvsroot/CMake/CMake/Source/cmPolicies.h,v <-- Source/cmPolicies.h
new revision: 1.28; previous revision: 1.27
/cvsroot/CMake/CMake/Source/cmSetCommand.cxx,v <-- Source/cmSetCommand.cxx
new revision: 1.39; previous revision: 1.38
/cvsroot/CMake/CMake/Source/cmSetCommand.h,v <-- Source/cmSetCommand.h
new revision: 1.25; previous revision: 1.24 |
|
|
|
(0019695)
|
|
Alex Neundorf
|
|
2010-03-03 12:57
|
|
I attached a CMakeLists.txt which shows another unexpected behaviour.
What it does is the following: it sets a variable FOO to "foo", then marks it
as advanced (although it is not in the cache), then does a find_program() on
the variable, and after that find_program() FOO is suddenly empty.
I.e. the output is:
...
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- FOO = "foo"
-- FOO = "foo"
-- FOO = "foo"
-- FOO = ""
-- Configuring done
I tested this with current HEAD (or master) and CMake 2.6.2. Both give the
same result.
FOO is then also in the cache with value "".
What happens is this:
first set() sets the value, but not in the cache.
Then there comes the mark_as_advanced(), which creates a cache entry, but with
empty value:
bool cmMarkAsAdvancedCommand
::InitialPass(std::vector<std::string> const& args, cmExecutionStatus &)
{
...
for(; i < args.size(); ++i)
{
std::string variable = args[i];
cmCacheManager* manager = this->Makefile->GetCacheManager();
cmCacheManager::CacheIterator it =
manager->GetCacheIterator(variable.c_str());
if ( it.IsAtEnd() )
{
this->Makefile->GetCacheManager()
->AddCacheEntry(variable.c_str(), 0, 0,
cmCacheManager::UNINITIALIZED);
overwrite = true;
}
Now this variable is kind-of in the cache. Then comes find_program():
bool cmFindProgramCommand
::InitialPass(std::vector<std::string> const& argsIn, cmExecutionStatus &)
{
this->VariableDocumentation = "Path to a program.";
this->CMakePathName = "PROGRAM";
// call cmFindBase::ParseArguments
if(!this->ParseArguments(argsIn))
{
return false;
}
if(this->AlreadyInCache)
{
// If the user specifies the entry on the command line without a
// type we should add the type and docstring but keep the original
// value.
if(this->AlreadyInCacheWithoutMetaInfo)
{
this->Makefile->GetDefinition(this->VariableName.c_str()));
this->Makefile->AddCacheDefinition(this->VariableName.c_str(), "",
this->VariableDocumentation.c_str(),
cmCacheManager::FILEPATH);
}
return true;
}
This sees the variable is already in the cache, but without meta info and adds
it using AddCacheDefinition().
After this call, makefile->GetDefinition("FOO") returns an empty string,
before this call it still returns "foo".
I didn't figure out the correct way how to improve this.
Making mark_as_advanced() put it in the cache with the current value has the
effect that find_program() also hits the same branch, but after the
AddCacheDefinition() FOO has the value
of "/home/alex/src/CMake/tests/find-cache-var-test/bH/foo", which is due to
the type FILEPATH, which, when set, causes AddCacheDefinition to expand the
value to a full path. If it is set simply as "STRING" in the cache, it
stays "FOO".
I think it should end up in the cache as "foo" with the type "FILEPATH", but I
didn't figure out how to achieve this. |
|
|
|
(0024859)
|
|
David Cole
|
|
2011-01-18 11:17
|
|
Looking at "older" "feedback" status bugs today... is this still an issue that we want to address moving forward? If so, please remove the "feedback" status and just set it to "assigned"... If not, please resolve it.
Thanks. |
|
|
|
(0024873)
|
|
Brad King
|
|
2011-01-18 11:48
|
|
|
|
|
(0024884)
|
|
Alex Neundorf
|
|
2011-01-18 12:45
|
|
IMO the example given in is not very convincing:
http://public.kitware.com/Bug/view.php?id=9008#c18029 [^]
There the order is basically:
1) set(Foo ON CACHE)
2) set(Foo OFF)
3) set(Foo ON CACHE)
I think it is not obvious that the 3rd call should do nothing.
So, from my side, I still think this should be improved, with a policy.
Alex |
|
|
|
(0024886)
|
|
Brad King
|
|
2011-01-18 13:05
|
|
The example in 0009008:0018029 is a common idiom in real projects. As the author of the main project I would expect the set(BUILD_SHARED_LIBS OFF) to make the subproject build static.
I'm not planning to spend time on this, so I assigned it to you.
I'm happy to review the next attempt at a policy, but the example in 0009008:0018029 must work even with NEW behavior. |
|
|
|
(0029607)
|
|
sleske
|
|
2012-06-04 14:59
|
|
I also stumbled over the unexpected effect of set(...CACHE...) as described here. While a change of the present behaviour might be a good idea, I realize it is difficult to get right.
For the meantime, I have tried to improve the documentation to better explain the present behaviour; hopefully this will at least reduce confusion. See 0013269. |
|
|
|
(0029610)
|
|
Brad King
|
|
2012-06-05 10:30
|
|
|
|
|
(0029611)
|
|
Brad King
|
|
2012-06-05 10:41
|
|
I'm now convinced this should not be changed even with a policy. I'm resolving this as "won't fix". At most documentation should be changed as discussed in 0013269.
The case in 0009008:0018029 (summarized in 0009008:0024884) must continue to work as noted in 0009008:0024886, and was not affected by the change linked in 0009008:0029610. The behavior of that case has not changed since CMake started 12 years ago.
The behavior is simple: "CACHE" without "FORCE" or "INTERNAL" means "set if not already set" in the cache.
The justification is simple: if the user can edit a value in the cache (in cmake-gui for example) then s/he expects their setting to be preserved.
|
|
|
|
(0031442)
|
|
David Cole
|
|
2012-11-05 14:32
|
|
|
Closing resolved issues that have not been updated in more than 4 months. |
|
modified_cache_handling.patch (5,531) 2009-06-28 16:41