[cmake-developers] [CMake 0016026]: CMake provides no reliable variable for 'the file a function is declared in'

2016-03-20 Thread Mantis Bug Tracker

The following issue has been SUBMITTED. 
== 
http://public.kitware.com/Bug/view.php?id=16026 
== 
Reported By:Stephen Kelly
Assigned To:
== 
Project:CMake
Issue ID:   16026
Category:   (No Category)
Reproducibility:have not tried
Severity:   minor
Priority:   normal
Status: new
== 
Date Submitted: 2016-03-20 19:35 CET
Last Modified:  2016-03-20 19:35 CET
== 
Summary:CMake provides no reliable variable for 'the file a
function is declared in'
Description: 

Given

  $ cat cmake/thefunc.cmake 

  set(somevar "${CMAKE_CURRENT_LIST_DIR}")

  function(thefunc)
message("somevar: ${somevar}")
message("listdir: ${CMAKE_CURRENT_LIST_DIR}")
  endfunction()

  $ cat dir1/CMakeLists.txt 

  include(thefunc)

  thefunc()

  include(GenerateExportHeader)

  $ cat CMakeLists.txt

  cmake_minimum_required(VERSION 2.8.12)

  project(ttt CXX)

  list(APPEND CMAKE_MODULE_PATH ${CMAKE_SOURCE_DIR}/cmake)

  add_subdirectory(dir1)

  thefunc()

  if (COMMAND generate_export_header)
add_library(foo foo.cpp)
generate_export_header(foo)
  endif()

the build output is

  somevar: /home/stephen/dev/src/playground/cmake/cmake
  listdir: /home/stephen/dev/src/playground/cmake/dir1
  somevar: 
  listdir: /home/stephen/dev/src/playground/cmake
  CMake Error: File /exportheader.cmake.in does not exist.
  CMake Error at
/home/stephen/dev/prefix/qtbase/kde/share/cmake-3.5/Modules/GenerateExportHeader.cmake:362
(configure_file):
configure_file Problem configuring file
  Call Stack (most recent call first):
   
/home/stephen/dev/prefix/qtbase/kde/share/cmake-3.5/Modules/GenerateExportHeader.cmake:378
(_do_generate_export_header)
CMakeLists.txt:14 (generate_export_header)


  -- Configuring incomplete, errors occurred!


That is: CMake doesn't provide a way to determine the file a macro or function
is defined in. The workaround is to determine that outside of the function/macro
scope. However, that workaround breaks down because the variable and the
function/macro do not follow the same scoping rules. The function/macro is
available 'globally' after definition, so users can make the mistake of trying
to use it in a scope which does not contain the `include()`.

This affects at least the GenerateExportHeader module and perhaps other modules
shipped with cmake.

One workaround is to check the existence of the variable in the function:

 https://git.reviewboard.kde.org/r/127432/diff/1

Another would be to use a global property instead of a variable for the
workaround.

A better solution may be for cmake to provide the 'path of the file this code
resides in' in a variable. 

== 

Issue History 
Date ModifiedUsername   FieldChange   
== 
2016-03-20 19:35 Stephen Kelly  New Issue
==

-- 

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] Patch to only consider build dependencies between files in the source directory

2016-03-20 Thread David Cole via cmake-developers
Brad's point with "/" or null terminator was that the directory name
**must** be the directory itself, or a sub-directory of the one in
question.

i.e.

if it's "my/src"

then it should either be exactly "my/src" or "my/src/someSubDir" , not
"my/srcSiblingDir"




On Fri, Mar 18, 2016 at 11:52 AM, Attila Krasznahorkay
 wrote:
