810675 - do not needlessly preprocess some mail/ files

Status: RESOLVED FIXED

(MailNews Core :: Build Config, defect)

Description

[:aceman]

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]

Attachment: 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]

Attachment: 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]

> 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]

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]]

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)]

(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)]

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]

(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)]

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]

Attachment: patch v3

Can we at least check in the part that is fine?

Comment 11

[:aceman]

Attachment: patch v4

One that actually applies ;)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/8e2f580b0622

Comment 13

[Mark Banner (:standard8)]

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]

OK, filed bug 861801.

So this one is finished.