Closed Bug 1121132 Opened 5 years ago Closed 5 years ago

A broken commit message can break builds

Categories

(Testing :: Mozbase, defect, critical)

defect
Not set
critical

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1146035

People

(Reporter: RyanVM, Unassigned)

Details

Attachments

(1 file, 2 obsolete files)

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: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1146035
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.