The default bug view has changed. See this FAQ.

Move CSRCS to moz.build

RESOLVED FIXED in mozilla28

Status

()

Core
Build Config
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: joey, Assigned: glandium)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(10 attachments, 21 obsolete attachments)

5.39 KB, patch
gps
: review+
Details | Diff | Splinter Review
8.21 KB, patch
Details | Diff | Splinter Review
4.18 KB, patch
standard8
: review+
jcranmer
: checkin+
Details | Diff | Splinter Review
18.78 KB, patch
Details | Diff | Splinter Review
16.07 KB, patch
Details | Diff | Splinter Review
4.23 KB, patch
mshal
: review+
Details | Diff | Splinter Review
6.04 KB, patch
Details | Diff | Splinter Review
7.65 KB, patch
Details | Diff | Splinter Review
2.12 KB, patch
mshal
: review+
Details | Diff | Splinter Review
23.41 KB, patch
mshal
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Reporter)

Updated

4 years ago
No longer depends on: 870370
(Reporter)

Updated

4 years ago
Blocks: 870407
(Reporter)

Updated

4 years ago
No longer blocks: 870407
(Reporter)

Updated

4 years ago
Blocks: 870408
(Reporter)

Updated

4 years ago
No longer blocks: 870408
(Reporter)

Comment 1

4 years ago
Created attachment 751872 [details] [diff] [review]
move CSRCS to moz.build (logic)
(Reporter)

Updated

4 years ago
Assignee: nobody → joey
(Reporter)

Comment 2

4 years ago
Comment on attachment 751872 [details] [diff] [review]
move CSRCS to moz.build (logic)

Add CSRCS as a passthrough variable.
Attachment #751872 - Flags: review?(gps)

Updated

4 years ago
Attachment #751872 - Flags: review?(gps) → review+
(Reporter)

Updated

4 years ago
Whiteboard: [leave open]
(Reporter)

Comment 3

4 years ago
Inbound push:

changeset: 132691:e3faa44c33e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3faa44c33e4
https://hg.mozilla.org/mozilla-central/rev/e3faa44c33e4
(Reporter)

Comment 5

4 years ago
Created attachment 753535 [details] [diff] [review]
move to moz.build (config)
(Reporter)

Comment 6

4 years ago
Comment on attachment 753535 [details] [diff] [review]
move to moz.build (config)

First batch of CSRCS variables to convert.
intl/uconv/moz.build - added missing tools/ directory to compile umaptable.
Try job pending.
Attachment #753535 - Flags: review?(mshal)
(Reporter)

Comment 7

