1445582 - output_searchplugins_list errors should be fatal

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Julien Cristau [:jcristau]]

The 52.7.0esr Italian build suffered from bug 1445278.  That error should have been fatal.

Comment 1

[Julien Cristau [:jcristau]]

Attachment: Bug 1445582 - error out when the searchplugins list is empty or a plugin is missing

Review commit: https://reviewboard.mozilla.org/r/227676/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227676/

Comment 2

[Florian Quèze [:florian]]

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 3

[Julien Cristau [:jcristau]]

Comment on attachment 8958753 [details]
Bug 1445582 - error out when the searchplugins list is empty or a plugin is missing

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227676/diff/1-2/

Comment 4

[Julien Cristau [:jcristau]]

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.

Comment 5

[Nathan Froyd [:froydnj]]

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.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9bdac68266e2

Comment 8

[Julien Cristau [:jcristau]]

Looks like there are no uplift approval flags here, so using the whiteboard.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/031af54f082d

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/5678bfed1348