Re: [cmake-developers] set(CACHE) and the local scope

[Ben Boeckel]

On Fri, Dec 11, 2015 at 22:13:46 +0100, Alexander Neundorf wrote:
> On Friday, December 11, 2015 14:44:39 Ben Boeckel wrote:
> ...
> > Option 3:
> > 
> > set(CACHE) (and any other cache-touching behavior) does *nothing*
> > with the cache if a local variable by that name is defined
> 
> just to clarify: and also does nothing to the local variable ?

Correct.

--Ben
-- 

Powered by www.kitware.com

Please keep messages on-topic and check the CMake FAQ at: 
http://www.cmake.org/Wiki/CMake_FAQ

Kitware offers various services to support the CMake community. For more 
information on each offering, please visit:

CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html

Visit other Kitware open-source projects at 
http://www.kitware.com/opensource/opensource.html

Follow this link to subscribe/unsubscribe:
http://public.kitware.com/mailman/listinfo/cmake-developers

Re: [cmake-developers] set(CACHE) and the local scope

[Alexander Neundorf]

On Friday, December 11, 2015 14:44:39 Ben Boeckel wrote:
...
> Option 3:
> 
> set(CACHE) (and any other cache-touching behavior) does 
*nothing*
> with the cache if a local variable by that name is defined

just to clarify: and also does nothing to the local variable ?

Alex

-- 

Powered by www.kitware.com

Please keep messages on-topic and check the CMake FAQ at: 
http://www.cmake.org/Wiki/CMake_FAQ

Kitware offers various services to support the CMake community. For more 
information on each offering, please visit:

CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html

Visit other Kitware open-source projects at 
http://www.kitware.com/opensource/opensource.html

Follow this link to subscribe/unsubscribe:
http://public.kitware.com/mailman/listinfo/cmake-developers

Re: [cmake-developers] set(CACHE) and the local scope

[Ben Boeckel]

On Thu, Dec 10, 2015 at 08:50:10 -0500, Brad King wrote:
>  BUG: change in how set cache overrides local definitions. Should mainly be a 
> NOP change for most cases
>  https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=f52d37c2

So Brad and I discussed today the reasons behind this change. Here is
the thread behind the cause for the change:

http://public.kitware.com/pipermail/cmake/2007-March/013198.html

The problem was this pattern:

function (set_cache var value)
set("${var}" "${value}" CACHE INTERNAL "docs")
endfunction ()

set(some_var value CACHE INTERNAL "docs")
set_cache(some_var other_value)
message("${some_var}")

Before this change, "value" would be output because the first set(CACHE)
would set `some_var` to `value` in the local scope, but afterwards,
because the variable is unset, the cache would be queried directly and
"other_value" would be output.

###

For work related to any changes to the interactions between the local
scope and the cache, here is the baseline that must be met:

The first configure must not differ from subsequent configures.

set(CACHE) using either FORCE or the INTERNAL type do not have this
problem currently since they always touch the local scope as well. I
don't think anyone is going to disagree that the existing behavior is a
problem in this regard, so the question is which behavior to prefer
(whether set(CACHE) has other optional arguments to select a specific
behavior or not is a separate question):

Option 1:

set(CACHE) always pulls from the cache into the local scope (first
configure behavior is canonical)

Option 2:

set(CACHE) never touches the local scope (subsequent configure
behavior is canonical)

