1041941 - Define types of programs/shared libraries in moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

From https://bugzilla.mozilla.org/show_bug.cgi?id=1036894#c55:

There are, however, one or two things I want to add that would alleviate this:
- allow intermediate static libs to have shared library dependencies, which would be linked to the final lib (and throw errors if the static lib is linked nowhere where those shared library deps are going to be used)
- define types of programs/shared libraries. Something to say "this is something for gecko", "this is independent", "this is a program with xpcom standalone linkage", etc. which would also replace e.g. resetting MOZ_GLUE_LDFLAGS and/or setting DISABLE_STL_WRAPPING
but I haven't formalized anything yet.


This is for the latter of these two points.

Here's a thought:

In e.g. $topsrcdir/moz.build:
  add_template('gecko_program', {
    'USE_LIBS': ['xpcomglue'],
  })
  add_template('gecko_dependent_program', {
    'USE_LIBS': ['mozalloc', 'xpcomglue_s', 'xul'], 
  })
  add_template('component', {
    'USE_LIBS': ['mozalloc', 'xpcomglue_s', 'xul'],
    'IS_COMPONENT': True
  })

(mozglue could even fit in there)

And in some other moz.build:
  TEMPLATE = 'component'

Thoughts?

One thing I don't like with using a function for add_template() is that it makes things cumbersome for conditions (for example, on ELF systems, we'd want mozglue only on program templates, but not on libraries, which we'd want it everywhere on other platforms)

Comment 1

[Mike Hommey [:glandium]]

Here's a thought I had under the shower:

moz.build:
  import_template('gecko_program', 'templates/gecko_program.mozbuild')
  import_template('gecko_component', 'templates/gecko_component.mozbuild')

templates/gecko_program.mozbuild:
  USE_LIBS += ['xpcomglue']

templates/gecko_component.mozbuild
  USE_LIBS += ['mozalloc', 'xpcomglue_s', 'xul']
  IS_COMPONENT = True

Comment 2

[Mike Hommey [:glandium]]

(and obviously, that could be used for more than programs and libraries)

Comment 3

[Gregory Szorc [:gps]]

I like the idea of importing other scripts and presumably executing them in the same sandbox as the importer. This is inherently more powerful and lends itself to tricks, such as referencing variables from the sandbox from the template. It also gives us a point where we could introduce hooks and other crazy behavior if we so wanted.

One thing I don't like about templates is that it constrains us to the model of 1 library per file. I'd like to see us move in a direction where we can consolidate moz.build files.

I'll have to think about it, but I think I'd prefer a model where we had something like:

add_library(component=True, sources=[...], extra_cflags=[...])

Comment 4

[Mike Hommey [:glandium]]

As much as I don't like the 1 library per moz.build thing, I don't like the function model either, because it makes things worse than they are currently for every conditional source, flag, etc.

I'd rather have something like:

l = Library('name')

l.SOURCES += [...]

etc.

And I think that would fit nicely with the model from comment 1, where you'd do:

l.TEMPLATE = 'gecko_component'

and that would import USE_LIBS and IS_COMPONENT from there.

Comment 5

[Mike Hommey [:glandium]]

Note I don't think we should make those template moz.build evaluate within the same sandbox, but have one of their own.

Comment 6

[Gregory Szorc [:gps]]

I can totally get on board with comment #4.

If we do l.TEMPLATE = 'gecko_component', we could do other wackiness, such as making the template thing a callable that receives an instance of Library and hacks it up. e.g.

def gecko_component_library(l):
    l.is_component = True

add_template(Library, 'gecko_component', gecko_component_library)

So many variations here. I'm open to whatever makes things easiest for everybody, especially non-build hackers.

Comment 7

[Joshua Cranmer [:jcranmer]]

I'm slightly averse, bikeshedding-wise, to calling things Library if we do a template route. Given the variety of types of builds (frameworks, executables, bundles, libraries), it makes more sense to me personally to call it a Binary.

But I think I like an interesting variation on comment #4:

lib = GeckoComponent('sample')
lib.sources += []

Comment 8

[Mike Hommey [:glandium]]

(In reply to Joshua Cranmer [:jcranmer] (high latency until August 11) from comment #7)
> I'm slightly averse, bikeshedding-wise, to calling things Library if we do a
> template route. Given the variety of types of builds (frameworks,
> executables, bundles, libraries), it makes more sense to me personally to
> call it a Binary.
> 
> But I think I like an interesting variation on comment #4:
> 
> lib = GeckoComponent('sample')
> lib.sources += []

Yes, that's the conclusion I ended up with independently after thinking about the future. But let's not get too much ahead of ourselves :) We'll get there eventually.