4 years ago
Created attachment 753559 [details] [diff] [review]
move to moz.build (config batch #1)
(Reporter)

Updated

4 years ago
Attachment #753535 - Attachment is obsolete: true
Attachment #753535 - Flags: review?(mshal)
(Reporter)

Comment 8

4 years ago
Comment on attachment 753559 [details] [diff] [review]
move to moz.build (config batch #1)

Same patch as before (initial set of converted CSRCS).
Removed 'tools' from intl/uconv/moz.build.

Another bug to be opened, platform filtering or linkage problems need to be resolved.
Attachment #753559 - Flags: review?(mshal)
Comment on attachment 753559 [details] [diff] [review]
move to moz.build (config batch #1)

>-SIMPLE_PROGRAMS	= $(CSRCS:.c=$(BIN_SUFFIX))
>+SIMPLE_PROGRAMS	= $(DISABLED_CSRCS:.c=$(BIN_SUFFIX))

As we discussed in person, can't you still use CSRCS here? The deferred resolution should work to pick up CSRCS from backend.mk, until we move SIMPLE_PROGRAMS to moz.build

>diff --git a/modules/libmar/tool/moz.build b/modules/libmar/tool/moz.build
>--- a/modules/libmar/tool/moz.build
>+++ b/modules/libmar/tool/moz.build
>@@ -3,8 +3,12 @@
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> MODULE = 'mar'
> 
> if CONFIG['MOZ_ENABLE_SIGNMAR']:
>     PROGRAM = 'signmar'
>+
>+# bug 875549: Temporarly break dep on HOST_CSRCS= so CSRCS can convert.
>+host_csrcs = ['mar.c']
>+CSRCS += host_csrcs

Why does the patch for bug 870406 have changes for bug 875549?

>diff --git a/netwerk/dns/moz.build b/netwerk/dns/moz.build
>--- a/netwerk/dns/moz.build
>+++ b/netwerk/dns/moz.build
>@@ -16,8 +16,9 @@ XPIDL_SOURCES += [
> XPIDL_MODULE = 'necko_dns'
> 
> MODULE = 'necko'
> 
> EXPORTS.mozilla.net += [
>     'DNS.h',
> ]
> 
>+CSRCS += ['nameprep.c', 'punycode.c', 'race.c']

Please use the standard one-per-line moz.build convention (here and everywhere else in this patch) -

CSRCS += [
    'nameprep.c',
    'punycode.c',
    'race.c',
]
Attachment #753559 - Flags: review?(mshal) → feedback+
(Reporter)

Comment 10

4 years ago
(In reply to Michael Shal [:mshal] from comment #9)
> Comment on attachment 753559 [details] [diff] [review]
> move to moz.build (config batch #1)
> 
> >-SIMPLE_PROGRAMS	= $(CSRCS:.c=$(BIN_SUFFIX))
> >+SIMPLE_PROGRAMS	= $(DISABLED_CSRCS:.c=$(BIN_SUFFIX))
> 
> As we discussed in person, can't you still use CSRCS here? The deferred
> resolution should work to pick up CSRCS from backend.mk, until we move
> SIMPLE_PROGRAMS to moz.build

CSRCS was empty on an earlier build attempt.


> 
> >diff --git a/modules/libmar/tool/moz.build b/modules/libmar/tool/moz.build
> >--- a/modules/libmar/tool/moz.build
> >+++ b/modules/libmar/tool/moz.build
> >@@ -3,8 +3,12 @@
> > # This Source Code Form is subject to the terms of the Mozilla Public
> > # License, v. 2.0. If a copy of the MPL was not distributed with this
> > # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > 
> > MODULE = 'mar'
> > 
> > if CONFIG['MOZ_ENABLE_SIGNMAR']:
> >     PROGRAM = 'signmar'
> >+
> >+# bug 875549: Temporarly break dep on HOST_CSRCS= so CSRCS can convert.
> >+host_csrcs = ['mar.c']
> >+CSRCS += host_csrcs
> 
> Why does the patch for bug 870406 have changes for bug 875549?

[info: 875549 is a followup bug]
This will allow the conversions to remain independent.

HOST_CSRCS is defined by Makefile.in.  moz.build::CSRCS will not be able to access that value because backend.mk will be included before HOST_CSRCS is defined.  To grab that value the HOST_CSRCS conversion would come into play which then requires landing both at the same time {increasing the chance of removal due to test failures :)}.

> 
> >diff --git a/netwerk/dns/moz.build b/netwerk/dns/moz.build
> >--- a/netwerk/dns/moz.build
> >+++ b/netwerk/dns/moz.build
> >@@ -16,8 +16,9 @@ XPIDL_SOURCES += [
> > XPIDL_MODULE = 'necko_dns'
> > 
> > MODULE = 'necko'
> > 
> > EXPORTS.mozilla.net += [
> >     'DNS.h',
> > ]
> > 
> >+CSRCS += ['nameprep.c', 'punycode.c', 'race.c']
> 
> Please use the standard one-per-line moz.build convention (here and
> everywhere else in this patch) -

todo

Do you know if the standards are written down anywhere ?  If not we probably need a section for this in the style guide:
https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style



> CSRCS += [
>     'nameprep.c',
>     'punycode.c',
>     'race.c',
> ]
(Reporter)

Comment 11

4 years ago
(In reply to Joey Armstrong [:joey] from comment #10)
> (In reply to Michael Shal [:mshal] from comment #9)
> > Comment on attachment 753559 [details] [diff] [review]
> > move to moz.build (config batch #1)
> > 
> > >-SIMPLE_PROGRAMS	= $(CSRCS:.c=$(BIN_SUFFIX))
> > >+SIMPLE_PROGRAMS	= $(DISABLED_CSRCS:.c=$(BIN_SUFFIX))
> > 
> > As we discussed in person, can't you still use CSRCS here? The deferred
> > resolution should work to pick up CSRCS from backend.mk, until we move
> > SIMPLE_PROGRAMS to moz.build

Oh also bug 632224 is already open for the SIMPLE_PROGRAMS conversion so this will go away in the near future.
(Reporter)

Comment 12

4 years ago
try results: https://tbpl.mozilla.org/?tree=Try&rev=756ae8cc8efb
(Reporter)

Comment 13

4 years ago
Created attachment 753599 [details] [diff] [review]
move to moz.build (config batch #1)
(Reporter)

Updated

4 years ago
Attachment #753559 - Attachment is obsolete: true
(Reporter)

Comment 14

4 years ago
Comment on attachment 753599 [details] [diff] [review]
move to moz.build (config batch #1)

nit cleanup, split file lists into one per line.
Attachment #753599 - Flags: review?(mshal)
(Reporter)

Comment 15

4 years ago
Try results for config edits, batch #1:
https://tbpl.mozilla.org/?tree=Try&rev=756ae8cc8efb
Comment on attachment 753599 [details] [diff] [review]
move to moz.build (config batch #1)


>diff --git a/netwerk/dns/moz.build b/netwerk/dns/moz.build
>--- a/netwerk/dns/moz.build
>+++ b/netwerk/dns/moz.build
>@@ -16,8 +16,13 @@ XPIDL_SOURCES += [
> XPIDL_MODULE = 'necko_dns'
> 
> MODULE = 'necko'
> 
> EXPORTS.mozilla.net += [
>     'DNS.h',
> ]
> 
>+CSRCS += [
>+    'nameprep.c',
>+    'punycode.c',
>+    'race.c',
>+    ]

nit: It looks like we align the closing ']' with the first character of the variable name, rather than indenting it. This shows up in a few other places in this patch.
Attachment #753599 - Flags: review?(mshal) → review+
(Reporter)

Comment 17

4 years ago
Created attachment 753970 [details] [diff] [review]
move to moz.build (config batch #1)
(Reporter)

Comment 18

4 years ago
Created attachment 753973 [details] [diff] [review]
CSRCS variable conversion, first batch of files

File batch #1 - nit fixes, align ']'.  Will have to revisit this post conversion, default auto-indent for python at least with emacs aligns with the value list -vs- var name definition. r=mshal carried forward.
Attachment #753599 - Attachment is obsolete: true
Attachment #753970 - Attachment is obsolete: true
(Reporter)

Comment 19

4 years ago
push to inbound:
https://bugzilla.mozilla.org/attachment.cgi?id=753973

changeset:   132965:773937adfcac
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/773937adfcac
(Reporter)

Comment 20

4 years ago
Comment on attachment 753973 [details] [diff] [review]
CSRCS variable conversion, first batch of files

re-post to attach info to the appropriate patch.

push to inbound:
https://bugzilla.mozilla.org/attachment.cgi?id=753973

changeset:   132965:773937adfcac
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/773937adfcac
https://hg.mozilla.org/mozilla-central/rev/773937adfcac
Created attachment 754485 [details] [diff] [review]
Migrate CSRCS in comm-central

The files in mailnews/mapi/mapihook/build are autogenerated by the MIDL, so I'm slightly concerned about moving it to moz.build. It does appear to work on try though:
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=3c22399a614c
Attachment #754485 - Flags: review?(mbanner)
(Reporter)

Comment 23

4 years ago
Created attachment 756101 [details] [diff] [review]
move CSRCS to moz.build (file bundle #2)

convert media/ and memory/ makefiles.
Attachment #756101 - Flags: review?(mshal)
Comment on attachment 756101 [details] [diff] [review]
move CSRCS to moz.build (file bundle #2)

>diff --git a/memory/jemalloc/moz.build b/memory/jemalloc/moz.build
>--- a/memory/jemalloc/moz.build
>+++ b/memory/jemalloc/moz.build
>@@ -1,8 +1,39 @@
> # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> # vim: set filetype=python:
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> MODULE = 'jemalloc'
> 
>+CSRCS += [
>+    'arena.c',
>+    'atomic.c',
>+    'base.c',
>+    'bitmap.c',
>+    'chunk.c',
>+    'chunk_dss.c',
>+    'chunk_mmap.c',
>+    'ckh.c',
>+    'ctl.c',
>+    'extent.c',
>+    'hash.c',
>+    'huge.c',
>+    'jemalloc.c',
>+    'mb.c',
>+    'mutex.c',
>+    'prof.c',
>+    'quarantine.c',
>+    'rtree.c',
>+    'stats.c',
>+    'tcache.c',
>+    'tsd.c',
>+    'util.c',
>+]
>+
>+# Only OSX needs the zone allocation implementation,
>+# but only if replace-malloc is not enabled.
>+if CONFIG['OS_TARGET'] == Darwin or CONFIG['MOZ_REPLACE_MALLOC'] == 'Darwin':
>+    CSRCS += [
>+        'zone.c',
>+    ]

I don't think this if statement is correct. The first Darwin is not in quotes, so wouldn't that be a variable? I think this should be:

if CONFIG['OS_TARGET'] == 'Darwin' and not CONFIG['MOZ_REPLACE_MALLOC']:
    CSRCS += [
        'zone.c',
    ]

(Please double check)

>diff --git a/memory/jemalloc/src/moz.build b/memory/jemalloc/src/moz.build
>new file mode 100644
>--- /dev/null
>+++ b/memory/jemalloc/src/moz.build
>@@ -0,0 +1,35 @@
>+# -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
>+# vim: set filetype=python:
>+# This Source Code Form is subject to the terms of the Mozilla Public
>+# License, v. 2.0. If a copy of the MPL was not distributed with this
>+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
>+
>+CSRCS += [
>+    'src/arena.c',
>+    'src/atomic.c',
>+    'src/base.c',
>+    'src/bitmap.c',
>+    'src/chunk.c',
>+    'src/chunk_dss.c',
>+    'src/chunk_mmap.c',
>+    'src/ckh.c',
>+    'src/ctl.c',
>+    'src/extent.c',
>+    'src/hash.c',
>+    'src/huge.c',
>+    'src/jemalloc.c',
>+    'src/mb.c',
>+    'src/mutex.c',
>+    'src/prof.c',
>+    'src/quarantine.c',
>+    'src/rtree.c',
>+    'src/stats.c',
>+    'src/tcache.c',
>+    'src/tsd.c',
>+    'src/util.c',
>+]
>+
>+if CONFIG['ABI'] == 'macho':
>+    CSRCS += [
>+        'src/zone.c',
>+    ]

Is ABI an actual config variable? It doesn't show up in my autoconf.mk, but maybe it is platform specific.

Everything else looks good to me - I can r+ when the if-statement is fixed.
Attachment #756101 - Flags: review?(mshal) → feedback+
(Reporter)

Comment 25

4 years ago
Comment on attachment 756101 [details] [diff] [review]
move CSRCS to moz.build (file bundle #2)

try results: https://tbpl.mozilla.org/?tree=Try&rev=364948d0fee3
(Reporter)

Comment 26

4 years ago
(In reply to Michael Shal [:mshal] from comment #24)
> Comment on attachment 756101 [details] [diff] [review]
> move CSRCS to moz.build (file bundle #2)
> 
> >diff --git a/memory/jemalloc/moz.build b/memory/jemalloc/moz.build
> >--- a/memory/jemalloc/moz.build
> >+++ b/memory/jemalloc/moz.build
> >@@ -1,8 +1,39 @@
> > # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> > # vim: set filetype=python:
> > # This Source Code Form is subject to the terms of the Mozilla Public
> > # License, v. 2.0. If a copy of the MPL was not distributed with this
> > # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > 
> > MODULE = 'jemalloc'
> > 
> >+CSRCS += [
> >+    'arena.c',
> >+    'atomic.c',
> >+    'base.c',
> >+    'bitmap.c',
> >+    'chunk.c',
> >+    'chunk_dss.c',
> >+    'chunk_mmap.c',
> >+    'ckh.c',
> >+    'ctl.c',
> >+    'extent.c',
> >+    'hash.c',
> >+    'huge.c',
> >+    'jemalloc.c',
> >+    'mb.c',
> >+    'mutex.c',
> >+    'prof.c',
> >+    'quarantine.c',
> >+    'rtree.c',
> >+    'stats.c',
> >+    'tcache.c',
> >+    'tsd.c',
> >+    'util.c',
> >+]
> >+
> >+# Only OSX needs the zone allocation implementation,
> >+# but only if replace-malloc is not enabled.
> >+if CONFIG['OS_TARGET'] == Darwin or CONFIG['MOZ_REPLACE_MALLOC'] == 'Darwin':
> >+    CSRCS += [
> >+        'zone.c',
> >+    ]
> 
> I don't think this if statement is correct. The first Darwin is not in
> quotes, so wouldn't that be a variable? I think this should be:
> 
> if CONFIG['OS_TARGET'] == 'Darwin' and not CONFIG['MOZ_REPLACE_MALLOC']:
>     CSRCS += [
>         'zone.c',
>     ]
> 
> (Please double check)

Hmmm, the file is not processed while using --enable-jemalloc but --enable-dmd was enough to display the error.  To be fixed shortly.

> 
> >diff --git a/memory/jemalloc/src/moz.build b/memory/jemalloc/src/moz.build
> >new file mode 100644
> >--- /dev/null
> >+++ b/memory/jemalloc/src/moz.build
> >@@ -0,0 +1,35 @@
>
> >+if CONFIG['ABI'] == 'macho':
> >+    CSRCS += [
> >+        'src/zone.c',
> >+    ]
> 
> Is ABI an actual config variable? It doesn't show up in my autoconf.mk, but
> maybe it is platform specific.

Will convert verbatim and open another bug to check on the ABI logic.  fyi> ABI=[@abi@] if searching config was not case insensitive.
 
> Everything else looks good to me - I can r+ when the if-statement is fixed.

ok
(Reporter)

Updated

4 years ago
See Also: → bug 878866
(Reporter)

Comment 27

4 years ago
Created attachment 757468 [details] [diff] [review]
move CSRCS to mozbuild (config batch #2)
(Reporter)

Updated

4 years ago
Attachment #756101 - Attachment is obsolete: true
(Reporter)

Comment 28

4 years ago
Comment on attachment 757468 [details] [diff] [review]
move CSRCS to mozbuild (config batch #2)

Quote 'Darwin' in the jemalloc conditional.
Opened bug 870406 to verify ABI/config usage.

last try results: https://tbpl.mozilla.org/?tree=Try&rev=364948d0fee3
Attachment #757468 - Flags: review?(mshal)
Comment on attachment 757468 [details] [diff] [review]
move CSRCS to mozbuild (config batch #2)

>diff --git a/media/libcubeb/src/moz.build b/media/libcubeb/src/moz.build
>--- a/media/libcubeb/src/moz.build
>+++ b/media/libcubeb/src/moz.build
>@@ -1,8 +1,46 @@
> # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> # vim: set filetype=python:
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> MODULE = 'cubeb'
> 
>+CSRCS += [
>+    'cubeb.c',
>+]
>+
>+if CONFIG['MOZ_ALSA']:
>+    CSRCS += [
>+        'cubeb_alsa.c',
>+    ]
>+
>+if CONFIG['MOZ_PULSEAUDIO']:
>+    CSRCS += [
>+        'cubeb_pulse.c',
>+    ]
>+
>+if CONFIG['OS_ARCH'] == 'OpenBSD':
>+    CSRCS += [
>+        'cubeb_sndio.c',
>+    ]
>+
>+

nit: Remove the extra line of whitespace

>+# Only OSX needs the zone allocation implementation,
>+# but only if replace-malloc is not enabled.
>+if CONFIG['OS_TARGET'] == 'Darwin' or CONFIG['MOZ_REPLACE_MALLOC'] == 'Darwin':
>+    CSRCS += [
>+        'zone.c',
>+    ]

This still looks wrong to me. MOZ_REPLACE_MALLOC won't get set to 'Darwin' - it is either '1' or not defined. I think you want:

if CONFIG['OS_TARGET'] == 'Darwin' and not CONFIG['MOZ_REPLACE_MALLOC']:

Once this is fixed I can r+.
Attachment #757468 - Flags: review?(mshal) → feedback+
(Reporter)

Comment 30

4 years ago
Created attachment 757539 [details] [diff] [review]
move CSRCS to mozbuild (config batch #2)
(Reporter)

Updated

4 years ago
Attachment #757468 - Attachment is obsolete: true
(Reporter)

Comment 31

4 years ago
Comment on attachment 757539 [details] [diff] [review]
move CSRCS to mozbuild (config batch #2)

nit fixes
Attachment #757539 - Flags: review?(mshal)
Comment on attachment 757539 [details] [diff] [review]
move CSRCS to mozbuild (config batch #2)

Looks good to me now! If try is happy, I think it's ready.
Attachment #757539 - Flags: review?(mshal) → review+
(Reporter)

Comment 33

4 years ago
Created attachment 757943 [details] [diff] [review]
move CSRCS to mozbuild (config batch #2)
(Reporter)

Updated

4 years ago
Attachment #757539 - Attachment is obsolete: true
(Reporter)

Comment 34

4 years ago
Comment on attachment 757943 [details] [diff] [review]
move CSRCS to mozbuild (config batch #2)

Same patch but removed memory/jemalloc/src/{Makefile.in,moz.build} per bug # 878866.
Earlier try job covered all but jemalloc/src::Darwin edit - http://tbpl.mozilla.org/?tree=Try&rev=364948d0fee3
r=mshal carried forward.
(Reporter)

Comment 35

4 years ago
Comment on attachment 757943 [details] [diff] [review]
move CSRCS to mozbuild (config batch #2)

Inbound push
committed changeset 133925:e5b6545901b0
http://hg.mozilla.org/integration/mozilla-inbound/rev/e5b6545901b0
https://hg.mozilla.org/mozilla-central/rev/e5b6545901b0
Comment on attachment 754485 [details] [diff] [review]
Migrate CSRCS in comm-central

Looks fine to me, I can't think of any issues at the moment, as it all gets translated back into makefile anyway.
Attachment #754485 - Flags: review?(mbanner) → review+
Comment on attachment 754485 [details] [diff] [review]
Migrate CSRCS in comm-central

https://hg.mozilla.org/comm-central/rev/0c54a5703d29
Attachment #754485 - Flags: checkin+
(Reporter)

Comment 39

4 years ago
Created attachment 760587 [details] [diff] [review]
move CSRCS to moz.build (file batch #3).
(Reporter)

Updated

4 years ago
See Also: → bug 875549
(Reporter)

Comment 40

4 years ago
Created attachment 770434 [details] [diff] [review]
move CSRCS to mozbuild (file batch #3)-stw
(Reporter)

Comment 41

4 years ago
Created attachment 770821 [details] [diff] [review]
move CSRCS to mozbuild (file batch #3).
(Reporter)

Updated

4 years ago
Attachment #770434 - Attachment is obsolete: true
Comment on attachment 770821 [details] [diff] [review]
move CSRCS to mozbuild (file batch #3).

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

Please fix the emptied if blocks in the makefiles?

::: tools/trace-malloc/moz.build
@@ +17,5 @@
> +simplecsrcs = [
> +    'leakstats.c',
> +    'tmstats.c',
> +]
> +        

Trailing ws

::: widget/gtk2/moz.build
@@ +61,5 @@
> +    CSRCS += [
> +        'maiRedundantObjectFactory.c',
> +    ]
> +
> +if CONFIG['NATIVE_THEME_SUPPORT']:

This is always defined in the makefile, and nowhere else.* Just remove the conditional.

[*] And it has been this way since bug 142334, http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/gfx/src/gtk/Attic&command=DIFF_FRAMESET&file=Makefile.in&rev2=1.86&rev1=1.85

@@ +66,5 @@
> +    if CONFIG['MOZ_ENABLE_GTK2']:
> +        CSRCS += [
> +            'gtk2drawing.c',
> +        ]
> +    elif CONFIG['MOZ_ENABLE_GTK2']:

This should be an else
(Reporter)

Comment 43

4 years ago
(In reply to :Ms2ger from comment #42)
> Comment on attachment 770821 [details] [diff] [review]
> move CSRCS to mozbuild (file batch #3).
> 
> Review of attachment 770821 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gtk2/moz.build
> @@ +61,5 @@
> > +    CSRCS += [
> > +        'maiRedundantObjectFactory.c',
> > +    ]
> > +
> > +if CONFIG['NATIVE_THEME_SUPPORT']:
> 
> This is always defined in the makefile, and nowhere else.* Just remove the
> conditional.
> 
> [*] And it has been this way since bug 142334,
> http://bonsai.mozilla.org/cvsview2.
> cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/gfx/
> src/gtk/Attic&command=DIFF_FRAMESET&file=Makefile.in&rev2=1.86&rev1=1.85

These conversions are mostly automated and will be moving content over verbatim.
They are not attempting to interpret logic or optimize away logic.  That would be an exercise for after conversions are complete.

Also the patch was uploaded for safekeeping, it has not been assigned for review yet.
I'm aware; just pointing out a bug to avoid you having to look for it later.
(Reporter)

Updated

4 years ago
Attachment #760587 - Attachment is obsolete: true
(Reporter)

Comment 45

4 years ago
Created attachment 774703 [details] [diff] [review]
move CSRCS to mozbuild (file batch #3).
(Reporter)

Comment 46

4 years ago
Created attachment 774704 [details] [diff] [review]
move CSRCS to mozbuild (file batch #3)

Rebase, fixed conflict and stray whitespace insertion in security/manager/ssl/src/moz.build.

Try results with a lot of orange:
https://tbpl.mozilla.org/?tree=Try&rev=114204240591
Attachment #770821 - Attachment is obsolete: true
Attachment #774703 - Attachment is obsolete: true
Attachment #774704 - Flags: review?(mshal)
Comment on attachment 774704 [details] [diff] [review]
move CSRCS to mozbuild (file batch #3)

># HG changeset patch
># User Joey Armstrong <joey@mozilla.com>
># Date 1372864769 14400
># Node ID 4fdd5bcd73000612a33a89339ada45d0ac3f65b9
># Parent 4dda210929fb56003dba4de87b19c6c5d5cb7d5a
>bug 870406: move CSRCS to mozbuild (file batch #3).
>
>diff --git a/mozglue/build/Makefile.in b/mozglue/build/Makefile.in
>--- a/mozglue/build/Makefile.in
>+++ b/mozglue/build/Makefile.in
>@@ -91,17 +91,16 @@ ifeq (android, $(MOZ_WIDGET_TOOLKIT))
> # Add Android specific code
> EXTRA_DSO_LDOPTS += $(MOZ_ZLIB_LIBS)
> SHARED_LIBRARY_LIBS += $(call EXPAND_LIBNAME_PATH,android,../android)
> # To properly wrap jemalloc's pthread_atfork call.
> EXTRA_DSO_LDOPTS += -Wl,--wrap=pthread_atfork
> endif
> 
> ifeq (gonk, $(MOZ_WIDGET_TOOLKIT))
>-CSRCS += cpuacct.c
> endif

The whole ifeq..endif block can go away.

>diff --git a/tools/trace-malloc/moz.build b/tools/trace-malloc/moz.build
>--- a/tools/trace-malloc/moz.build
>+++ b/tools/trace-malloc/moz.build
>@@ -1,13 +1,33 @@
> # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> # vim: set filetype=python:
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
>+extracsrcs = [
>+    'tmreader.c',
>+]
>+
>+progcsrcs = [
>+    'formdata.c',
>+    'spacecategory.c',
>+    'spacetrace.c',
>+]
>+
>+simplecsrcs = [
>+    'leakstats.c',
>+    'tmstats.c',
>+]
>+        

nit: the last blank line here has a bunch of space characters in it

> if not CONFIG['MOZ_PROFILE_GENERATE']:
>     PROGRAM = 'spacetrace'
>+    CSRCS += progcsrcs
> 
> CPP_SOURCES += [
>     '$(EXTRACPPSRCS)',
>     '$(SIMPLECPPSRCS)',
> ]
>+
>+CSRCS += extracsrcs
>+CSRCS += simplecsrcs
>+CSRCS.sort()

Why do we need to sort() here? I don't think it's necessary, so it should probably just go away.

>diff --git a/widget/gtk2/Makefile.in b/widget/gtk2/Makefile.in
>--- a/widget/gtk2/Makefile.in
>+++ b/widget/gtk2/Makefile.in
>@@ -18,32 +18,26 @@ endif
> 
> EXPORT_LIBRARY	= 1
> LIBXUL_LIBRARY = 1
> 
> NATIVE_THEME_SUPPORT = 1
> 
> 
> 
>-CSRCS		= \
>-		mozcontainer.c \
>-		$(NULL)
> 
> ifdef ACCESSIBILITY
>-CSRCS += maiRedundantObjectFactory.c
> endif

Please remove the whole ifdef block.

> # build our subdirs, too
> 
> SHARED_LIBRARY_LIBS = ../xpwidgets/libxpwidgets_s.a
> 
> ifdef NATIVE_THEME_SUPPORT
> ifdef MOZ_ENABLE_GTK2
>-CSRCS		+= gtk2drawing.c
> else
>-CSRCS		+= gtk3drawing.c
> endif

The whole ifdef MOZ_ENABLE_GTK2 block can go away too.

>diff --git a/widget/gtk2/moz.build b/widget/gtk2/moz.build
>--- a/widget/gtk2/moz.build
>+++ b/widget/gtk2/moz.build
>@@ -48,10 +48,26 @@ if CONFIG['NS_PRINTING']:
>     ]
> 
> if CONFIG['MOZ_X11']:
>     CPP_SOURCES += [
>         'nsClipboard.cpp',
>         'nsDragService.cpp',
>     ]
> 
>-CPP_SOURCES += [
>+CSRCS += [
>+    'mozcontainer.c',
> ]
>+
>+if CONFIG['ACCESSIBILITY']:
>+    CSRCS += [
>+        'maiRedundantObjectFactory.c',
>+    ]
>+
>+if CONFIG['NATIVE_THEME_SUPPORT']:

NATIVE_THEME_SUPPORT is not a CONFIG variable - it is a local variable in the Makefile.in that is hard-coded to 1. Unless there's a good reason to keep that around, I recommend just deleting NATIVE_THEME_SUPPORT.

>diff --git a/widget/gtkxtbin/Makefile.in b/widget/gtkxtbin/Makefile.in
>--- a/widget/gtkxtbin/Makefile.in
>+++ b/widget/gtkxtbin/Makefile.in
>@@ -10,19 +10,16 @@ srcdir		= @srcdir@
> VPATH		= @srcdir@
> 
> include $(DEPTH)/config/autoconf.mk
> 
> EXPORT_LIBRARY = 1
> LIBXUL_LIBRARY = 1
> 
> ifdef MOZ_ENABLE_GTK2
>-CSRCS		= \
>-		gtk2xtbin.c \
>-		$(NULL)
> endif

This ifdef block can go away.

This looks almost ready - the r- is primarily for the NATIVE_THEME_SUPPORT issue.
Attachment #774704 - Flags: review?(mshal) → review-
(Reporter)

Comment 48

4 years ago
(In reply to Michael Shal [:mshal] from comment #47)
> Comment on attachment 774704 [details] [diff] [review]
> move CSRCS to mozbuild (file batch #3)
> 
> >+CSRCS += extracsrcs
> >+CSRCS += simplecsrcs
> >+CSRCS.sort()
> 
> Why do we need to sort() here? I don't think it's necessary, so it should
> probably just go away.

CSRCS has the StrictOrdering attribute, mach configure will fail in this directory w/o it.
(Reporter)

Comment 49

4 years ago
(In reply to Michael Shal [:mshal] from comment #47)
> Comment on attachment 774704 [details] [diff] [review]
> move CSRCS to mozbuild (file batch #3)
> 
> >diff --git a/mozglue/build/Makefile.in b/mozglue/build/Makefile.in
> >--- a/mozglue/build/Makefile.in
> >+++ b/mozglue/build/Makefile.in
> > 
> > ifeq (gonk, $(MOZ_WIDGET_TOOLKIT))
> >-CSRCS += cpuacct.c
> > endif
> 
> The whole ifeq..endif block can go away.

The conversion script will need an edit to purge these automatically.  They are remnants of removing DISABLED_* tokens.
(In reply to Joey Armstrong [:joey] from comment #48)
> (In reply to Michael Shal [:mshal] from comment #47)
> > Comment on attachment 774704 [details] [diff] [review]
> > move CSRCS to mozbuild (file batch #3)
> > 
> > >+CSRCS += extracsrcs
> > >+CSRCS += simplecsrcs
> > >+CSRCS.sort()
> > 
> > Why do we need to sort() here? I don't think it's necessary, so it should
> > probably just go away.
> 
> CSRCS has the StrictOrdering attribute, mach configure will fail in this
> directory w/o it.

That only applies to an individual += statement - so for example extrasrcs has to be sorted, but the whole CSRCS list does not. I just tried removing the CSRCS.sort() line and config.status ran correctly. If for some reason we needed CSRCS to be sorted, we could sort it in the mozbuild python code, rather than in each moz.build file.
(Reporter)

Comment 51

4 years ago
(In reply to Michael Shal [:mshal] from comment #50)
> (In reply to Joey Armstrong [:joey] from comment #48)
> > (In reply to Michael Shal [:mshal] from comment #47)
> > > Comment on attachment 774704 [details] [diff] [review]
> > > move CSRCS to mozbuild (file batch #3)
> > > 
> > > >+CSRCS += extracsrcs
> > > >+CSRCS += simplecsrcs
> > > >+CSRCS.sort()
> > > 
> > > Why do we need to sort() here? I don't think it's necessary, so it should
> > > probably just go away.
> > 
> > CSRCS has the StrictOrdering attribute, mach configure will fail in this
> > directory w/o it.
> 
> That only applies to an individual += statement - so for example extrasrcs
> has to be sorted, but the whole CSRCS list does not. I just tried removing
> the CSRCS.sort() line and config.status ran correctly.

If the file addition was wrapped by a platform/toolbox/etc conditional you may not see it.
Some will only show up if you temporarily hack 'True' into the conditionals so everything will be evaluated.

> If for some reason we needed CSRCS to be sorted, we could sort it in the mozbuild python code, rather than in each moz.build file.

Won't an internal sort negate the reason the sort attribute was added ?  That being maintain mozbuild content in sorted order.  Maybe the answer is to add a disable-sort override, by default sorting is required but can be selectively disabled for special cases.
(In reply to Joey Armstrong [:joey] from comment #51)
> (In reply to Michael Shal [:mshal] from comment #50)
> > (In reply to Joey Armstrong [:joey] from comment #48)
> > > (In reply to Michael Shal [:mshal] from comment #47)
> > > > Comment on attachment 774704 [details] [diff] [review]
> > > > move CSRCS to mozbuild (file batch #3)
> > > > 
> > > > >+CSRCS += extracsrcs
> > > > >+CSRCS += simplecsrcs
> > > > >+CSRCS.sort()
> > > > 
> > > > Why do we need to sort() here? I don't think it's necessary, so it should
> > > > probably just go away.
> > > 
> > > CSRCS has the StrictOrdering attribute, mach configure will fail in this
> > > directory w/o it.
> > 
> > That only applies to an individual += statement - so for example extrasrcs
> > has to be sorted, but the whole CSRCS list does not. I just tried removing
> > the CSRCS.sort() line and config.status ran correctly.
> 
> If the file addition was wrapped by a platform/toolbox/etc conditional you
> may not see it.
> Some will only show up if you temporarily hack 'True' into the conditionals
> so everything will be evaluated.
> 

Try it out for yourself. This is fine:

CSRCS += ['z.c']
CSRCS += ['a.c']

This is not:
CSRCS += ['z.c', 'a.c']

> > If for some reason we needed CSRCS to be sorted, we could sort it in the mozbuild python code, rather than in each moz.build file.
> 
> Won't an internal sort negate the reason the sort attribute was added ? 
> That being maintain mozbuild content in sorted order.  Maybe the answer is
> to add a disable-sort override, by default sorting is required but can be
> selectively disabled for special cases.

The point is that so in a giant list of files, there is an ordering to it. Making sure that CSRCS is sorted at the end of the file doesn't add any value.
(Reporter)

Comment 53

4 years ago
(In reply to Michael Shal [:mshal] from comment #52)
>
> Try it out for yourself. This is fine:
> 
> CSRCS += ['z.c']
> CSRCS += ['a.c']
> 
> This is not:
> CSRCS += ['z.c', 'a.c']

Yes I know how the syntax/sorting works.  I'll see if I can reproduce the failure that forced adding .sort().
(Reporter)

Comment 54

4 years ago
(In reply to Michael Shal [:mshal] from comment #52)
> Try it out for yourself. This is fine:
> 
> CSRCS += ['z.c']
> CSRCS += ['a.c']
> 
> This is not:
> CSRCS += ['z.c', 'a.c']

Ok I see where this is coming from now.  When local variables are used to accumulate values -- which is in play here because this conversion is dependent on a set of yet-to-be-converted variables.

If values are accumulated within a local variable.
That list is unsorted due to platform/toolkit conditionals { which can happen, mozbuild just does not complain about isolated assignments within the full list }.
Attempt to append local vars to the CSRCS var and it will croak.

If variables 'CSRCS' and 'csrcs' are used:

CSRCS += ['aa.c']
if CONFIG ['MOZ_WIDGET_TOOLKIT'] != 'android':
  CSRCS += ['mm.c']
CSRCS += ['rr.c'
if CONFIG['OS_ARCH'] != 'Darwin':
  CSRCS += ['jj.c']

The above succeeds for CSRCS.

Replace CSRCS with csrcs with the following assignment and mach will complain:
# CSRCS += csrcs

The error as reported by Python is:
  ['UnsortedError: An attempt was made to add an unsorted sequence to a list.  The incoming list is unsorted starting at element 1.  We expected "jj.mm" but got "mm.mm"\n']

So if anyone needs to perform local collection or filtering on the file list prior to assignment as I did with the unconverted variables the assignment will trigger a sort failure.
(Reporter)

Comment 55

4 years ago
Created attachment 776365 [details] [diff] [review]
move CSRCS to mozbuild (file batch #3).
(Reporter)

Updated

4 years ago
Attachment #774704 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
See Also: → bug 894371
(Reporter)

Comment 56

4 years ago
Comment on attachment 776365 [details] [diff] [review]
move CSRCS to mozbuild (file batch #3).

nits fixed.
removed NATIVE_THEME_SUPPORT conditional.
removed local vars containing file lists.  We lose the breakdown of file by type but the sort problem goes away.

cleaning up conditionals around variables being deleted morphs automated conversions into a manual task.  Cleaned these up but opened a bug to add a script that would remove them automatically.  The script searching for empty makefiles should also support this logic.
Attachment #776365 - Flags: review?(mshal)
Comment on attachment 776365 [details] [diff] [review]
move CSRCS to mozbuild (file batch #3).

This looks good to me! We may need to cleanup tools/trace-malloc/moz.build once the rest of the variables are moved over (especially for my lazy CPP_SOURCES change there - blech). But for now this works.
Attachment #776365 - Flags: review?(mshal) → review+
(Reporter)

Comment 58

4 years ago
Created attachment 776581 [details] [diff] [review]
move CSRCS to mozbuild (file batch #3).
(Reporter)

Updated

4 years ago
Attachment #776365 - Attachment is obsolete: true
(Reporter)

Comment 59

4 years ago
Comment on attachment 776581 [details] [diff] [review]
move CSRCS to mozbuild (file batch #3).

rebase, r=mshal carried forward
(Reporter)

Comment 60

4 years ago
Inbound push:
committed changeset 138734:8bbd27688a89
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bbd27688a89
(Reporter)

Comment 61

4 years ago
Created attachment 776652 [details] [diff] [review]
cleanup - purge DISABLED_CSRCS variables.
(Reporter)

Updated

4 years ago
Attachment #776652 - Attachment is obsolete: true
(Reporter)

Comment 62

4 years ago
Created attachment 776655 [details] [diff] [review]
cleanup - purge DISABLED_CSRCS variables.
Backed out for linux bustage.
http://hg.mozilla.org/integration/mozilla-inbound/rev/e97286839337

https://tbpl.mozilla.org/php/getParsedLog.php?id=25342807&tree=Mozilla-Inbound
Depends on: 894606
(Reporter)

Comment 64

4 years ago
Created attachment 777315 [details] [diff] [review]
move CSRCS to mozbuild (file batch #3).
(Reporter)

Updated

4 years ago
Attachment #776581 - Attachment is obsolete: true
Blocks: 904572
(Reporter)

Comment 65

4 years ago
Created attachment 799767 [details] [diff] [review]
move CSRCS to mozbuild - patch #4.
(Reporter)

Comment 66

4 years ago
Comment on attachment 799767 [details] [diff] [review]
move CSRCS to mozbuild - patch #4.

try build results: http://tbpl.mozilla.org/?tree=Try&rev=d7019662c912
(Reporter)

Comment 67

4 years ago
Comment on attachment 799767 [details] [diff] [review]
move CSRCS to mozbuild - patch #4.

Refresh patch, modules/zlib/src/Makefile.in can be deleted.
Build try results for the patch:
http://tbpl.mozilla.org/?tree=Try&rev=d7019662c912
A test job is on the way.
Attachment #799767 - Flags: review?(mshal)
(Reporter)

Comment 68

4 years ago
(In reply to Joey Armstrong [:joey] from comment #67)
> Comment on attachment 799767 [details] [diff] [review]
> move CSRCS to mozbuild - patch #4.
> 
> Refresh patch, modules/zlib/src/Makefile.in can be deleted.
> Build try results for the patch:
> http://tbpl.mozilla.org/?tree=Try&rev=d7019662c912
> A test job is on the way.


try-test: http://tbpl.mozilla.org/?tree=Try&rev=d14e75ee5b29
Comment on attachment 799767 [details] [diff] [review]
move CSRCS to mozbuild - patch #4.

>diff --git a/build/unix/elfhack/inject/moz.build b/build/unix/elfhack/inject/moz.build
>--- a/build/unix/elfhack/inject/moz.build
>+++ b/build/unix/elfhack/inject/moz.build
>@@ -1,7 +1,20 @@
> # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> # vim: set filetype=python:
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> NO_DIST_INSTALL = True
>+
>+
>+if CONFIG['TARGET_CPU'][-2:] == '86':
>+    cpu = 'x86'
>+elif CONFIG['TARGET_CPU'][:3] == 'arm':
>+    cpu = 'arm'

I think it would be more clear if we did:

if CONFIG['TARGET_CPU'].endswith('86'):
    cpu = 'x86'
elif CONFIG['TARGET_CPU'].startswith('arm'):
    cpu = 'arm'

Though I am not a python expert :)

>diff --git a/modules/zlib/src/moz.build b/modules/zlib/src/moz.build
>--- a/modules/zlib/src/moz.build
>+++ b/modules/zlib/src/moz.build
>@@ -9,8 +9,25 @@ MODULE = 'zlib'
> EXPORTS += [
>     'mozzconf.h',
>     'zconf.h',
>     'zlib.h',
> ]
> 
> LIBRARY_NAME = 'mozz'
> 
>+CSRCS += [
>+    'adler32.c',
>+    'compress.c',
>+    'crc32.c',
>+    'deflate.c',
>+    'gzclose.c',
>+    'gzlib.c',
>+    'gzread.c',
>+    'gzwrite.c',
>+    'infback.c',
>+    'inffast.c',
>+    'inflate.c',
>+    'inftrees.c',
>+    'trees.c',
>+    'uncompr.c',
>+    'zutil.c',
>+]

Can you also remove objs.mk now that all this info is in moz.build? It doesn't look like anything else in the tree includes it, and I'd rather not have a defunct list of source files in the tree. I can r+ once it is removed, or if you confirm it is still needed.
Attachment #799767 - Flags: review?(mshal) → feedback+
(Reporter)

Comment 70

4 years ago
(In reply to Michael Shal [:mshal] from comment #69)
> Comment on attachment 799767 [details] [diff] [review]
> move CSRCS to mozbuild - patch #4.
> 
> >diff --git a/build/unix/elfhack/inject/moz.build b/build/unix/elfhack/inject/moz.build
> >--- a/build/unix/elfhack/inject/moz.build
> >+++ b/build/unix/elfhack/inject/moz.build
> >@@ -1,7 +1,20 @@
> > # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> > # vim: set filetype=python:
> > # This Source Code Form is subject to the terms of the Mozilla Public
> > # License, v. 2.0. If a copy of the MPL was not distributed with this
> > # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > 
> > NO_DIST_INSTALL = True
> >+
> >+
> >+if CONFIG['TARGET_CPU'][-2:] == '86':
> >+    cpu = 'x86'
> >+elif CONFIG['TARGET_CPU'][:3] == 'arm':
> >+    cpu = 'arm'
> 
> I think it would be more clear if we did:
> 
> if CONFIG['TARGET_CPU'].endswith('86'):
>     cpu = 'x86'
> elif CONFIG['TARGET_CPU'].startswith('arm'):
>     cpu = 'arm'
> 
> Though I am not a python expert :)

I did have these tests in earlier but python was complaining that utf values were not a string.
(Reporter)

Comment 71

4 years ago
patch #3 will need to be recreated, some vars were converted with bug 906619.
See Also: → bug 906619
(Reporter)

Comment 72

4 years ago
Created attachment 802590 [details] [diff] [review]
move CSRCS to mozbuild (batch #3)

refresh patch #3
Attachment #777315 - Attachment is obsolete: true
(Reporter)

Comment 73

4 years ago
Created attachment 805985 [details] [diff] [review]
move CSRCS to mozbuild - patch #4.
(Reporter)

Updated

4 years ago
Attachment #799767 - Attachment is obsolete: true
(Reporter)

Comment 74

4 years ago
Comment on attachment 805985 [details] [diff] [review]
move CSRCS to mozbuild - patch #4.

obj.mk removed added string funcs since no complaints about utf encoding this time.  try run: http://tbpl.mozilla.org/?tree=Try&rev=d14e75ee5b29
Attachment #805985 - Flags: review?(mshal)
Comment on attachment 805985 [details] [diff] [review]
move CSRCS to mozbuild - patch #4.

Looks good to me!
Attachment #805985 - Flags: review?(mshal) → review+
(Reporter)

Comment 76

4 years ago
Comment on attachment 802590 [details] [diff] [review]
move CSRCS to mozbuild (batch #3)

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

Re-try widget/gtk2 w/o that pesky NATIVE_THEME_SUPPORT dependency from Makefile.in

http://tbpl.mozilla.org/?tree=Try&rev=f9be0e50f943
(Reporter)

Comment 77

4 years ago
Created attachment 807931 [details] [diff] [review]
move CSRCS to mozbuild (file batch #3).
(Reporter)

Updated

4 years ago
Attachment #802590 - Attachment is obsolete: true
(Reporter)

Comment 78

4 years ago
Comment on attachment 807931 [details] [diff] [review]
move CSRCS to mozbuild (file batch #3).

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

Finally, widget/gtk2 has a clean run on linux*.
move csrcs to mozbuild batch 3
Attachment #807931 - Flags: review?(mshal)
(Reporter)

Comment 79

4 years ago
Comment on attachment 805985 [details] [diff] [review]
move CSRCS to mozbuild - patch #4.

Inbound push: https://hg.mozilla.org/integration/mozilla-inbound/rev/cee18252461e
Comment on attachment 807931 [details] [diff] [review]
move CSRCS to mozbuild (file batch #3).

>diff --git a/tools/trace-malloc/moz.build b/tools/trace-malloc/moz.build
>--- a/tools/trace-malloc/moz.build
>+++ b/tools/trace-malloc/moz.build
>@@ -1,28 +1,36 @@
> # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> # vim: set filetype=python:
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> if not CONFIG['MOZ_PROFILE_GENERATE']:
>     PROGRAM = 'spacetrace'
>+    CSRCS += [
>+        'formdata.c',
>+        'spacecategory.c',
>+        'spacetrace.c',
>+    ]
> 
> bin_suffix = CONFIG['BIN_SUFFIX']
> 
> 
> simple_c_sources = [
>     'leakstats',
>     'tmstats',
> ]
> 
> CSRCS += [
>     '%s.c' % s for s in simple_c_sources
> ]
>+CSRCS += [
>+    'tmreader.c', # EXTRACSRCS

Can we get rid of the # EXTRACSRCS comment? I don't think it tells us anything useful.
Attachment #807931 - Flags: review?(mshal) → review+
(Reporter)

Comment 81

4 years ago
Created attachment 807972 [details] [diff] [review]
move CSRCS to mozbuild (file batch #3).
(Reporter)

Updated

4 years ago
Attachment #807931 - Attachment is obsolete: true
(Reporter)

Comment 82

4 years ago
Comment on attachment 807972 [details] [diff] [review]
move CSRCS to mozbuild (file batch #3).

prune the comment about source variable from Makefile.in.
r=mshal carried forward
(Reporter)

Comment 83

4 years ago
Comment on attachment 807972 [details] [diff] [review]
move CSRCS to mozbuild (file batch #3).

Inbound push for patch 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/bfc40b8abf4c
https://hg.mozilla.org/mozilla-central/rev/cee18252461e
https://hg.mozilla.org/mozilla-central/rev/bfc40b8abf4c
(Reporter)

Comment 85

4 years ago
Created attachment 809223 [details] [diff] [review]
move CSRCS to mozbuild (batch #5)
(Reporter)

Comment 86

4 years ago
Created attachment 809239 [details] [diff] [review]
move CSRCS to mozbuild (stlport)
(Reporter)

Comment 87

4 years ago
Comment on attachment 809239 [details] [diff] [review]
move CSRCS to mozbuild (stlport)

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

1) move CSRCS to mozbuild, try job-http://tbpl.mozilla.org/?tree=Try&rev=44ea5b5e31c6
2) Expand CPP_SOURCES makefile macro while we are in here.
Attachment #809239 - Flags: review?(mshal)
Comment on attachment 809239 [details] [diff] [review]
move CSRCS to mozbuild (stlport)

Looks good - thanks for cleaning up CPP_SOURCES as well! Any idea what is up with the try results? Windows is having a bad day, though it doesn't look like it's related to your changes.
Attachment #809239 - Flags: review?(mshal) → review+
(Assignee)

Updated

4 years ago
Blocks: 932197
(Assignee)

Comment 89

4 years ago
Created attachment 823874 [details] [diff] [review]
part n - Move more CSRCS to moz.build

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

This unbitrots the remaining bits from here, refactor some stuff for cairo, and add a few more moves. With this applied, the only remaining CSRCS in the tree require more stuff and would each need their own bug imho, so I'm all for closing this bug once this patch lands.
Attachment #823874 - Flags: review?(mshal)
(Assignee)

Updated

4 years ago
Assignee: joey → mh+mozilla
Status: NEW → ASSIGNED
(Assignee)

Comment 90

4 years ago
Created attachment 823884 [details] [diff] [review]
part n - Move more CSRCS to moz.build

https://tbpl.mozilla.org/?tree=Try&rev=443008c77360

(Previous try had a problem on another patch, and a problem in the ordering of one of the SOURCES in this patch)
Attachment #823884 - Flags: review?(mshal)
(Assignee)

Updated

4 years ago
Attachment #823874 - Attachment is obsolete: true
Attachment #823874 - Flags: review?(mshal)
Comment on attachment 823884 [details] [diff] [review]
part n - Move more CSRCS to moz.build

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

::: gfx/cairo/cairo/src/moz.build
@@ +16,5 @@
>      'cairo-version.h',
>      'cairo.h',
>  ]
>  
> +if CONFIG['MOZ_WIDGET_TOOLKIT'] not in ('cocoa', 'uikit'):

Where does uikit come from?

::: gfx/ots/src/Makefile.in
@@ +25,5 @@
>  #
>  
>  
>  ifeq (WINNT,$(OS_TARGET))
>  endif

Feel free to remove this too

::: xpcom/base/moz.build
@@ +67,5 @@
>      EXPORTS += [
>          'nsWindowsHelpers.h',
>      ]
> +    if CONFIG['MOZ_DEBUG']:
> +        EXPORTS += ['pure.h']

Makes sense, I guess
(Assignee)

Comment 92

4 years ago
(In reply to :Ms2ger from comment #91)
> > +if CONFIG['MOZ_WIDGET_TOOLKIT'] not in ('cocoa', 'uikit'):
> 
> Where does uikit come from?

From configure.in. It probably doesn't work anymore, but meh.
(Assignee)

Comment 93

4 years ago
Created attachment 823938 [details] [diff] [review]
part n - Move more CSRCS to moz.build

With various fixups. Crossing fingers.

https://tbpl.mozilla.org/?tree=Try&rev=520fd6a9e27b
Attachment #823938 - Flags: review?(mshal)
(Assignee)

Updated

4 years ago
Attachment #823884 - Attachment is obsolete: true
Attachment #823884 - Flags: review?(mshal)
(In reply to Mike Hommey [:glandium] from comment #92)
> > Where does uikit come from?
> 
> From configure.in. It probably doesn't work anymore, but meh.

It's never fully worked, but I haven't ruled out getting back to that port.
Comment on attachment 823938 [details] [diff] [review]
part n - Move more CSRCS to moz.build

>diff --git a/build/stlport/moz.build b/build/stlport/moz.build
>--- a/build/stlport/moz.build
>+++ b/build/stlport/moz.build
>@@ -2,8 +2,46 @@
> # vim: set filetype=python:
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> LIBRARY_NAME = 'stlport_static'
> 
> FORCE_STATIC_LIB = True
>+
>+SOURCES += [
>+    'src/allocators.cpp',
>+    'src/bitset.cpp',
>+    'src/codecvt.cpp',
>+    'src/collate.cpp',
>+    'src/complex.cpp',
>+    'src/complex_io.cpp',
>+    'src/complex_trig.cpp',
>+    'src/ctype.cpp',
>+    'src/dll_main.cpp',
>+    'src/facets_byname.cpp',
>+    'src/fstream.cpp',
>+    'src/ios.cpp',
>+    'src/iostream.cpp',
>+    'src/istream.cpp',
>+    'src/locale.cpp',
>+    'src/locale_catalog.cpp',
>+    'src/locale_impl.cpp',
>+    'src/messages.cpp',
>+    'src/monetary.cpp',
>+    'src/num_get.cpp',
>+    'src/num_get_float.cpp',
>+    'src/num_put.cpp',
>+    'src/num_put_float.cpp',
>+    'src/numpunct.cpp',
>+    'src/ostream.cpp',
>+    'src/sstream.cpp',
>+    'src/stdio_streambuf.cpp',
>+    'src/string.cpp',
>+    'src/strstream.cpp',
>+    'src/time_facets.cpp',
>+]
>+
>+SOURCES += [
>+    'src/c_locale.c',
>+    'src/cxa.c',
>+]

Any reason not to combine these SOURCES blocks?

Everyone else looks good to me.
Attachment #823938 - Flags: review?(mshal) → review+
(Assignee)

Comment 96

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/564210bf70b3
(Assignee)

Updated

4 years ago
Whiteboard: [leave open]

Updated

4 years ago
Depends on: 933672
https://hg.mozilla.org/mozilla-central/rev/564210bf70b3
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Comment 98

3 years ago
"part n - Move more CSRCS to moz.build" breaks the build for me:

/usr/lib/gcc/x86_64-pc-linux-gnu/4.8.3/../../../../x86_64-pc-linux-gnu/bin/ld: error: /var/tmp/moz-build-dir/toolkit/library/../../ipc/chromium/event.o: requires dynamic R_X8
6_64_PC32 reloc against 'eventfd' which may overflow at runtime; recompile with -fPIC
/usr/lib/gcc/x86_64-pc-linux-gnu/4.8.3/../../../../x86_64-pc-linux-gnu/bin/ld: error: read-only segment has dynamic relocations
/var/tmp/moz-build-dir/toolkit/library/../../ipc/chromium/event.o:event.c:function evthread_make_base_notifiable.part.27: error: undefined reference to 'eventfd'
collect2: error: ld returned 1 exit status
make[5]: *** [libxul.so] Error 1
(Assignee)

Comment 99

3 years ago
(In reply to Octoploid from comment #98)
> "part n - Move more CSRCS to moz.build" breaks the build for me:
> 
> /usr/lib/gcc/x86_64-pc-linux-gnu/4.8.3/../../../../x86_64-pc-linux-gnu/bin/
> ld: error:
> /var/tmp/moz-build-dir/toolkit/library/../../ipc/chromium/event.o: requires
> dynamic R_X8
> 6_64_PC32 reloc against 'eventfd' which may overflow at runtime; recompile
> with -fPIC
> /usr/lib/gcc/x86_64-pc-linux-gnu/4.8.3/../../../../x86_64-pc-linux-gnu/bin/
> ld: error: read-only segment has dynamic relocations
> /var/tmp/moz-build-dir/toolkit/library/../../ipc/chromium/event.o:event.c:
> function evthread_make_base_notifiable.part.27: error: undefined reference
> to 'eventfd'
> collect2: error: ld returned 1 exit status
> make[5]: *** [libxul.so] Error 1

bug 933672?

Comment 100

3 years ago
(In reply to Mike Hommey [:glandium] from comment #99)
> bug 933672?

Oops, yes.
(Assignee)

Updated

3 years ago
Blocks: 928196

Updated

3 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.