Closed
      
        Bug 1445582
      
      
        Opened 7 years ago
          Closed 7 years ago
      
        
    
  
output_searchplugins_list errors should be fatal  
    Categories
(Firefox Build System :: General, defect)
        Firefox Build System
          
        
        
      
        
    
        General
          
        
        
      
        
    Tracking
(firefox-esr52 fixed, firefox59 wontfix, firefox60 fixed, firefox61 fixed)
        RESOLVED
        FIXED
        
    
  
        
            mozilla61
        
    
  
People
(Reporter: jcristau, Assigned: jcristau)
References
Details
Attachments
(1 file)
The 52.7.0esr Italian build suffered from bug 1445278.  That error should have been fatal.
| Comment hidden (mozreview-request) | 
| Comment 2•7 years ago
           | ||
| mozreview-review | ||
Comment on attachment 8958753 [details]
Bug 1445582 - error out when the searchplugins list is empty or a plugin is missing
https://reviewboard.mozilla.org/r/227676/#review233412
::: browser/locales/Makefile.in:42
(Diff revision 1)
>  MOZ_INSTALLER_PATH=$(topsrcdir)/browser/installer/windows
>  
> -SEARCHPLUGINS_FILENAMES := $(shell $(call py_action,output_searchplugins_list,$(srcdir)/search/list.json $(AB_CD)))
> +SEARCHPLUGINS_FILENAMES := $(or $(shell $(call py_action,output_searchplugins_list,$(srcdir)/search/list.json $(AB_CD))), $(error Missing search plugins))
>  SEARCHPLUGINS_PATH := .deps/generated_$(AB_CD)
>  SEARCHPLUGINS_TARGET := libs searchplugins
>  SEARCHPLUGINS := $(foreach plugin,$(addsuffix .xml,$(SEARCHPLUGINS_FILENAMES)),$(or $(wildcard $(srcdir)/searchplugins/$(plugin)),$(warning Missing searchplugin: $(plugin))))
Should this also be an error rather than a warning?
::: browser/locales/Makefile.in:43
(Diff revision 1)
>  
> -SEARCHPLUGINS_FILENAMES := $(shell $(call py_action,output_searchplugins_list,$(srcdir)/search/list.json $(AB_CD)))
> +SEARCHPLUGINS_FILENAMES := $(or $(shell $(call py_action,output_searchplugins_list,$(srcdir)/search/list.json $(AB_CD))), $(error Missing search plugins))
>  SEARCHPLUGINS_PATH := .deps/generated_$(AB_CD)
>  SEARCHPLUGINS_TARGET := libs searchplugins
>  SEARCHPLUGINS := $(foreach plugin,$(addsuffix .xml,$(SEARCHPLUGINS_FILENAMES)),$(or $(wildcard $(srcdir)/searchplugins/$(plugin)),$(warning Missing searchplugin: $(plugin))))
>  # Some locale-specific search plugins may have preprocessor directives, but the
Not really related to this bug, but now that all the search plugins are in the tree, I don't think we still need preprocessing.
| Comment hidden (mozreview-request) | 
| Assignee | ||
| Comment 4•7 years ago
           | ||
| mozreview-review-reply | ||
Comment on attachment 8958753 [details]
Bug 1445582 - error out when the searchplugins list is empty or a plugin is missing
https://reviewboard.mozilla.org/r/227676/#review233412
> Should this also be an error rather than a warning?
Doesn't sound like a bad idea.  Changed now.
| Updated•7 years ago
           | 
          status-firefox59:
          --- → wontfix
          status-firefox60:
          --- → affected
          status-firefox61:
          --- → affected
          status-firefox-esr52:
          --- → affected
| Assignee | ||
| Updated•7 years ago
           | 
        Attachment #8958753 -
        Flags: review?(core-build-config-reviews)
| Updated•7 years ago
           | 
|   | ||
| Updated•7 years ago
           | 
        Attachment #8958753 -
        Flags: review?(core-build-config-reviews) → review?(nfroyd)
|   | ||
| Comment 5•7 years ago
           | ||
| mozreview-review | ||
Comment on attachment 8958753 [details]
Bug 1445582 - error out when the searchplugins list is empty or a plugin is missing
https://reviewboard.mozilla.org/r/227676/#review234808
I don't have a ton of familiarity with this code, but erroring out earlier rather than later seems like a good strategy.
        Attachment #8958753 -
        Flags: review?(nfroyd) → review+
Pushed by jcristau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9bdac68266e2
error out when the searchplugins list is empty or a plugin is missing r=froydnj
|   | ||
| Comment 7•7 years ago
           | ||
| bugherder | ||
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
| Assignee | ||
| Comment 8•7 years ago
           | ||
Looks like there are no uplift approval flags here, so using the whiteboard.
Whiteboard: [checkin-needed-beta]
| Comment 9•7 years ago
           | ||
| bugherder uplift | ||
Whiteboard: [checkin-needed-beta]
| Comment 10•7 years ago
           | ||
| uplift | ||
          You need to log in
          before you can comment on or make changes to this bug.
        
Description
•