Closed Bug 1121132 Opened 9 years ago Closed 9 years ago

A broken commit message can break builds

Categories

(Testing :: Mozbase, defect)

Product:
Testing
Component:
Mozbase
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(Not tracked)

Status:
RESOLVED DUPLICATE of bug 1146035

People

(Reporter: RyanVM, Unassigned)

Details

Attachments

(1 file, 2 obsolete files)

MozReview Request: Bug 1121132 - script.py's write_to_file should use codec.open for unicode support, r=jlund
39 bytes, text/x-review-board-request
jlund
: review-
Details
Attached file MozReview Request: bz://1121132/ahal (obsolete) — 

        Attachment #8548430 -
        Flags: review?(jlund)

    
    

    
  
/r/2459 - Bug 1121132 - script.py's write_to_file should use codec.open for unicode support, r=jlund

Pull down this commit:

hg pull review -r c8963cc8dbb654118618d041345e90b8bf410540

    
    

    
  
(In reply to Andrew Halberstadt [:ahal] from comment #3)
> I think this should fix it (according to [1]). Here's a try run:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=7be2d228d4ad
> 
> [1]
> https://docs.python.org/2/howto/unicode.html#reading-and-writing-unicode-data

FYI, the original commit message that caused bustage on fx-team worked when pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9671b6be4cd (test failures unrelated).

    
    

    
  
https://reviewboard.mozilla.org/r/2457/#review1731

ah, this reminds me, I hadn't followed up with: https://bugzilla.mozilla.org/show_bug.cgi?id=1106342 I guess unicode characters are more popular than I thought :)

in that bug I used a similar approach which was r- by catlee. I agree it is probably safer to add a parameter to write_to_file and that encodes only when we want: e.g. for a commit message. feel free to leave with me or else add the option to write_to_file and I'll r+

    
    

    
  
Heh, originally I had added an encoding parameter to that function, but then noticed that elsewhere in script.py we just use 'utf-8' directly, so decided to follow that convention:
https://dxr.mozilla.org/build:mozharness/source/mozharness/base/script.py#1343

Is what you're proposing simply to add an encoding='utf-8' to the method signature?

    
    

    
  
(In reply to Andrew Halberstadt [:ahal] from comment #6)
> Heh, originally I had added an encoding parameter to that function, but then
> noticed that elsewhere in script.py we just use 'utf-8' directly, so decided
> to follow that convention:
> https://dxr.mozilla.org/build:mozharness/source/mozharness/base/script.
> py#1343

which might be safe to follow that convention. poke catlee if you feel strongly about this. I suspect his concerns are the unknown outcome of switching all the current working instances where we call write_to_file without encoding open() 

> 
> Is what you're proposing simply to add an encoding='utf-8' to the method
> signature?

we want encoding passed but we want the default to be None (i.e. encoding=None in write_to_file meth sig). Then for the commit message, explicitly pass encoding='utf-8'.


at least that's my interpretation of https://bugzilla.mozilla.org/show_bug.cgi?id=1106342#c1

    
    

    
  
Comment on attachment 8548430 [details]
MozReview Request: bz://1121132/ahal

putting r- to clear my bugzilla queue while the patch is iterated on.

if you feel strongly about defaulting to utf-8 over None, I don't want to block but you should pass it by catlee since he r- me on it.

        Attachment #8548430 -
        Flags: review?(jlund) → review-

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Whoops... just uploaded this to the wrong bug. Can this be deleted?

        Attachment #8554824 -
        Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE

    
    

    
  

        Attachment #8548430 -
        Attachment is obsolete: true

        Attachment #8619123 -
        Flags: review-

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: