1228467 - Transform "no preprocessing directives" warnings into errors

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Don't preprocess chrome files without preprocessor directives

FWIW, I checked the corresponding files in all locales, and they have no preprocessor directives either (which is unsurprising)

Comment 2

[Mike Hommey [:glandium]]

Attachment: Don't preprocess dom/base/UseCounters.conf

There is preprocessing directive in the file. Also, the current support for
preprocessing doesn't take into account any DEFINES that would be set in the
corresponding moz.build, which would be quite surprising would someone want
to use preprocessing on that file in the future.

Comment 3

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #2)
> Created attachment 8692927 [details] [diff] [review]
> Don't preprocess dom/base/UseCounters.conf
> 
> There is preprocessing directive in the file. Also, the current support for
          ^ no

Comment 4

[Mike Hommey [:glandium]]

Attachment: Don't preprocess layout/tools/reftest/reftest-cmdline.manifest

Comment 5

[Mike Hommey [:glandium]]

Attachment: Make the preprocessor error out for "no preprocessing directives" instead of warning

This will undoubtedly break c-c, but it's not really something hard to overcome, see the 3 other patches.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8692927 [details] [diff] [review]
Don't preprocess dom/base/UseCounters.conf

Review of attachment 8692927 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is OK, given that we usually use prefs rather than defines for DOM features, and we're unlikely to gate use counters (or features!) on particular platforms, which would be the other major reason for #ifdefs, judging from a skim through dom/

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8692926 [details] [diff] [review]
Don't preprocess chrome files without preprocessor directives

Review of attachment 8692926 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/locales/jar.mn
@@ +8,5 @@
>  % locale b2g-l10n @AB_CD@ %locale/@AB_CD@/b2g-l10n/
>  
>  % override chrome://global/locale/aboutCertError.dtd     chrome://b2g-l10n/locale/aboutCertError.dtd
>  % override chrome://global/locale/appstrings.properties  chrome://b2g-l10n/locale/appstrings.properties
> +  locale/@AB_CD@/b2g-l10n/aboutCertError.dtd       (%chrome/overrides/aboutCertError.dtd)

What if a specific locale contains preprocessing directives? I have a very vague recollection of having to add a '*' somewhere because this occurs in the wild. I guess we won't know until we do repacks in a few weeks/months :/

Comment 8

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #7)
> ::: b2g/locales/jar.mn
> @@ +8,5 @@
> >  % locale b2g-l10n @AB_CD@ %locale/@AB_CD@/b2g-l10n/
> >  
> >  % override chrome://global/locale/aboutCertError.dtd     chrome://b2g-l10n/locale/aboutCertError.dtd
> >  % override chrome://global/locale/appstrings.properties  chrome://b2g-l10n/locale/appstrings.properties
> > +  locale/@AB_CD@/b2g-l10n/aboutCertError.dtd       (%chrome/overrides/aboutCertError.dtd)
> 
> What if a specific locale contains preprocessing directives? I have a very
> vague recollection of having to add a '*' somewhere because this occurs in
> the wild. I guess we won't know until we do repacks in a few weeks/months :/

As mentioned in comment 1, I checked all locales already.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/235a270a4c2a
https://hg.mozilla.org/integration/mozilla-inbound/rev/9125a6b6c61d
https://hg.mozilla.org/integration/mozilla-inbound/rev/18f833a960d3
https://hg.mozilla.org/integration/mozilla-inbound/rev/778332f8017d

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/235a270a4c2a
https://hg.mozilla.org/mozilla-central/rev/9125a6b6c61d
https://hg.mozilla.org/mozilla-central/rev/18f833a960d3
https://hg.mozilla.org/mozilla-central/rev/778332f8017d