> Hi Brad,
>
>>> +  // If it's an absolute path, check if it starts with the source
>>> +  // direcotory:
>>> +  return ( ( path.find( SourceDir ) != 0 ) &&
>>> +   ( path.find( BinaryDir ) != 0 ) );
>>
>> Please look at using strncmp and a check that the following character
>> is a nul terminator or '/'.  Otherwise an external location with
>> a common prefix may be mistaken for part of the project.
>
> Not sure what scenario you have in mind here. :-/ std::string::find should 
> only return 0 if the source directory path or binary directory path is what 
> the evaluated path begins with. Why should we worry what the path continues 
> with?
>
> I was wondering about possibly using strncmp, but thought that 
> performance-wise std::string::find should be fine as well here.
>
> I'm not at all against changing the code, I just don't understand yet what 
> setup the current code would not handle correctly.
>
>> Also please add documentation in a
>>
>>  Help/variable/CMAKE_DEPENDS_IN_PROJECT_ONLY.rst
>>
>> file and update Help/manual/cmake-variables.7.rst to reference it.
>
> Will do. Was just not sure where to add this documentation.
>
> Cheers,
> Attila
> --
>
> 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: 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] Using CMake generated ninja file as a subninja file

2016-03-20 Thread Nicolas Desprès
On Wed, Mar 9, 2016 at 9:41 PM, Ben Boeckel  wrote:

> On Tue, Mar 08, 2016 at 12:36:31 +0100, Nicolas Desprès wrote:
> > Did you have a chance to review my patches?
>
> So I looked at it today, and it looks good overall. A few niggles:
>
Thanks

>
> +inline bool cmEndsWith(const std::string& str, const std::string& what)
> +{
> +  assert(str.size() >= what.size());
>
> Probably better to just "return false;" if this would fire. Better than
> crashing if something in a release would have hit this.
>
>
Ok.


> +  return str.compare(str.size() - what.size(), what.size(), what) == 0;
> +}
> +
> +inline void cmStripSuffixIfExists(std::string* str,
> +  const std::string& suffix)
> +{
> +  assert(str != NULL);
>
> Why not just use a reference rather than a pointer?
>

I don't like to use non-const reference argument because the caller may not
expect its variable to be modified since it not passed it by address.
Anyway, reference argument are used in other places in the source code so
it does not matter.


>
> +  if (cmEndsWith(*str, suffix))
> +str->resize(str->size() - suffix.size());
>
> Missing braces.
>

Ok.


>
> -std::string cmGlobalNinjaGenerator::ConvertToNinjaPath(const std::string&
> path)
> +static void EnsureTrailingSlash(std::string* path)
> +{
> +  assert(path != NULL);
>
> Same thing: why not a reference?
>
Done.

>
> +  if (path->empty())
> +return;
> +  std::string::value_type last = (*path)[path->size()-1];
> +#ifdef _WIN32
> +  if (last != '\\')
> +*path += '\\';
> +#else
> +  if (last != '/')
> +*path += '/';
> +#endif
>
> Missing braces in the if statements here.
>
>
Ok.


> -  cmGlobalNinjaGenerator::WriteDefault(os,
> -   outputs,
> -   "Make the all target the
> default.");
> +  if (!this->HasOutputPathPrefix())
> +cmGlobalNinjaGenerator::WriteDefault(os,
> + outputs,
> + "Make the all target the
> default.");
>
> Missing braces.
>
Ok.

>
> +void
> +cmGlobalNinjaGenerator::StripNinjaOutputPathPrefixAsSuffix(std::string*
> path)
> +{
> +  assert(path != NULL);
>
> More pointers :) .
>
Ok.

>
> +  if (path->empty())
> +return;
>
> And braces.
>
Ok.

The fixes are that commit:
https://github.com/nicolasdespres/CMake/commit/3fa749a19847898fcbb5635a273b0d02707dd4bd


> As for the tests:
>
> +  file(WRITE "${top_build_ninja}" "\
> +subninja sub/build.ninja
> +default sub/all
> +")
>
> I think adding the (documented) bit:
>
> +rule RERUN
> +  command = true
> +  description = Testing regeneration
> +  generator = 1
> +
> +build build.ninja: RERUN || sub/build.ninja
> +  pool = console
>
> here and testing that if the CMakeLists.txt is touched that CMake reruns
> would be worth it. It seems to work here, so keeping it working would be
> great.
>

I guess that test should exist on the super-generator side. But it is
probably safer to test it here too.

The fix is in that commit:
https://github.com/nicolasdespres/CMake/commit/13f341588bc6ee1cb0ec5dce8f44602f5d066ca9

Tell me if you prefer I squash all the commits together before you review.

Thanks,

-- 
Nicolas Desprès
-- 

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