880246 - Move EXTRA_PP_COMPONENTS to moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Joey Armstrong [:joey]]

Attachment: move EXTRA_PP_COMPONENTS to moz.build (logic).

Comment 2

[Joey Armstrong [:joey]]

Comment on attachment 760980 [details] [diff] [review]
move EXTRA_PP_COMPONENTS to moz.build (logic).

Add EXTRA_PP_COMPONENTS as a passthrough variable.

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 760980 [details] [diff] [review]
move EXTRA_PP_COMPONENTS to moz.build (logic).

Review of attachment 760980 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +106,5 @@
>  
> +    'EXTRA_PP_COMPONENTS': (StrictOrderingOnAppendList, list, [],
> +        """Javascript XPCOM files.
> +
> +       This variable contains a list of files to preprocess.

Please note that these are installed in the /components directory of the distribution.

Comment 4

[Joey Armstrong [:joey]]

Attachment: move EXTRA_PP_COMPONENTS to moz.build (logic).

Comment 5

[Joey Armstrong [:joey]]

Comment on attachment 761404 [details] [diff] [review]
move EXTRA_PP_COMPONENTS to moz.build (logic).

Added a doc comment about the destination directory, r=gps carried forward.

Comment 6

[Joey Armstrong [:joey]]

Comment on attachment 761404 [details] [diff] [review]
move EXTRA_PP_COMPONENTS to moz.build (logic).

Inbound push:
committed changeset 134818:76bbed327768
https://hg.mozilla.org/integration/mozilla-inbound/rev/76bbed327768

Comment 7

[Joey Armstrong [:joey]]

Attachment: move EXTRA_PP_COMPONENTS: file batch #1

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/76bbed327768

Comment 9

[Joey Armstrong [:joey]]

Attachment: move EXTRA_PP_COMPONENTS to mozbuild (file batch #1)

Comment 10

[Joey Armstrong [:joey]]

Comment on attachment 762258 [details] [diff] [review]
move EXTRA_PP_COMPONENTS to mozbuild (file batch #1)

First batch of converted files.
Try results for b2g and fedora pending.  All others passed earlier.

Comment 11

[Joey Armstrong [:joey]]

Comment on attachment 762258 [details] [diff] [review]
move EXTRA_PP_COMPONENTS to mozbuild (file batch #1)

Try job: https://tbpl.mozilla.org/?tree=Try&rev=d11471046362

Comment 12

[Michael Shal [:mshal]]

Comment on attachment 762258 [details] [diff] [review]
move EXTRA_PP_COMPONENTS to mozbuild (file batch #1)

>diff --git a/toolkit/components/filepicker/moz.build b/toolkit/components/filepicker/moz.build
>--- a/toolkit/components/filepicker/moz.build
>+++ b/toolkit/components/filepicker/moz.build
>@@ -11,8 +11,13 @@ if CONFIG['MOZ_XUL'] and \
>             'nsIFileView.idl',
>         ]
>         CPP_SOURCES += [
>             'nsFileView.cpp',
>         ]
>         EXTRA_COMPONENTS += [
>             'nsFilePicker.js',
>         ]
>+
>+if CONFIG['MOZ_XUL'] and CONFIG['MOZ_WIDGET_TOOLKIT'] not in ('android', 'cocoa', 'qt', 'os2', 'windows'):
>+    EXTRA_PP_COMPONENTS += [
>+        'nsFilePicker.manifest',
>+    ]

Can you just re-use the existing conditional block here? Or do we have a bug on file to fix those up after the automated conversion?

Comment 13

[Gregory Szorc [:gps]]

Please re-use existing conditional blocks as part of manual porting. There was likely only 1 block in the source Makefile.in before we started moz.build conversion. So if we're lazy and produce multiple conditional blocks, it may be a net regression in terms of readability.

Comment 14

[Joey Armstrong [:joey]]

(In reply to Gregory Szorc [:gps] from comment #13)
> Please re-use existing conditional blocks as part of manual porting. There
> was likely only 1 block in the source Makefile.in before we started
> moz.build conversion. So if we're lazy and produce multiple conditional
> blocks, it may be a net regression in terms of readability.

Umm why would joining two conditionals into a single block when we are able to be a problem ?  This was a nit condition from some of the earlier conversions that would have required a followup bug to cleanup.

Comment 15

[Joey Armstrong [:joey]]

(In reply to Joey Armstrong [:joey] from comment #14)
> (In reply to Gregory Szorc [:gps] from comment #13)
> > Please re-use existing conditional blocks as part of manual porting. There
> > was likely only 1 block in the source Makefile.in before we started
> > moz.build conversion. So if we're lazy and produce multiple conditional
> > blocks, it may be a net regression in terms of readability.
> 
> Umm why would joining two conditionals into a single block when we are able
> to be a problem ?  This was a nit condition from some of the earlier
> conversions that would have required a followup bug to cleanup.

Oh NM I missed the other conditional [ thought this comment was from mike ]

Comment 16

[Joey Armstrong [:joey]]

Attachment: move EXTRA_PP_COMPONENTS to mozbuild (file batch #1)

Comment 17

[Joey Armstrong [:joey]]

Comment on attachment 762775 [details] [diff] [review]
move EXTRA_PP_COMPONENTS to mozbuild (file batch #1)

toolkit/components/filepicker/moz.build was an automated addition not manual but can move the conditional into the other block for the inbound push.

Comment 18

[Joey Armstrong [:joey]]

Comment on attachment 762775 [details] [diff] [review]
move EXTRA_PP_COMPONENTS to mozbuild (file batch #1)

Inbound push:
committed changeset 135102:a6d60a556ed4
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6d60a556ed4

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a6d60a556ed4

Comment 20

[jarmstrong]

Attachment: move EXTRA_PP_COMPONENTS to mozbuild (batch #2).

Comment 21

[jarmstrong]

Comment on attachment 764346 [details] [diff] [review]
move EXTRA_PP_COMPONENTS to mozbuild (batch #2).

Conversion patch has baked long enough.  Remove DISABLED_ tokens and add EXPORT_PP_COMPONENTS to the bad variable list.

Comment 22

[Michael Shal [:mshal]]

Comment on attachment 764346 [details] [diff] [review]
move EXTRA_PP_COMPONENTS to mozbuild (batch #2).

Review of attachment 764346 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 23

[jarmstrong]

Attachment: move EXTRA_PP_COMPONENTS to mozbuild (batch #2).

Comment 24

[jarmstrong]

Attachment: move EXTRA_PP_COMPONENTS to mozbuild (batch #2).

Comment 25

[jarmstrong]

Attachment: Cleanup for mozbuild EXTRA_PP_COMPONENTS conversion.

Fixed embedded bug comment, should be cleanup not file batch #2.
Last patch reviewed deleted DISABLED_EXTRA_PP_COMPONENTS.  Refresh patch and upload with corrected comment.  r=mshal carried forward.

Comment 26

[jarmstrong]

Comment on attachment 765538 [details] [diff] [review]
Cleanup for mozbuild EXTRA_PP_COMPONENTS conversion.

Inbound push:
committed changeset 135858:69f37f4cdd2e
https://hg.mozilla.org/integration/mozilla-inbound/rev/69f37f4cdd2e

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/69f37f4cdd2e

Comment 28

[jarmstrong]

Attachment: move EXTRA_PP_COMPONENTS to mozbuild (file batch #3).

Comment 29

[Joey Armstrong [:joey]]

Comment on attachment 768283 [details] [diff] [review]
move EXTRA_PP_COMPONENTS to mozbuild (file batch #3).

Fix one last straggler from the cleanup bug.

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ebac633ae469