Comment 9

[Mike Hommey [:glandium]]

Attachment: Example

This is what my current PoC allows. The rationale for using a function named after the template when using it is my short-term goal of implementing something along the line of comment 4. One step at a time.

Comment 10

[Mike Hommey [:glandium]]

Attachment: Example

Err, previous patch didn't include all the relevant changes.

Comment 11

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

Comment on attachment 8472817 [details] [diff] [review]
Example

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

::: xpcom/sample/moz.build
@@ +28,5 @@
>      'nsSample.js',
>      'nsSample.manifest',
>  ]
>  
>  # LIBRARY_NAME names the library generated by this makefile,

LIBRARY_NAME no longer

Comment 12

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8472817 [details] [diff] [review]
Example

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

I like!

One question I have: are you planning on doing some sort of inheritance of templates? That could be really useful for some bugs.

Comment 13

[Mike Hommey [:glandium]]

Another syntax idea I just got:

with Library('foo')
    SOURCES += []
    etc.

That's completely non-pythonic (without as l, and without using l.) but that /could/ be nicer than l=Library('foo'); l.SOURCES += [].

Comment 14

[Mike Hommey [:glandium]]

(In reply to Joshua Cranmer [:jcranmer] from comment #12)
> One question I have: are you planning on doing some sort of inheritance of
> templates? That could be really useful for some bugs.

I've been thinking about it. Haven't come up with a nice way to do it yet. Well... here's an idea:

with Template('Foo'):
    USE_LIBS += [...]

with Template('Bar'):
    Foo()
    CFLAGS += [...]

Comment 15

[Gregory Szorc [:gps]]

That is a very clever abuse of context managers. And I think it could work.

Your context manager might even be able to stash away/reset locals() and globals() during execution to provide a completely new (and isolated) sub-sandbox. (Although that property may be implicit in your proposal.)

This is all completely non-pythonic. But this is a very nice backdoor to allow execution of code while keeping declaration consistent and contained.

I'm optimistic this could work. But I'd like to see a little more before fully approving this.

Comment 16

[Gregory Szorc [:gps]]

Comment on attachment 8472817 [details] [diff] [review]
Example

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

Me like.

Comment 17

[Mike Hommey [:glandium]]

Attachment: Add support for templates in moz.build

So, in the end, this uses functions instead of with for declaration of the templates, which will allow some neat things:
- adding a docstring to those functions and generate documentation from there.
- doing smarter things with the template params, like SOURCES += ['%s.cpp' % t for t in CPP_UNIT_TESTS]

I wanted to add docs about how to write templates, but I really don't know where to put that.

Comment 18

[Mike Hommey [:glandium]]

Note for jcranmer: this also allows c-c to define its own templates (as long as the moz.build using them are children of where it's defined in the moz.build DIRS traversal)

Comment 19

[Mike Hommey [:glandium]]

Attachment: Use templates for Gecko XPCOM components

This is what using those templates look like with XPCOM binary components.

Comment 20

[Joshua Cranmer [:jcranmer]]

(In reply to Mike Hommey [:glandium] from comment #18)
> Note for jcranmer: this also allows c-c to define its own templates (as long
> as the moz.build using them are children of where it's defined in the
> moz.build DIRS traversal)

I assume you mean descendants in the traversal tree as opposed to descendants in the filesystem tree. I don't think c-c needs templates (it is either normal Mozilla into-libxul-with-you or XPCOM component libraries, or it's a one-off compilation process), but maybe Instantbird's libpurple may be helped.

One drive-by feedback comment: I see you're using some Python magic to recompile the functions in different contexts... what happens if I were to say, for example:

template.mozbuild:
foo = "eager"
def Template():
  DEFINES['FOO'] = foo

moz.build:
foo = "lazy"
Template()

Does -DFOO=lazy or -DFOO=eager get passed in? Or is there an error? As long as this is ultimately documented, I don't think it matters too much what the correct answer should be (although lazy would be discouraged if you're going to use arguments anyways).

Comment 21

[Mike Hommey [:glandium]]

(In reply to Joshua Cranmer [:jcranmer] from comment #20)
> (In reply to Mike Hommey [:glandium] from comment #18)
> > Note for jcranmer: this also allows c-c to define its own templates (as long
> > as the moz.build using them are children of where it's defined in the
> > moz.build DIRS traversal)
> 
> I assume you mean descendants in the traversal tree as opposed to
> descendants in the filesystem tree. I don't think c-c needs templates (it is
> either normal Mozilla into-libxul-with-you or XPCOM component libraries, or
> it's a one-off compilation process), but maybe Instantbird's libpurple may
> be helped.
> 
> One drive-by feedback comment: I see you're using some Python magic to
> recompile the functions in different contexts... what happens if I were to
> say, for example:
> 
> template.mozbuild:
> foo = "eager"
> def Template():
>   DEFINES['FOO'] = foo
> 
> moz.build:
> foo = "lazy"
> Template()

That's covered in the tests, that's expected to be throwing an error.

Comment 22

[Gregory Szorc [:gps]]

Comment on attachment 8475047 [details] [diff] [review]
Add support for templates in moz.build

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

Oh no, you've discovered the "inspect" and "tokenize" modules.

I'm mostly happy with this code. I'd like to see some documentation, both inline (because this code is advanced) and some RST in build/docs on how templates work generally.

I also think we need an escape hatch that allows the co-existence of template functions and regular functions. I left some options in an inline comment. We could bikeshed those or you could implement one and I'd likely give it r+. Anything is better than nothing.

::: python/mozbuild/mozbuild/frontend/reader.py
@@ +149,5 @@
> +                self.templates[key].declared_path))
> +        if inspect.isfunction(value):
> +            if key.islower() or key.isupper() or key[0].islower():
> +                raise Exception('Template function names must be CamelCase.')
> +            self.templates[key] = self._create_template(value)

So, file-level symbols that are functions automatically get turned into templates? If so, this at least needs documentation.

I'm a bit concerned that this precludes the future use of functions within moz.build files. What if a moz.build file needs a function to provide utility code?

I almost wonder if we should establish some kind of convention for declaring templates so that there is no ambiguity. Ideas:

* Template function names must begin with a well-defined string, such as "Template"
* Template functions must be decorated with @template
* Functions only magically get turned into templates during an include('file', templates=True) processing mode.

Need some more thought here.

@@ +312,5 @@
>  
>      def _error(self, message):
>          raise SandboxCalledError(self._execution_stack, message)
>  
> +    def _create_template(self, func):

Please docstring this.

@@ +343,5 @@
> +        # n           FOO = 'bar'
> +        # n+1         (...)
> +        #
> +        # (this is simpler than trying to deindent the function body)
> +        # So we need to preprend with n - 1 newlines so that line numbers

typo: "preprend"

@@ +351,5 @@
> +
> +        return func, code, self._execution_stack[-1]
> +
> +    @memoize
> +    def _create_template_function(self, name):

Please document.

@@ +367,5 @@
> +
> +            # In case this becomes too expensive to recompile the same code
> +            # over and over again, we can compile it in _create_template and
> +            # make exec_source accept compiled code directly.
> +            sandbox.exec_source(code, path)

Compiling should be extremely fast. I think we'd need to do pretty much everything in templates for this to be a performance concern.

@@ +375,5 @@
> +                    self[key].update(value)
> +                elif isinstance(value, list):
> +                    self[key] += value
> +                else:
> +                    self[key] = value

I initially thought that |self[key] = value| would leave the door open for subtle breakage due to new types. But MozbuildSandbox.__setitem__ should forbid it. Perhaps that should be documented?

::: python/mozbuild/mozbuild/test/frontend/test_sandbox.py
@@ +453,5 @@
> +        self.assertEqual(sandbox2._context, {
> +            'SOURCES': ['qux.cpp', 'bar.cpp', 'foo.cpp', 'hoge.cpp'],
> +            'USE_LIBS': ['foo'],
> +            'DIRS': [],
> +        })

Please also add a test that misnamed functions result in appropriate error.

Comment 23

[Mike Hommey [:glandium]]

> * Template functions must be decorated with @template

I like this idea.

Comment 24

[Gregory Szorc [:gps]]

Comment on attachment 8475051 [details] [diff] [review]
Use templates for Gecko XPCOM components

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

I love the user-visible improvement! I'm still kind of curious where all this work is going. So far, this is little different from functions added manually in sandbox.py and reader.py. Hopefully we can find time to chat in Munich next week.

We'll want to hook the Sphinx docs up to this. That will likely require a custom Sphinx directive (python/mozbuild/mozbuild/sphinx.py). We currently don't read moz.build as part of Sphinx itself. Instead, we read moz.build to see what directories need Sphinx attention and then we run Sphinx over them (tools/docs/mach_commands.py). We'll somehow need to pass template information to Sphinx or have Sphinx do moz.build processing itself. I'm fine with a giant hack that stuffs template info in a global variable where Sphinx can find it.

::: build/templates.mozbuild
@@ +6,5 @@
> +
> +def GeckoBinary():
> +    '''Template for binaries using Gecko.
> +
> +    This template is meant to be used in other templates.

If we end up with a lot of "hidden" templates, I wonder if we should introduce a decorator for them so docs can hide them.

Comment 25

[Joshua Cranmer [:jcranmer]]

(In reply to Gregory Szorc [:gps] from comment #24)
> I love the user-visible improvement! I'm still kind of curious where all
> this work is going.

I've mentioned on IRC before that I'm interested in leveraging templates to move config.mk logic out of Makefile syntax and into something more pythonic, although I may wait for c = Library()-ish syntax before doing that. I can also foresee this being used to, e.g., get rid of the DISABLE_STL_WRAPPING flag by converting it to template-based CXXFLAGS munging. That's just where I want to go, though.

Comment 26

[Gregory Szorc [:gps]]

Yeah, I've heard those plans too. I'm just kinda curious if there is any benefit over "just add methods to MozbuildSandbox." I worry we're focused on making the sandbox dynamic and generic instead of solving Mozilla's pressing needs.

At this time, I view templating as an easier way to define functions in a moz.build sandbox. But build peers are the only ones defining functions on sandboxes, so there isn't a clear advantage to this method to me. I'm willing to let it play out. I trust myself and others to put the brakes on the slippery slope.

Comment 27

[Mike Hommey [:glandium]]

Templates could sure be functions in the python code, but mixing them with other code is not really helpful for clarity. Also, the syntax for them in python code would be dreadful.

Anyways, the main things I planned using templates for is mozglue/stl wrapping, and multiple-library/program per directory. I'm also thinking about killing the *WithFlags classes and using sub-contexts there too, which would bring us better type checking for those flags. But that's longer term.

Comment 28

[Mike Hommey [:glandium]]

Attachment: Add support for templates in moz.build

I think this addresses your concerns.

Comment 29

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/313a81600191
https://hg.mozilla.org/integration/mozilla-inbound/rev/62b9f6d4328b

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/313a81600191
https://hg.mozilla.org/mozilla-central/rev/62b9f6d4328b

Comment 31

[Mike Hommey [:glandium]]

Attachment: Use templates for programs, simple programs, libraries and C++ unit tests

This moves PROGRAM, SIMPLE_PROGRAMS, LIBRARY_NAME, CPP_UNIT_TESTS and the corresponding HOST_* variables to use templates.
This looks like it's useless, but it is a transitional state, and I won't go further in this bug. I'll file followups to add stdc++compat, stlport and mozglue to the relevant templates, which is why this patch rejects non-templated setting of those variables.

This does not include the corresponding moz.build changes. I'll attach those separately, although I'll land everything as one changeset.

Comment 32

[Mike Hommey [:glandium]]

Attachment: Replace all uses of PROGRAM, SIMPLE_PROGRAMS, CPP_UNIT_TESTS, LIBRARY_NAME, etc. with the corresponding template

Note this was automatically translated, and I'm going to attach the script that was used for that.

Comment 33

[Mike Hommey [:glandium]]

Attachment: Script used to create attachment 8479620

Comment 34

[Gregory Szorc [:gps]]

Comment on attachment 8479618 [details] [diff] [review]
Use templates for programs, simple programs, libraries and C++ unit tests

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

I'm curious where you are going with this.

The comment in reader.py implies you will be emitting separate contexts for each template invocation. That's cool. Should allow multiple libraries, etc per directory. Very nice!

The approach in context.py to identify certain variables as template-only variables seems a bit weird to me. Here are some alternative ideas to consider:

1) Declare TEMPLATE_VARIABLES in a separate list from VARIABLES. Concatenate the set together everywhere VARIABLES is used today.
2) Remove template variables from VARIABLES all-together. Allow variable assignments in template contexts to just work, skipping the "is key allowed" checks.

I'm just not sure what value having things still listed in VARIABLES is buying us.

Again, I'm not sure where you are going with this and would appreciate more context. Please share in Munich!

One possibility I see is this ultimately culminating with template functions emitting classes and moving the custom logic from emitter.py/data.py into templates.mozbuild. That would remove a layer of abstraction and make things much easier to reason about. Do tell your intentions!

I'm granting review because I think I see where you're going and like the direction of making binaries easier to define and process.

::: python/mozbuild/mozbuild/frontend/context.py
@@ +951,5 @@
>          raise RuntimeError('%s has a "list" storage type. Use "List" instead.'
>              % name)
>  
> +# Set of variables that are only allowed in templates:
> +TEMPLATE_VARIABLES = {

Shouldn't this be a flag on entries in VARIABLES?

Comment 35

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01a0e2c9c595

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/01a0e2c9c595