Allow to build with --enable-project=memory

RESOLVED FIXED in Firefox 59

Status

enhancement
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
mozilla59
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(5 attachments)

Assignee

Description

2 years ago
No description provided.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8928402 [details]
Bug 1417309 - Remove the nscore.h include from basictypes.h.

https://reviewboard.mozilla.org/r/199622/#review204874

r=me assuming this is green.
Attachment #8928402 - Flags: review?(nfroyd) → review+

Comment 7

2 years ago
mozreview-review
Comment on attachment 8928399 [details]
Bug 1417309 - Only build elfhack when building a toolkit-based application.

https://reviewboard.mozilla.org/r/199616/#review204946

::: old-configure.in:3880
(Diff revision 1)
>  
>  dnl ========================================================
>  dnl = --disable-elf-hack
>  dnl ========================================================
>  
> +if test -n "$MOZ_WIDGET_TOOLKIT"; then

Can this whole stanza move to `moz.configure`?  If not, why not?
Attachment #8928399 - Flags: review+

Comment 8

2 years ago
mozreview-review
Comment on attachment 8928400 [details]
Bug 1417309 - Fix mozbuild.context.Path.join when the path was created in an included moz.build file.

https://reviewboard.mozilla.org/r/199618/#review204950

r- to discuss this, probably with gps.

First, this definitely needs a test.

Second, this approach doesn't make sense to me.  If we can replace the context for arbitrary `Path` instances with a meaningless `DummyContext`, then `Path` itself can't really require anything more from its encapsulating `Context` than `srcdir`.  And that appears to be the case.  If that's true, shouldn't we be making `Path` care about only the `srcdir` and not faking out the rich `Context` object, leading to frustrating bugs later on when `Path` instances care about more from the `Context`?

Or am I misunderstanding the metaprogramming here and this only overrides `srcdir`?  Either way, this doesn't smell right to me.

::: commit-message-6c0c3:5
(Diff revision 1)
> +Bug 1417309 - Fix mozbuild.context.Path.join when the path was created in an included moz.build file. r?build
> +
> +When the directory traversal code, or the backend code operates on Path
> +instances, the context in which they were created may have changed
> +since the instanced have been created. And when they use Path.join,

nit: fix "instanced".
Attachment #8928400 - Flags: review-

Comment 9

2 years ago
mozreview-review
Comment on attachment 8928401 [details]
Bug 1417309 - Move some definitions from top-level moz.build to application-specific moz.build files.

https://reviewboard.mozilla.org/r/199620/#review204952

Mechanically, this looks fine.

What I don't understand is how you can `--enable-{application,project}=memory` and not have `MOZ_BUILD_APP` set.  If I'm reading https://searchfox.org/mozilla-central/source/build/moz.configure/init.configure#935-987 correctly, you'll always get `MOZ_BUILD_APP` set.  And that makes the check in the top-level `moz.build` redundant.  Explain to me what happens here.

::: commit-message-506a8:9
(Diff revision 1)
> +toolkit-based one (like browser or mobile/android), we don't want to be
> +building things that are specific to gecko and/or spidermonkey.
> +
> +At the same time, this lifts the exception that js standalone doesn't
> +have an app.mozbuild included, and makes the moz.build parsers that
> +don't set a MOZ_BUILD_APP get all the things through toolkit.mozbuild.

nit: replace "all the things" with more detail.
Attachment #8928401 - Flags: review+

Comment 10

2 years ago
mozreview-review
Comment on attachment 8928403 [details]
Bug 1417309 - Add the necessary bits to support a --enable-project=memory option.

https://reviewboard.mozilla.org/r/199624/#review204956

::: memory/moz.build:15
(Diff revision 1)
>      'build',
> -    'mozalloc',
>      'fallible',
>  ]
>  
> +# For now, don't build mozalloc when building with --enable-project=memory

Why not?
Attachment #8928403 - Flags: review+
Attachment #8928399 - Flags: review?(core-build-config-reviews)
Attachment #8928400 - Flags: review?(core-build-config-reviews)
Attachment #8928401 - Flags: review?(core-build-config-reviews)
Attachment #8928403 - Flags: review?(core-build-config-reviews)
Assignee

Comment 11

