Closed Bug 883284 Opened 7 years ago Closed 7 years ago

Move LIBXUL_LIBRARY into moz.build

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla26

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

(Blocks 1 open bug)

Details

Attachments

(13 files)

4.64 KB, patch
mshal
: review+
Details | Diff | Splinter Review
43.36 KB, patch
joey
: review+
Details | Diff | Splinter Review
4.08 KB, patch
joey
: review+
Details | Diff | Splinter Review
64.18 KB, patch
mshal
: review+
Details | Diff | Splinter Review
4.39 KB, patch
mshal
: review+
Details | Diff | Splinter Review
48.60 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
4.30 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
39.59 KB, patch
ted
: review+
Details | Diff | Splinter Review
2.11 KB, patch
ted
: review+
Details | Diff | Splinter Review
60.91 KB, patch
glandium
: review+
Details | Diff | Splinter Review
5.10 KB, patch
glandium
: review+
Details | Diff | Splinter Review
12.89 KB, patch
gps
: review+
Details | Diff | Splinter Review
899 bytes, patch
khuey
: review+
Details | Diff | Splinter Review
Depends on: 896177
Assignee: nobody → Ms2ger
Attached patch Part a: logicSplinter Review
Attachment #792064 - Flags: review?(mshal)
Attachment #792069 - Flags: review?(joey)
Attachment #792070 - Flags: review?(mshal)
Attachment #792070 - Attachment description: Part b (1): automatic moves (d-e) → Part c (1): automatic moves (d-e)
Attachment #792071 - Flags: review?(mshal)
Attachment #792074 - Flags: review?(benjamin)
Attachment #792076 - Flags: review?(benjamin)
Attachment #792079 - Flags: review?(ted)
Attachment #792081 - Flags: review?(mh+mozilla)
Attachment #792082 - Flags: review?(mh+mozilla)
Attachment #792083 - Flags: review?(gps)
Attachment #792085 - Flags: review?(khuey)
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
Attachment #792065 - Flags: review?(joey) → review+
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
Attachment #792069 - Flags: review?(joey) → review+
Attachment #792081 - Flags: review?(mh+mozilla) → review+
Attachment #792082 - Flags: review?(mh+mozilla) → review+
Attachment #792074 - Flags: review?(benjamin) → review+
Attachment #792076 - Flags: review?(benjamin) → review+
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.
Attachment #792064 - Flags: review?(mshal) → review-
Depends on: 882859
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.
Attachment #792064 - Flags: review- → review+
Comment on attachment 792070 [details] [diff] [review]
Part c (1): automatic moves (d-e)

Looks good!
Attachment #792070 - Flags: review?(mshal) → review+
Comment on attachment 792071 [details] [diff] [review]
Part c (2): fixup (d-e)

Hooray removing Makefiles!
Attachment #792071 - Flags: review?(mshal) → review+
Attachment #792079 - Flags: review?(ted) → review+
Comment on attachment 792078 [details] [diff] [review]
Part e (1): automatic moves (k-o)

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

rs=me.
Attachment #792078 - Flags: review?(ted) → review+
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.
Attachment #792083 - Flags: review?(gps) → review+
Depends on: 908142
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.