[opensource-dev] Review Request: VWR-24247: develop.py configure still searches for the wrong header file when checking for Tut

[Aleric Inglewood]

> On 2010-12-19 04:03:39, Boroondas Gupte wrote:
> > indra/cmake/FindTut.cmake, line 10
> > <http://codereview.secondlife.com/r/39/diff/1/?file=101#file101line10>
> >
> >     According to the CMake 2.8.1 man page, NO_SYSTEM_ENVIRONMENT_PATH makes find_path not only skip $PATH but also $INCLUDE. I think we should respect the latter when looking for headers.

I was aware of that, but I don't think that INCLUDE is ever used on linux. Not only not for any existing project, but also gcc doesn't honor it. It can hardly be considered to be a 'standard' (useful) search path for *system* header files when the default compiler isn't even using that environment variable.

My guess was that this is more of a Windows(tm) thing. And indeed, Michelle just told me that on windows the command line compiler needs %INCLUDE% to function. So, I think that cmake searches PATH and INCLUDE because of Windows(tm). For example, Windows(tm) sets INCLUDE="C:\Program Files (x86)\Microsoft Visual Studio 8\VC\INCLUDE";"C:\Program Files\Microsoft SDKs\Windows\v6.0A\Include".

However, this code is only run on linux, where INCLUDE is not used, and it might even be harmful to search in PATH for header files. Like I said before, it probably won't hurt in this case (it's unlikely that anyone has an executable installed in their PATH called 'tut/tut.hpp') but I don't consider it to be the right thing for linux.

That being said - in this particular case (for tut/tut.hpp) it will work just as fine with or without, so if that is a showstopper for Oz (or whoever has to add this patch) than I'd be ok with removing NO_SYSTEM_ENVIRONMENT_PATH and gambling that nothing will be found in PATH, instead of using the patch as-is: ignoring the "windows" INCLUDE on linux just like gcc ignores it.


- Aleric


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/39/#review49
-----------------------------------------------------------


On 2010-12-18 09:01:35, Aleric Inglewood wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/39/
> -----------------------------------------------------------
> 
> (Updated 2010-12-18 09:01:35)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> -------
> 
> There is no #include "tut.h" anywhere.
> The only includes are #include "tut/tut.hpp" (which is correct).
> Tut installs in <prefix>/include/tut/* among which the headerfile tut.hpp.
> The changed file, indra/cmake/FindTut.cmake, is only included when configured with --standalone (and thus only on linux).
> find_path searches in /usr/include and /usr/local/include anyway, so there is no way to add that.
> Adding the NO_SYSTEM_ENVIRONMENT_PATH stops it from reading the PATH environment variable and looking
> for tut/tut.hpp, which probably won't harm, but it simply makes no sense: we're looking for a headerfile, not an executable.
> 
> Without this patch (and without creating a fake /usr/local/include/tut.h), I get the error:
> CMake Error at cmake/FindTut.cmake:26 (message):
>   Could not find Tut
> Call Stack (most recent call first):
>   cmake/Tut.cmake:8 (include)
>   llmessage/CMakeLists.txt:13 (include)
> 
> With the patch, it finds /usr/local/include/tut/tut.hpp as expected, setting TUT_INCLUDE_DIR to "/usr/local/include",
> and it also finds (in my case) /usr/src/secondlife/viewers/snowstorm/viewer-development/include/tut/tut.hpp, setting
> TUT_INCLUDE_DIR to "/usr/src/secondlife/viewers/snowstorm/viewer-development/include", since I have a symbolic
> link from /usr/src/secondlife/viewers/snowstorm/viewer-development/include/tut to ../linden/libraries/include/tut
> (which was installed manually with ./scripts/install.py tut) and I have the environment variable CMAKE_INCLUDE_PATH
> set to "/usr/src/secondlife/llqtwebkit/install-imprudence/include:/usr/src/secondlife/viewers/snowstorm/viewer-development/include:/sl/usr/include".
> In other words, this allows developers to install headers whereever they want and use the API of cmake as it is
> intended, to find those headers.
> 
> 
> This addresses bug VWR-24247.
>     http://jira.secondlife.com/browse/VWR-24247
> 
> 
> Diffs
> -----
> 
>   indra/cmake/FindTut.cmake b0689af42a71 
> 
> Diff: http://codereview.secondlife.com/r/39/diff
> 
> 
> Testing
> -------
> 
> Tested to see if it finds the header when installed in /usr/local/include as well in a path specified
> with CMAKE_INCLUDE_PATH.
> 
> Note that you need to configure with --standalone in order to test this.
> Also note that unless tut is installed in /usr/local, and when you configure with -DLL_TESTS:BOOL=ON,
> it still won't compile because of another bug. I will submit a patch for that separately.
> 
> 
> Thanks,
> 
> Aleric
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20101219/7e35d996/attachment-0001.htm