Last Comment Bug 810675 - do not needlessly preprocess some mail/ files
: do not needlessly preprocess some mail/ files
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: Thunderbird 23.0
Assigned To: :aceman
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-11 04:27 PST by :aceman
Modified: 2013-04-15 03:51 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (1.34 KB, patch)
2012-11-11 05:14 PST, :aceman
no flags Details | Diff | Splinter Review
patch v2 (4.01 KB, patch)
2012-11-11 08:17 PST, :aceman
standard8: review-
Details | Diff | Splinter Review
patch v3 (2.04 KB, patch)
2013-03-30 15:57 PDT, :aceman
no flags Details | Diff | Splinter Review
patch v4 (1.96 KB, patch)
2013-03-30 16:05 PDT, :aceman
standard8: review+
Details | Diff | Splinter Review

Description :aceman 2012-11-11 04:27:43 PST
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
Comment 1 :aceman 2012-11-11 05:14:08 PST
Created attachment 680434 [details] [diff] [review]
patch

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 .
Comment 2 :aceman 2012-11-11 08:17:06 PST
Created attachment 680446 [details] [diff] [review]
patch v2

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.
Comment 3 Philip Chee 2012-11-11 10:14:28 PST
> 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.
Comment 4 :aceman 2012-11-11 10:57:45 PST
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.
Comment 5 Florian Quèze [:florian] [:flo] (PTO until August 29th) 2012-11-11 11:42:50 PST
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.
Comment 6 Mark Banner (:standard8) 2012-11-12 01:43:06 PST
(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 7 Mark Banner (:standard8) 2012-11-12 01:45:46 PST
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.
Comment 8 :aceman 2012-11-12 02:43:58 PST
(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.
Comment 9 Mark Banner (:standard8) 2012-11-12 04:02:03 PST
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.
Comment 10 :aceman 2013-03-30 15:57:01 PDT
Created attachment 731577 [details] [diff] [review]
patch v3

Can we at least check in the part that is fine?
Comment 11 :aceman 2013-03-30 16:05:05 PDT
Created attachment 731580 [details] [diff] [review]
patch v4

One that actually applies ;)
Comment 12 Ryan VanderMeulen [:RyanVM] 2013-04-13 05:13:06 PDT
https://hg.mozilla.org/comm-central/rev/8e2f580b0622
Comment 13 Mark Banner (:standard8) 2013-04-15 00:01:08 PDT
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.
Comment 14 :aceman 2013-04-15 03:51:57 PDT
OK, filed bug 861801.

So this one is finished.

Note You need to log in before you can comment on or make changes to this bug.