461: patch 1211 broke Win32 diff functionality in post-review with Perforce

josep******@gmai***** (Google Code) (Is this you? Claim this profile.)
April 21, 2008 
What's the URL of the page containing the problem?
n/a

The checkin 1211 that changed the "execute" method in the
contrib/post-review to use subprocess instead of os.popen has the
unfortunate side effect of breaking post-review running on Win32 natively
with Python 2.4.2 and 2.5.1 (those I checked, presumably others) when
working with Perforce.

The difference is that the output generated from a command differs in line
endings, which is significant to the diff command in post-review. I finally
tracked this down and wrote a test script to generate files showing the
differences, which is attached (some changes will be needed to  make that
test script work for you locally - provide a file easily printed out from
your local perforce instance).

I believe this is specific to python running on Win32 natively
testy.py 
import os
import subprocess
import sys
command = "p4 print -q \"//p4-tss/users/jheck/maventest/src/test/java/com/disney/jheck/AppTest.java#4\""
def one():
    p = subprocess.Popen(command,
                        stdin=subprocess.PIPE,
                        stdout=subprocess.PIPE,
                        stderr=subprocess.STDOUT,
                        shell=True)
    data = p.stdout.read()
    rc = p.wait()
    zout = open("one",'w')
    zout.write(data)
    zout.close()
def two():
    f = os.popen(command, 'r')
    result = f.read()
    f.close()
    zout = open("two",'w')
    zout.write(result)
    zout.close()
one()
two()

 






  


 










 

  

   chipx86

  

 
 

  

    
     

   
   #1
   chipx86


  


  
Is this fixed in your review request?



  
    


   
  • 
    
    
    -Priority-Medium
    +Priority-Critical
    +Component-Scripts
    +Milestone-Release1.0
    
   
    • 


  


 








 

   

 
 

  

    
     

   
   #2
   josep******@gmai***** (Google Code)



(Is this you? Claim this profile.)


  


  
I'm afraid not entirely. I didn't resolve the output difference between os.popen and
the subprocess bits. I'm afraid I came down with the flu last week and it's stopped
pretty much all my work right now. I'll come back to it to resolve this piece though
(unless someone else figures it out and fixes it first)



 








 

  

   david

  

 
 

  

    
     

   
   #3
   david


  


  
Should hopefully be fixed with r1307.



  
    


   
  • 
    
    
    +Fixed
    
   
    • 


  


 








 

   

 
 

  

    
     

   
   #4
   josep******@gmai***** (Google Code)



(Is this you? Claim this profile.)


  


  
All fixed - Thanks! And I learned about "universal_newlines" in the process - double win!