Closed Bug 1360571 Opened 6 years ago Closed 6 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: 6 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.