All submissions to this site are governed by the Second Life Viewer Contribution Agreement. By submitting patches and other information using this site, you acknowledge that you have read, understood, and agreed to those terms.

Review Board 1.6.11

Welcome to the Second Life Viewer Code Review tool.
See the documentation on our wiki for how to use this site.

VWR-24247: develop.py configure still searches for the wrong header file when checking for Tut

Review Request #39 - Created Dec. 18, 2010 and submitted

Aleric Inglewood Reviewers
viewer
VWR-24247
None viewer-development 
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.

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.
Boroondas Gupte
Posted (Dec. 19, 2010, 4:03 a.m.)

   

  

   

   
    
    
   


  






   







    

     
     

      

       
        

         
          indra/cmake/FindTut.cmake
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    

     
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.


     

  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    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.

    
    
    
   
    2. 

   


  





    

    

   


  
 




  

 



















 

  




 

 

  
Ship it!

  

  
Boroondas Gupte

  
Posted  (Dec. 19, 2010, 5:41 a.m.)

 

 

   
Ah, I wasn't aware gcc ignores INCLUDE. In that case, the change is good as-is.