2 years ago
(In reply to Nick Alexander :nalexander from comment #10)
> Comment on attachment 8928403 [details]
> Bug 1417309 - Add the necessary bits to support a --enable-project=memory
> option.
> 
> https://reviewboard.mozilla.org/r/199624/#review204956
> 
> ::: memory/moz.build:15
> (Diff revision 1)
> >      'build',
> > -    'mozalloc',
> >      'fallible',
> >  ]
> >  
> > +# For now, don't build mozalloc when building with --enable-project=memory
> 
> Why not?

Because it's a source of build problems at the moment.

(In reply to Nick Alexander :nalexander from comment #9)
> Comment on attachment 8928401 [details]
> Bug 1417309 - Move some definitions from top-level moz.build to
> application-specific moz.build files.
> 
> https://reviewboard.mozilla.org/r/199620/#review204952
> 
> Mechanically, this looks fine.
> 
> What I don't understand is how you can
> `--enable-{application,project}=memory` and not have `MOZ_BUILD_APP` set. 
> If I'm reading
> https://searchfox.org/mozilla-central/source/build/moz.configure/init.
> configure#935-987 correctly, you'll always get `MOZ_BUILD_APP` set.  And
> that makes the check in the top-level `moz.build` redundant.  Explain to me
> what happens here.

This is for the non-tradition use cases where we actually don't recurse moz.builds with a proper configuration.

(In reply to Nick Alexander :nalexander from comment #8)
> Comment on attachment 8928400 [details]
> Bug 1417309 - Fix mozbuild.context.Path.join when the path was created in an
> included moz.build file.
> 
> https://reviewboard.mozilla.org/r/199618/#review204950
> 
> r- to discuss this, probably with gps.
> 
> First, this definitely needs a test.
> 
> Second, this approach doesn't make sense to me.  If we can replace the
> context for arbitrary `Path` instances with a meaningless `DummyContext`,
> then `Path` itself can't really require anything more from its encapsulating
> `Context` than `srcdir`.  And that appears to be the case.  If that's true,
> shouldn't we be making `Path` care about only the `srcdir` and not faking
> out the rich `Context` object, leading to frustrating bugs later on when
> `Path` instances care about more from the `Context`?
> 
> Or am I misunderstanding the metaprogramming here and this only overrides
> `srcdir`?  Either way, this doesn't smell right to me.

This only overrides srcdir by creating a subclass of Context and assigning it to the object __class__ temporarily.
Assignee

Comment 12

2 years ago
See comment 8 and 11
Flags: needinfo?(gps)
Assignee

Comment 13

2 years ago
Actually, forget it, I'll just work around the Path issue by using topsrcdir-relative paths in js/app.mozbuild.
Flags: needinfo?(gps)
Assignee

Comment 14

2 years ago
mozreview-review-reply
Comment on attachment 8928399 [details]
Bug 1417309 - Only build elfhack when building a toolkit-based application.

https://reviewboard.mozilla.org/r/199616/#review204946

> Can this whole stanza move to `moz.configure`?  If not, why not?

I'll leave that to a followup.

Comment 15

2 years ago
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c4fac311fe1
Only build elfhack when building a toolkit-based application. r=nalexander
https://hg.mozilla.org/integration/mozilla-inbound/rev/74a6998f7206
Move some definitions from top-level moz.build to application-specific moz.build files. r=nalexander
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e648d014277
Remove the nscore.h include from basictypes.h. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/c995692a3c92
Add the necessary bits to support a --enable-project=memory option. r=nalexander
(In reply to Mike Hommey [:glandium] from comment #14)
> Comment on attachment 8928399 [details]
> Bug 1417309 - Only build elfhack when building a toolkit-based application.
> 
> https://reviewboard.mozilla.org/r/199616/#review204946
> 
> > Can this whole stanza move to `moz.configure`?  If not, why not?
> 
> I'll leave that to a followup.

glandium kindly did this in Bug 1417309.

Mike, perhaps you could whip up a --enable-project=memory Task Cluster build job so that this stays green?
Flags: needinfo?(mh+mozilla)
Assignee

Comment 18

2 years ago
I've thought about it, but at the moment, it's not entirely useful on its own. For one, it doesn't build the corresponding unit tests. And I'm not entirely sure it actually builds on more than Linux. Anyways, I'm using this actively, so breakage will be noticed quickly if it happens, but yes, eventually, it might be worth adding TC jobs.
Flags: needinfo?(mh+mozilla)
Assignee

Updated

2 years ago
Depends on: 1420100

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.