Closed Bug 810675 Opened 9 years ago Closed 8 years ago
do not needlessly preprocess some mail/ files
mail/components/im/all-im.js: WARNING: no preprocessor directives found mail/branding/nightly/locales/en-US/brand.properties: WARNING: no useful preprocessor directives found mail/branding/nightly/thunderbird-branding.js: WARNING: no preprocessor directives found mail/steel/steelApplication.manifest: WARNING: no preprocessor directives found
I couldn't find out why all-im.js, thunderbird-branding.js and steelApplication.manifest get preprocessed, they are not listed in a jar.mn file. It may be some deeper Makefile process. So I at least fix brand.properties .
I solve the warning in the pref .js file by including a dummy directive because we still want the preprocessor pass due to the --line-endings=crlf option on Win. Thanks to florian for hints.
> I solve the warning in the pref .js file by including a dummy directive because we > still want the preprocessor pass due to the --line-endings=crlf option on Win. I think you should fix Preprocessor.py instead of this hack.
But I think the preprocessor warning is right. On Linux/Mac we do not pass that --line-endings=crlf option so the preprocessor probably has nothing to do so it issues that warning. We can't fix the preprocessor. We would need to change the rules.mk file (add some PREFS_JS_NONPP_EXPORTS) so that we know exactly for which files to call the preprocessor and which can be skipped. I.e. PREFS_JS_EXPORTS -> preprocess PREFS_JS_NONPP_EXPORTS -> preprocess if on WinNT, otherwise not.
The only fix I see for Preprocessor.py would be to add a command line flag to ignore that warning, and to pass that flag from rules.mk for non-Windows OSes.
(In reply to :aceman from comment #4) > But I think the preprocessor warning is right. On Linux/Mac we do not pass > that --line-endings=crlf option so the preprocessor probably has nothing to > do so it issues that warning. So, let me check I'm understanding this correctly. We're currently preprocessing the files, the side-effect of it is that they have the correct line endings on Windows? That's something I don't think I care about - these files get packed up into omni.ja and aren't seen by the majority of users. I don't think we need those line endings to be correct, the builds should cope fine without those being preprocessed.
Comment on attachment 680446 [details] [diff] [review] patch v2 r- because of the previous comment. It'd also be good to check if the official branding in other-licenses has the same warnings.
Attachment #680446 - Flags: review?(mbanner) → review-
(In reply to Mark Banner (:standard8) from comment #6) > So, let me check I'm understanding this correctly. We're currently > preprocessing the files, the side-effect of it is that they have the correct > line endings on Windows? We also preprocess them because most of those files do have preprocessor directives. Only the ~3 mentioned here do not have any therefore trigger the warning. So ignoring the crlf issue does not help much. We'd still need to have a new config variable (PREFS_JS_NONPP_EXPORTS) for files that do not need preprocessing and then we would differ from m-c rules.mk. But m-c also has these warnings for some files.
OK, can I suggest you file a core bug there and see what they suggest doing (new config or dummy directives)? I'd be happy to go with either way I think.
Can we at least check in the part that is fine?
One that actually applies ;)
Attachment #731580 - Flags: review?(mbanner) → review+
For the additional pref file issue, I'd suggest we file a bug in core stating that pref files may give this warning, and split this bug into a new one that states specifically about the pref file issue.
OK, filed bug 861801. So this one is finished.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [please leave open]
Target Milestone: --- → Thunderbird 23.0
You need to log in before you can comment on or make changes to this bug.