Closed Bug 1109826 Opened 5 years ago Closed 5 years ago

silence all the WARNING: no preprocessor directives found warning spew

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla37

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(6 files, 1 obsolete file)

No point in having it for certain classes of things, and some things don't even
need it.
It has no preprocessing directives, so it can be safely placed in EXTRA_JS_MODULES.
Attachment #8534545 - Flags: review?(mshal)
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.
Attachment #8534548 - Flags: review?(mshal)
As the comments say, we preprocess these, but there's no reason that we need to
punish people for having files without preprocessor directives...
Attachment #8534549 - Flags: review?(mshal)
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.
Same logic as in part 2 applies here.
Attachment #8534550 - Flags: review?(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 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.
Flags: needinfo?(fabrice)
Attachment #8534546 - Flags: review?(mshal) → review+
Attachment #8534547 - Flags: review?(mshal) → review+
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.
Attachment #8534548 - Flags: review?(mshal) → review+
Attachment #8534549 - Flags: review?(mshal) → review+
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.
Attachment #8534550 - Flags: review?(mshal) → review+
(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.
Flags: needinfo?(fabrice)
Comment on attachment 8534545 [details] [diff] [review]
part 0a - move ImportExport.jsm out of EXTRA_PP_JS_MODULES

Per #c12
Attachment #8534545 - Flags: review?(mshal) → review-
*shrug* OK, then.
Attachment #8534545 - Attachment is obsolete: true
Attachment #8535187 - Flags: review?(fabrice)
Attachment #8535187 - Flags: review?(fabrice) → review+
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.