1109826 - silence all the WARNING: no preprocessor directives found warning spew

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nathan Froyd [:froydnj]]

No point in having it for certain classes of things, and some things don't even
need it.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 0a - move ImportExport.jsm out of EXTRA_PP_JS_MODULES

It has no preprocessing directives, so it can be safely placed in EXTRA_JS_MODULES.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 0b - don't preprocess content-contextmenu.svg for browser/ themes

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 0c - don't preprocess aboutPrivateBrowsing.js

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 1 - make the preprocessor able to avoid emitting warnings about missing directives

This part is the only controversial part, IMHO.  I wanted the option to be
rather long to dissuade people from using it, but coming up with a good name
was not easy.  Bikeshedding welcome.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: part 2 - don't complain about missing directives for PREF_JS_EXPORTS and DIST_FILES

As the comments say, we preprocess these, but there's no reason that we need to
punish people for having files without preprocessor directives...

Comment 6

[Gregory Szorc [:gps]]

Be warned that some files have preprocessing markers in non-en-US locales. I recall one bug with us removing preprocessing from search engines only to find out it broke l10n repacks.

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: part 3 - don't complain about missing directives for browser/ search plugins

Same logic as in part 2 applies here.

Comment 8

[Michael Shal [:mshal]]

(In reply to Gregory Szorc [:gps] from comment #6)
> Be warned that some files have preprocessing markers in non-en-US locales. I
> recall one bug with us removing preprocessing from search engines only to
> find out it broke l10n repacks.

Ahh, I was wondering about this. It looks like the en-US search plugins no longer have any preprocessor directives. I'm guessing because of things like this it would be too annoying to just have Preprocessor.py return an error instead of a warning if no preprocessor directives are found? That would force us to make sure we only preprocess the things that need preprocessing, but maybe a PITA for these generic preprocess-everything-just-in-case variables like search plugins, DIST_FILES, and PREF_JS_EXPORTS. I seem to recall that the moz.build plans would possibly have per-file attributes? If so maybe we could have Preprocessor.py enforce that it only operates on files with preprocessor directives in them in the future.

I don't have another suggestion for avoiding warning spew in the meantime, though.

Comment 9

[Michael Shal [:mshal]]

Comment on attachment 8534545 [details] [diff] [review]
part 0a - move ImportExport.jsm out of EXTRA_PP_JS_MODULES

ImportExport.jsm does have some preprocessor directives, though they are commented out:

function debug(aMsg) {
//#ifdef DEBUG
  dump("-*- ImportExport.jsm : " + aMsg + "\n");
//#endif
}

fabrice, can we just delete those preprocessor lines? Or uncomment them? I'm a bit concerned that they'll be even more useless with ImportExport.jsm not being preprocessed, since someone might be inclined to uncomment them and then find it doesn't work at all.

Comment 10

[Michael Shal [:mshal]]

Comment on attachment 8534548 [details] [diff] [review]
part 1 - make the preprocessor able to avoid emitting warnings about missing directives

I think a long name is good - I think if we start using it in more than just a small handful of places, we're doing it wrong.

Comment 11

[Michael Shal [:mshal]]

Comment on attachment 8534550 [details] [diff] [review]
part 3 - don't complain about missing directives for browser/ search plugins

It might help to comment that some locale-specific searchplugins do have preprocessor directives as per #c6 (maybe dig up an example?). I was certainly confused just looking at the en-US ones in tree.

Comment 12

[[:fabrice] Fabrice Desré]

(In reply to Michael Shal [:mshal] from comment #9)
> Comment on attachment 8534545 [details] [diff] [review]
> part 0a - move ImportExport.jsm out of EXTRA_PP_JS_MODULES
> 
> ImportExport.jsm does have some preprocessor directives, though they are
> commented out:
> 
> function debug(aMsg) {
> //#ifdef DEBUG
>   dump("-*- ImportExport.jsm : " + aMsg + "\n");
> //#endif
> }
> 
> fabrice, can we just delete those preprocessor lines? Or uncomment them? I'm
> a bit concerned that they'll be even more useless with ImportExport.jsm not
> being preprocessed, since someone might be inclined to uncomment them and
> then find it doesn't work at all.

No, the right fix is to uncomment the #ifdef to make them actually useful. I forgot to uncomment before landing.

Comment 13

[Michael Shal [:mshal]]

Comment on attachment 8534545 [details] [diff] [review]
part 0a - move ImportExport.jsm out of EXTRA_PP_JS_MODULES

Per #c12

Comment 14

[Nathan Froyd [:froydnj]]

Attachment: part 0a - uncomment preprocessing directives in ImportExport.jsm

*shrug* OK, then.

Comment 15

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1dc036c5fdb
https://hg.mozilla.org/integration/mozilla-inbound/rev/299b1bd0cb9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bce1811e3b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c74ca497206
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c797aed238e
https://hg.mozilla.org/integration/mozilla-inbound/rev/201a0263d161

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a1dc036c5fdb
https://hg.mozilla.org/mozilla-central/rev/299b1bd0cb9e
https://hg.mozilla.org/mozilla-central/rev/7bce1811e3b9
https://hg.mozilla.org/mozilla-central/rev/8c74ca497206
https://hg.mozilla.org/mozilla-central/rev/9c797aed238e
https://hg.mozilla.org/mozilla-central/rev/201a0263d161