Closed Bug 1360571 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox Build System :: General, defect)

52 Branch
defect
Not set
normal

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

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

Details

Attachments

(1 file, 3 obsolete files)

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.patchSplinter 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: 4 years ago
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.