883284 - Move LIBXUL_LIBRARY into moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Unfortunately, this breaks: https://tbpl.mozilla.org/?tree=Try&rev=d00b27ebbe0a

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part a: logic

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b (1): automatic moves (a-c)

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b (2): fixup (a-c)

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part c (1): automatic moves (d-e)

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part c (2): fixup (d-e)

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part d (1): automatic moves (f-j)

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part d (2): fixup (f-j)

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part e (1): automatic moves (k-o)

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part e (2): fixup (k-o)

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part f (1): automatic moves (p-z)

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part f (2): fixup (p-z)

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part g: manual moves

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part h: disallow in makefiles

Comment 14

[Joey Armstrong [:joey]]

Comment on attachment 792065 [details] [diff] [review]
Part b (1): automatic moves (a-c)

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

Moving var=1 => var=True looks fine.
Patch may need a rebase.


hg qimport ~/LIBXUL_LIBRARY-convert-a-c
hg qpush -a
applying LIBXUL_LIBRARY-convert-a-c
patching file content/base/src/Makefile.in
Hunk #1 FAILED at 6
1 out of 1 hunks FAILED -- saving rejects to file content/base/src/Makefile.in.rej
patching file content/base/src/moz.build
Hunk #1 FAILED at 166
1 out of 1 hunks FAILED -- saving rejects to file content/base/src/moz.build.rej
patching file content/canvas/src/Makefile.in
Hunk #1 FAILED at 6
1 out of 1 hunks FAILED -- saving rejects to file content/canvas/src/Makefile.in.rej
patching file content/canvas/src/moz.build
Hunk #1 FAILED at 69
1 out of 1 hunks FAILED -- saving rejects to file content/canvas/src/moz.build.rej
patching file content/events/src/Makefile.in
Hunk #1 FAILED at 6

Comment 15

[Joey Armstrong [:joey]]

Comment on attachment 792069 [details] [diff] [review]
Part b (2): fixup (a-c)

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

Moving comments from one file to another look ok.

# ?- bug 883284 added the '# nsDependentJSString comment, bug 882859 removed it -?

883284: content/xslt/src/xslt/Makefile.in 
+# For nsDependentJSString

882859: content/xslt/src/xslt/Makefile.in
-# For nsDependentJSString

Comment 16

[Michael Shal [:mshal]]

Comment on attachment 792064 [details] [diff] [review]
Part a: logic

>diff --git a/python/mozbuild/mozbuild/test/backend/test_recursivemake.py b/python/mozbuild/mozbuild/test/backend/test_recursivemake.py
>--- a/python/mozbuild/mozbuild/test/backend/test_recursivemake.py
>+++ b/python/mozbuild/mozbuild/test/backend/test_recursivemake.py
>@@ -199,16 +199,19 @@
>                 'HOST_CSRCS += foo.c',
>             ],
>             'HOST_LIBRARY_NAME': [
>                 'HOST_LIBRARY_NAME := host_bar',
>             ],
>             'LIBRARY_NAME': [
>                 'LIBRARY_NAME := lib_name',
>             ],
>+            'LIBXUL_LIBRARY': [
>+                'LIBXUL_LIBRARY := 1',
>+            ],

This new test doesn't pass for me. I'm getting:

TEST-UNEXPECTED-FAIL | /home/mshal/mozilla-central-git/python/mozbuild/mozbuild/test/backend/test_recursivemake.py | line 228, test_variable_passthru: Lists differ: ['LIBXUL_LIBRARY := True'] != [u'LIBXUL_LIBRARY := 1']

First differing element 0:
LIBXUL_LIBRARY := True
LIBXUL_LIBRARY := 1

- ['LIBXUL_LIBRARY := True']
?                     ^^^^

+ [u'LIBXUL_LIBRARY := 1']
?  +                   ^

The other bool passthru variable, NO_DIST_INSTALL, has some custom code in emitter.py:

        if sandbox['NO_DIST_INSTALL']:
            passthru.variables['NO_DIST_INSTALL'] = '1'

I'm guessing we need something similar here? Though I think the True -> 1 logic would be better off in the make backend, rather than the emitter. Not sure that really matters, though.

Comment 17

[Michael Shal [:mshal]]

Comment on attachment 792064 [details] [diff] [review]
Part a: logic

According to Ms2ger, the bool handling is being done as part of bug 882859 (and makes the test case work). I've updated the bug dependencies, so assuming that goes in first, this is ready to go.

Comment 18

[Michael Shal [:mshal]]

Comment on attachment 792070 [details] [diff] [review]
Part c (1): automatic moves (d-e)

Looks good!

Comment 19

[Michael Shal [:mshal]]

Comment on attachment 792071 [details] [diff] [review]
Part c (2): fixup (d-e)

Hooray removing Makefiles!

Comment 20

[(not currently active) Ted Mielczarek]

Comment on attachment 792078 [details] [diff] [review]
Part e (1): automatic moves (k-o)

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

rs=me.

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 792083 [details] [diff] [review]
Part g: manual moves

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

LGTM.

::: gfx/graphite2/src/moz.build
@@ +9,5 @@
> +if CONFIG['OS_TARGET'] != 'WINNT':
> +    LIBXUL_LIBRARY = True
> +else:
> +    # FORCE_STATIC_LIB = True
> +    pass

This is somewhat ugly, but sadly necessary for the short term. Thanks for commenting FORCE_STATIC_LIB to aid future conversion.

::: tools/profiler/moz.build
@@ +5,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  if CONFIG['MOZ_ENABLE_PROFILER_SPS']:
>      FAIL_ON_WARNINGS = not CONFIG['_MSC_VER']
>      

Nit: fix whitespace while you're here.

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/142e45f98989
https://hg.mozilla.org/mozilla-central/rev/418307f9dfe7
https://hg.mozilla.org/mozilla-central/rev/7ef14b724957
https://hg.mozilla.org/mozilla-central/rev/043b46d19b1c
https://hg.mozilla.org/mozilla-central/rev/3338bd68d9a4
https://hg.mozilla.org/mozilla-central/rev/1da53c9cadac
https://hg.mozilla.org/mozilla-central/rev/94f73ac4eca6
https://hg.mozilla.org/mozilla-central/rev/71f2b4a95d03