Closed Bug 1360571 Opened 8 years ago Closed 8 years ago

mozconfig.py should use as shell bash instead of sh on Solaris

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Version:
52 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: petr.sumbera, Assigned: petr.sumbera)

Details

Attachments

(1 file, 3 obsolete files)

Bug1360571.patch
2.44 KB, patch
glandium
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0 Build ID: 20170323110425 Steps to reproduce: On Solaris /bin/sh is not bash (it's ksh) and thus it don't know about 'local' keyword as used in python/mozbuild/mozbuild/mozconfig_loader. Therefoe it should be allowed to use bash on Solaris.
Attached patch Bug1360571.patch (obsolete) — — Splinter Review
Attachment #8862867 - Flags: review?(mh+mozilla)
Component: Untriaged → Build Config
Product: Firefox → Core
Comment on attachment 8862867 [details] [diff] [review] Bug1360571.patch Review of attachment 8862867 [details] [diff] [review]: ----------------------------------------------------------------- Actually, since mozconfig_loader uses local, using /bin/sh is wrong on all systems.
Attachment #8862867 - Flags: review?(mh+mozilla) → review-
What do you propose? Use bash for all, avoid 'local' usage, ...
I don't have a strong opinion. The local is essentially there to avoid possibly overriding variables from mozconfigs. Using names prefixed with e.g. _mozconfig_ would have the same kind of effect.
Attached patch Bug1360571.patch (obsolete) — — Splinter Review
Attachment #8862867 - Attachment is obsolete: true
Attachment #8864816 - Flags: review?(mh+mozilla)
Comment on attachment 8864816 [details] [diff] [review] Bug1360571.patch Review of attachment 8864816 [details] [diff] [review]: ----------------------------------------------------------------- Please change the commit message to say something like Remove "local" bashism from mozconfig_loader mozconfig_loader is invoked with /bin/sh, which may not be bash. We could force mozconfigs to be run with bash instead, but that might be disruptive for existing mozconfigs, and the sole bashism used in mozconfig_loader is rather straightforward to workaround by namespacing the variables.
Attachment #8864816 - Flags: review?(mh+mozilla) → review+
Attached patch Bug1360571.patch (obsolete) — — Splinter Review
I have changed the commit message as requested.
Attachment #8864816 - Attachment is obsolete: true
Attachment #8865776 - Flags: review?(mh+mozilla)
Comment on attachment 8865776 [details] [diff] [review] Bug1360571.patch Review of attachment 8865776 [details] [diff] [review]: ----------------------------------------------------------------- You skipped the explanatory paragraph.
Attachment #8865776 - Flags: review?(mh+mozilla)
Attached patch Bug1360571.patch — — Splinter Review
Attachment #8865776 - Attachment is obsolete: true
Attachment #8865784 - Flags: review?(mh+mozilla)
Comment on attachment 8865784 [details] [diff] [review] Bug1360571.patch Review of attachment 8865784 [details] [diff] [review]: ----------------------------------------------------------------- Thank you
Attachment #8865784 - Flags: review?(mh+mozilla) → review+
Keywords: checkin-needed
Assignee: nobody → petr.sumbera
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6baa8102c70d Remove "local" bashism from mozconfig_loader. r=glandium
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6baa8102c70d
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: