Review Board 2.0.15


swig: move all swigged objects into m5.internal.swig package

Review Request #878 - Created Sept. 24, 2011 and updated

Information
Steve Reinhardt
gem5
Reviewers
Default
ali, gblack, nate, stever 
swig: move all swigged objects into m5.internal.swig package

SWIG-generated packages for params, enums, wrapped C++ SimObjects,
etc. were being generated directly into m5.internal, with some
funky code to import some of the declarations into m5.internal.params.
Moved all generated packages under m5.internal.swig and imported
things directly from there where necessary.  Cleaned up some
exposed loose ends.  Turns out m5.internal.params doesn't really seem
to be needed, so it's gone now.

Also renamed param_Foo packages that wrap both the C++ version of
Foo and FooParams to just Foo; now that they're under a separate
subpackage there's no ambiguity with the primary Python objects,
and no need to have the param_ prefix to support the funky
importing into internal.params.

   

  


 


 

  

  









  
 


  

 


















 

  


 


  



   

   
Gabe Black

   
Posted  (Sept. 24, 2011, 10:33 a.m.)


  


  


  

   
I don't have anything against this change, but I do have a question. How does this affect simulation scripts? Do they need to import things different, declare things differently, ...?



  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    It should be transparent to user simulation scripts... all this swig-wrapped stuff is only accessed directly by other internal methods.  Definitely nothing in the regression tests needed to change.
    
    
    
   
    2. 

   


  





  

 


  

 



















 

  


 


  



   

   
Nathan Binkert

   
Posted  (Sept. 25, 2011, 2:21 a.m.)


  


  


  

   
Make sure that you don't run afoul of the problem in changeset c6e283904437.  We used to have something like this, but I undid it because it broke older versions of swig.  You have a different approach here, but I'd hate to have you reintroduce a bug.



  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Ugh, that's a bummer... thanks for pointing it out.  Is it unreasonable just to advance the minimum required swig version?  I see that 1.3.39 came out in 3/09 (2.5 yrs ago), and the latest release is 2.0.4, so supporting versions older than 1.3.39 doesn't seem terribly compelling to me.

At least this explains the weird mess that is the status quo... I wondered what you were thinking when you did it the way you did :-).
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    I was also going to suggest bumping the minimum version number. Newer doesn't necessarily mean better since the new versions have bugs too (if I remember right, 2.0.4 + gcc 4.6.1 doesn't build but 2.0.3 does) so we shouldn't be *too* aggressive, but bumping beyond several year old bugs seems reasonable.
    
    
    
   
    3. 


   
  3. 
    
    
     
    
      
     
    
     
    I don't have a problem with it.  Though we shouldn't go as much in terms of years, but rather we should pick distros that we want to support.  The main reason for supporting the version that we did was because zizzer was running 8.04.
    
    
    
   
    4. 

   


  





  

 


  

 



















 

  


 


  



   

   
Nathan Binkert

   
Posted  (Sept. 25, 2011, 2:25 a.m.)


  


  


  

   



  

   

   
    
    
   


  




   







    

     
     

      

       
        

         
          src/python/m5/params.py
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
I'm pretty sure that this will break older versions of swig.  See changeset c6e283904437




  

   

   
    
    
   


  


    


    

     
     

      

       
        

         
          src/python/m5/params.py
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
Yeah, this too.




  

   

   
    
    
   


  


    


    

     
     

      

       
        

         
          src/python/swig/inet.i
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
We don't build this as a module.




  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Not sure what you mean... it seems to work.  Doesn't listing it as SwigSource() in python/SConscript enough?  Or do I need to do more than that?
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    Sorry.  I didn't realize that you exposed it in the SConscript. This is correct if that is the case.  (Why did you expose them?)
    
    
    
   
    3. 


   
  3. 
    
    
     
    
      
     
    
     
    Since they were being imported via internal.params and I got rid of that package, I had to change something... I suppose I could have imported them from internal.swig.wherever but at the time it just seemed asymmetric that some of the swig/*.i files ended up in their own packages but these others didn't (and you couldn't tell by looking at the .i files where they ended up), so it just seemed more regular to make them be their own packages too.  If there's a good reason not to do it that way, I don't mind revisiting it.
    
    
    
   
    4. 

   


  


    


    

     
     

      

       
        

         
          src/python/swig/time.i
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
We don't build this as a module either.  I guess it's future proofing though.




  

   

   
    
    
   


  


    


   



  

 


  

 

















 



 

  


 


  



   

   
Ali Saidi

   
Posted  (Feb. 1, 2012, 5:38 a.m.)


  


  


  

   
what happened to this?



  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    I think at the time I didn't want to bother with testing against older versions of swig, and it was mostly just a cosmetic change anyway, so I just left it.  If we actually did bump the minimum swig rev to take care of the backwards compatibility issue, I'd be happy to re-test it and commit if it still works.
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    Now that everything is in a single package, I don't think that error that I mentioned before necessarily applies.  I think we should be able to create a single swig package and it should just work with the relative imports.  That said, this is only a guess and I'm only 75% confident.  It would need a little bit of proof.
    
    
    
   
    3.