The main benefit of the first option is that reading the code is more
"logical" in that set(CACHE) always does something to that variable
(makes the cache's value available as ${var}). The value is
unpredictable since the user can always set the cache to some bogus
value (the STRINGS property or type hints will not save you because -D
exists).

The main benefit of the second option is that projects embedding
external projects could override cache variables inside of that project
without set(CACHE INTERNAL) (which doesn't help in the case of FORCE or
INTERNAL variables that inner project uses anyways).

A third option that Brad and I brainstormed after tracking down various
bits of history and thinking about the behaviors is:

Option 3:

set(CACHE) (and any other cache-touching behavior) does *nothing*
with the cache if a local variable by that name is defined

This has the benefits of the second in that superset projects can set
projects *and* hide cache values with a single set() command and also
caches become less cluttered (e.g., if you set PYTHON_EXECUTABLE
explicitly, the cache entry for FindPythonInterp doesn't appear and
since the project is presumably forcing the value using a local
variable, that is what is wanted anyways, so don't let the user mess
that up).

This policy would become an invasive change (there are 40 (+6 if you
count cmCPluginAPI) call sites of cmMakefile::AddCacheDefinition that
need audited for behavior changes due to whatever policy behavior is
chosen.

Thoughts?

--Ben
-- 

Powered by www.kitware.com

Please keep messages on-topic and check the CMake FAQ at: 
http://www.cmake.org/Wiki/CMake_FAQ

Kitware offers various services to support the CMake community. For more 
information on each offering, please visit:

CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html

Visit other Kitware open-source projects at 
http://www.kitware.com/opensource/opensource.html

Follow this link to subscribe/unsubscribe:
http://public.kitware.com/mailman/listinfo/cmake-developers

Re: [cmake-developers] set(CACHE) and the local scope

[Robert Maynard]

I like the sounds of both Option 1, and Option 2. I feel Option 3 is really
bad.

We should remember that people understand/are taught the current CMake
behavior of local variables being preferred over cache variables.

If we move to Option 3, that rule becomes "local variables are preferred
over cache variables, and cache variables are not constructed if a local
variable exists ( even if unset? ) with the same name.". I can already
imagine people writing functions that try to set cache variables, but can't
since a local function variable is blocking them.




On Fri, Dec 11, 2015 at 2:44 PM, Ben Boeckel 
wrote:

> On Thu, Dec 10, 2015 at 08:50:10 -0500, Brad King wrote:
> >  BUG: change in how set cache overrides local definitions. Should mainly
> be a NOP change for most cases
> >  https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=f52d37c2
>
> So Brad and I discussed today the reasons behind this change. Here is
> the thread behind the cause for the change:
>
> http://public.kitware.com/pipermail/cmake/2007-March/013198.html
>
> The problem was this pattern:
>
> function (set_cache var value)
> set("${var}" "${value}" CACHE INTERNAL "docs")
> endfunction ()
>
> set(some_var value CACHE INTERNAL "docs")
> set_cache(some_var other_value)
> message("${some_var}")
>
> Before this change, "value" would be output because the first set(CACHE)
> would set `some_var` to `value` in the local scope, but afterwards,
> because the variable is unset, the cache would be queried directly and
> "other_value" would be output.
>
> ###
>
> For work related to any changes to the interactions between the local
> scope and the cache, here is the baseline that must be met:
>
> The first configure must not differ from subsequent configures.
>
> set(CACHE) using either FORCE or the INTERNAL type do not have this
> problem currently since they always touch the local scope as well. I
> don't think anyone is going to disagree that the existing behavior is a
> problem in this regard, so the question is which behavior to prefer
> (whether set(CACHE) has other optional arguments to select a specific
> behavior or not is a separate question):
>
> Option 1:
>
> set(CACHE) always pulls from the cache into the local scope (first
> configure behavior is canonical)
>
> Option 2:
>
> set(CACHE) never touches the local scope (subsequent configure
> behavior is canonical)
>
> The main benefit of the first option is that reading the code is more
> "logical" in that set(CACHE) always does something to that variable
> (makes the cache's value available as ${var}). The value is
> unpredictable since the user can always set the cache to some bogus
> value (the STRINGS property or type hints will not save you because -D
> exists).
>
> The main benefit of the second option is that projects embedding
> external projects could override cache variables inside of that project
> without set(CACHE INTERNAL) (which doesn't help in the case of FORCE or
> INTERNAL variables that inner project uses anyways).
>
> A third option that Brad and I brainstormed after tracking down various
> bits of history and thinking about the behaviors is:
>
> Option 3:
>
> set(CACHE) (and any other cache-touching behavior) does *nothing*
> with the cache if a local variable by that name is defined
>
> This has the benefits of the second in that superset projects can set
> projects *and* hide cache values with a single set() command and also
> caches become less cluttered (e.g., if you set PYTHON_EXECUTABLE
> explicitly, the cache entry for FindPythonInterp doesn't appear and
> since the project is presumably forcing the value using a local
> variable, that is what is wanted anyways, so don't let the user mess
> that up).
>
> This policy would become an invasive change (there are 40 (+6 if you
> count cmCPluginAPI) call sites of cmMakefile::AddCacheDefinition that
> need audited for behavior changes due to whatever policy behavior is
> chosen.
>
> Thoughts?
>
> --Ben
> --
>
> Powered by www.kitware.com
>
> Please keep messages on-topic and check the CMake FAQ at:
> http://www.cmake.org/Wiki/CMake_FAQ
>
> Kitware offers various services to support the CMake community. For more
> information on each offering, please visit:
>
> CMake Support: http://cmake.org/cmake/help/support.html
> CMake Consulting: http://cmake.org/cmake/help/consulting.html
> CMake Training Courses: http://cmake.org/cmake/help/training.html
>
> Visit other Kitware open-source projects at
> http://www.kitware.com/opensource/opensource.html
>
> Follow this link to subscribe/unsubscribe:
> http://public.kitware.com/mailman/listinfo/cmake-developers
>
-- 

Powered by www.kitware.com

Please keep messages on-topic and check the CMake FAQ at: 
http://www.cmake.org/Wiki/CMake_FAQ

Kitware offers various services to support the CMake community. For more 
information on each offering, please visit:

CMake Support: 

Re: [cmake-developers] set(CACHE) and the local scope

[Alexander Neundorf]

On Wednesday, December 09, 2015 17:35:28 Ben Boeckel wrote:
> Hi,
> 
> So some behavior I was unaware of until today came up:
> 
> set(var ON)
> option(var "description" OFF)
> message("var: ${var}")

Assuming I wouldn't know about the subtle characteristics of normal vs. 
cache variables, I think I would expect that var has the value of the option 
afterwards.

I.e. on the first run it would be OFF (since that's the default value of the 
option), and all later runs it would have the value which is in the cache.

Alex

-- 

Powered by www.kitware.com

Please keep messages on-topic and check the CMake FAQ at: 
http://www.cmake.org/Wiki/CMake_FAQ

Kitware offers various services to support the CMake community. For more 
information on each offering, please visit:

CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html

Visit other Kitware open-source projects at 
http://www.kitware.com/opensource/opensource.html

Follow this link to subscribe/unsubscribe:
http://public.kitware.com/mailman/listinfo/cmake-developers

Re: [cmake-developers] set(CACHE) and the local scope

[Alexander Neundorf]

On Thursday, December 10, 2015 15:26:54 Brad King wrote:
> On 12/10/2015 03:20 PM, Alexander Neundorf wrote:
> >> set(var ON)
> >> option(var "description" OFF)
> >> message("var: ${var}")
> > 
> > I.e. on the first run it would be OFF (since that's the default value
> > of the option), and all later runs it would have the value which is in 
the
> > cache.
> This is calling for the opposite change than Ben's proposal.  Ben
> suggests never unsetting the local value to expose the cached value.
> Alex is suggesting always doing so, even if the cache option is
> not created by the command.
> 
> Alternatively the option() or set(CACHE) commands could also set
> a local variable to the same value as the cache entry.
> 
> This is pretty fundamental behavior so if we are going to mess with
> it through a policy we better get it right.

Yes. :-)

My motivation: if the option() would just set/get the cache variable, and 
leave the local variable untouched, it would be a NOOP in the example 
above, and this is not obvious from seeing that code.

Alex

-- 

Powered by www.kitware.com

Please keep messages on-topic and check the CMake FAQ at: 
http://www.cmake.org/Wiki/CMake_FAQ

Kitware offers various services to support the CMake community. For more 
information on each offering, please visit:

CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html

Visit other Kitware open-source projects at 
http://www.kitware.com/opensource/opensource.html

Follow this link to subscribe/unsubscribe:
http://public.kitware.com/mailman/listinfo/cmake-developers

Re: [cmake-developers] set(CACHE) and the local scope

[Ben Boeckel]

On Thu, Dec 10, 2015 at 21:20:21 +0100, Alexander Neundorf wrote:
> On Wednesday, December 09, 2015 17:35:28 Ben Boeckel wrote:
> > So some behavior I was unaware of until today came up:
> > 
> > set(var ON)
> > option(var "description" OFF)
> > message("var: ${var}")
> 
> Assuming I wouldn't know about the subtle characteristics of normal vs. 
> cache variables, I think I would expect that var has the value of the option 
> afterwards.
> 
> I.e. on the first run it would be OFF (since that's the default value of the 
> option), and all later runs it would have the value which is in the cache.

The problem with this behavior is that if I do -Dinternal_var:BOOL=OFF,
it will override any variable of that name inside the project and it
cannot override it without knowing it is in the cache to unset it so
that the local variable is used again.

This behavior also breaks cmake_dependent_option as-is since it sets a
local variable to the fallback value if its requirements are not met
(which is how it remembers the user decision when it becomes a viable
option again).

--Ben
-- 

Powered by www.kitware.com

Please keep messages on-topic and check the CMake FAQ at: 
http://www.cmake.org/Wiki/CMake_FAQ

Kitware offers various services to support the CMake community. For more 
information on each offering, please visit:

CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html

Visit other Kitware open-source projects at 
http://www.kitware.com/opensource/opensource.html

Follow this link to subscribe/unsubscribe:
http://public.kitware.com/mailman/listinfo/cmake-developers

Re: [cmake-developers] set(CACHE) and the local scope

[Brad King]

On 12/10/2015 03:20 PM, Alexander Neundorf wrote:
>> set(var ON)
>> option(var "description" OFF)
>> message("var: ${var}")
> 
> I.e. on the first run it would be OFF (since that's the default value
> of the option), and all later runs it would have the value which is in the 
> cache.

This is calling for the opposite change than Ben's proposal.  Ben
suggests never unsetting the local value to expose the cached value.
Alex is suggesting always doing so, even if the cache option is
not created by the command.

Alternatively the option() or set(CACHE) commands could also set
a local variable to the same value as the cache entry.

This is pretty fundamental behavior so if we are going to mess with
it through a policy we better get it right.

-Brad

-- 

Powered by www.kitware.com

Please keep messages on-topic and check the CMake FAQ at: 
http://www.cmake.org/Wiki/CMake_FAQ

Kitware offers various services to support the CMake community. For more 
information on each offering, please visit:

CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html

Visit other Kitware open-source projects at 
http://www.kitware.com/opensource/opensource.html

Follow this link to subscribe/unsubscribe:
http://public.kitware.com/mailman/listinfo/cmake-developers

Re: [cmake-developers] set(CACHE) and the local scope

[Brad King]

On 12/09/2015 05:35 PM, Ben Boeckel wrote:
> So some behavior I was unaware of until today came up:
> 
> set(var ON)
> option(var "description" OFF)
> message("var: ${var}")
> 
> outputs "OFF" for the first configure and "ON" for subsequent
> configures. This is because set(var CACHE) does unset(var) *if* the
> cache was touched. This is not done on the second time around since it
> is already in the cache.

That is a long-standing subtlety introduced without discussion, review,
or tests here:

 BUG: change in how set cache overrides local definitions. Should mainly be a 
NOP change for most cases
 https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=f52d37c2

IIRC there was confusion at the time in the case of

 set(var 1)
 set(var 2 CACHE STRING ...)
 message("${var}") # prints "1" before the above change.

> I think a policy to remove the unset(var) behavior should be added since
> the current behavior means that clean builds can be wildly different
> than incremental builds.

One reason a policy has not been introduced for this before is that
producing a warning for the policy may be very verbose unless it is
delayed until variable dereference, but the latter would be a huge
performance hit to check.  Still, I think things would be better off
in the long run with some policy for it.

> Related, I have a branch on the stage (update-variable-docs) which
> attempts to clarify some darker corners of the set() command and the
> *VARIABLES directory properties.

The "LOCAL_VARIABLES" change uses "SCOPE_VARIABLES" in some places.
The release note should only cover the new feature.

-Brad

-- 

Powered by www.kitware.com

Please keep messages on-topic and check the CMake FAQ at: 
http://www.cmake.org/Wiki/CMake_FAQ

Kitware offers various services to support the CMake community. For more 
information on each offering, please visit:

CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html

Visit other Kitware open-source projects at 
http://www.kitware.com/opensource/opensource.html

Follow this link to subscribe/unsubscribe:
http://public.kitware.com/mailman/listinfo/cmake-developers

Re: [cmake-developers] set(CACHE) and the local scope

[Ben Boeckel]

On Thu, Dec 10, 2015 at 08:50:10 -0500, Brad King wrote:
> That is a long-standing subtlety introduced without discussion, review,
> or tests here:
> 
>  BUG: change in how set cache overrides local definitions. Should mainly be a 
> NOP change for most cases
>  https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=f52d37c2
> 
> IIRC there was confusion at the time in the case of
> 
>  set(var 1)
>  set(var 2 CACHE STRING ...)
>  message("${var}") # prints "1" before the above change.

The above commit has the same behavior as it does today: first configure
!= subsequent configure, so I don't see the confusion as being *less*
after the fix since any existing tree wouldn't say "2" either, but I
suppose it was something about dashboards doing clean builds.

> On 12/09/2015 05:35 PM, Ben Boeckel wrote:
> > I think a policy to remove the unset(var) behavior should be added since
> > the current behavior means that clean builds can be wildly different
> > than incremental builds.
> 
> One reason a policy has not been introduced for this before is that
> producing a warning for the policy may be very verbose unless it is
> delayed until variable dereference, but the latter would be a huge
> performance hit to check.  Still, I think things would be better off
> in the long run with some policy for it.

Like with some of the more disruptive policies (e.g., CMP0054), it's a
clarification of some potentially^Wconfusing behavior which can bite you
in certain cases pretty hard (CI vs. developer builds).

> The "LOCAL_VARIABLES" change uses "SCOPE_VARIABLES" in some places.
> The release note should only cover the new feature.

Oops. Updated. Also reordered the branch so the feature is at the end of
the branch.

--Ben
-- 

Powered by www.kitware.com

Please keep messages on-topic and check the CMake FAQ at: 
http://www.cmake.org/Wiki/CMake_FAQ

Kitware offers various services to support the CMake community. For more 
information on each offering, please visit:

CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html

Visit other Kitware open-source projects at 
http://www.kitware.com/opensource/opensource.html

Follow this link to subscribe/unsubscribe:
http://public.kitware.com/mailman/listinfo/cmake-developers