Closed Bug 1445582 Opened 2 years ago Closed 2 years ago

output_searchplugins_list errors should be fatal

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox-esr52 fixed, firefox59 wontfix, firefox60 fixed, firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- fixed
firefox59 --- wontfix
firefox60 --- fixed
firefox61 --- fixed

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 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 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.
Attachment #8958753 - Flags: review?(core-build-config-reviews)
Blocks: 1445278
No longer depends on: 1445278
Attachment #8958753 - Flags: review?(core-build-config-reviews) → review?(nfroyd)
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
https://hg.mozilla.org/mozilla-central/rev/9bdac68266e2
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Looks like there are no uplift approval flags here, so using the whiteboard.
Whiteboard: [checkin-needed-beta]
You need to log in before you can comment on or make changes to this bug.