make moz.build *SOURCES variables emit proper objects

RESOLVED FIXED in mozilla37

Status

()

Core
Build Config
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla37
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Having SOURCES and its close relatives go through VariablePassthru
objects clutters the handling of VariablePassthru in build backends and
makes it less obvious how to handle things that actually get compiled.

Therefore, this patch introduces four new moz.build objects
corresponding to the major variants of SOURCES.  It looks like a large
patch, but there's an ample amount of new tests included, which accounts
for about half of the changes.
(Assignee)

Comment 1

3 years ago
Created attachment 8533965 [details] [diff] [review]
make moz.build *SOURCES variables emit proper objects

Having SOURCES and its close relatives go through VariablePassthru
objects clutters the handling of VariablePassthru in build backends and
makes it less obvious how to handle things that actually get compiled.

Therefore, this patch introduces four new moz.build objects
corresponding to the major variants of SOURCES.  It looks like a large
patch, but there's an ample amount of new tests included, which accounts
for about half of the changes.

I think this is necessary for bug 1108750...but at a minimum, it makes my head
hurt a little less looking at emitter.py and recursivemake.py.  I think the
visualstudio.py changes are correct, but I haven't tested them in any way.
Tests pass on x86-64 Linux and things even seem to build correctly.
Attachment #8533965 - Flags: review?(gps)
(Assignee)

Updated

3 years ago
Blocks: 1108750
(Assignee)

Comment 2

3 years ago
Comment on attachment 8533965 [details] [diff] [review]
make moz.build *SOURCES variables emit proper objects

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

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +471,5 @@
> +        # kinds that can be listed therein.
> +        all_suffixes = list(suffix_map.keys())
> +        varmap = dict(
> +            SOURCES=(Sources, all_suffixes),
> +            HOST_SOURCES=(HostSources, ['.c', '.mm', '.cpp']),

These lists should probably be hung off the *Sources classes as class variables instead.  That would allow their __init__ methods to check that the canonical suffix is actually supported, which would in turn enable better and more robust tests.

@@ +485,5 @@
> +            for f in context[variable]:
> +                ext = mozpath.splitext(f)[1]
> +                if ext not in allowed_suffixes:
> +                    raise SandboxValidationError(
> +                        '%s has an unknown file type.' % f, context)

We should maybe have a test for this, with likely-to-be-used-but-not-yet extensions (.rs, swift's, c#'s?) ?

Comment 3

3 years ago
Comment on attachment 8533965 [details] [diff] [review]
make moz.build *SOURCES variables emit proper objects

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

Nice work. Just nits. Not enough to hold back r+.

And so you have the context, most things in VariablePassthru were done that way for expedience. We didn't want bikeshedding on proper semantics to block moving things to moz.build. The reasoning is that anything in moz.build is better than Makefile.in.

In the ideal world, VariablePassthru should not exist: it is a layering violation between the abstraction of build config and build backend. Every piece of build config should be captured by a dedicated class or similar data structure. You won't hear me complain when things are moved out of VariablePassthru :)

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +362,5 @@
>              # Other ConfigFileSubstitution should have been acked by
>              # CommonBackend.
>              assert os.path.basename(obj.output_path) == 'Makefile'
>              self._create_makefile(obj)
> +        elif isinstance(obj, Sources) or isinstance(obj, GeneratedSources):

isinstance() can take a tuple of types. e.g.

  isinstance(obj, (Sources, GeneratedSources))

Please change.

::: python/mozbuild/mozbuild/frontend/data.py
@@ +752,5 @@
> +    """Represents generated files to be compiled during the build."""
> +
> +    def __init__(self, context, files, canonical_suffix):
> +        BaseSources.__init__(self, context, files, canonical_suffix)
> +        

Nit: whitespace

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +452,5 @@
> +            '.s': set(['.asm']),
> +            '.c': set(),
> +            '.m': set(),
> +            '.mm': set(),
> +            '.cpp': set(['.cc', '.cxx']),

We require Python 2.7 and 2.7 has a nice set literal syntax:

   {'.cc', '.cxx'}

Some people don't like it because {} is also used for dicts. I think the syntax here is fine. But you can change post review if you want.

@@ +471,5 @@
> +        # kinds that can be listed therein.
> +        all_suffixes = list(suffix_map.keys())
> +        varmap = dict(
> +            SOURCES=(Sources, all_suffixes),
> +            HOST_SOURCES=(HostSources, ['.c', '.mm', '.cpp']),

I'm fine with the mapping here or as class variables.

@@ +477,5 @@
> +            GENERATED_SOURCES=(GeneratedSources, all_suffixes),
> +        )
> +
> +        for variable, (klass, suffixes) in varmap.items():
> +            allowed_suffixes = set().union(*[suffix_map[s] for s in suffixes])

That's a clever one-liner. Didn't realize .union(*args) worked like that. Nifty.

@@ +485,5 @@
> +            for f in context[variable]:
> +                ext = mozpath.splitext(f)[1]
> +                if ext not in allowed_suffixes:
> +                    raise SandboxValidationError(
> +                        '%s has an unknown file type.' % f, context)

You won't hear me say "no" to tests. At the same time, build breakage is typically obvious, so tests often amount to guaranteeing a sane user experience when things are wrong. It is difficult to assess the value of this. We typically let lack of tests slide.
Attachment #8533965 - Flags: review?(gps) → review+
(Assignee)

Comment 4

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ff7bcdb1271

I opted to leave the list of acceptable suffixes where it was.
Flags: in-testsuite-

Comment 5

3 years ago
I'm building from -inbound so I have 7ff7bcdb1271 in my tree.
Running mach build-backend --backend=VisualStudio causes the following error:

Philip@LENOVO-PC /c/t1/hg/comm-central
$ ./mozilla/mach build-backend --backend=VisualStudio
 0:07.75 c:\t1\hg\objdir-sm\_virtualenv\Scripts\python.exe c:\t1\hg\objdir-sm\config.status --backend=VisualStudio
Reticulating splines...
Traceback (most recent call last):
  File "c:\t1\hg\objdir-sm\config.status", line 954, in <module>
    config_status(**args)
  File "c:\t1\hg\comm-central\mozilla\python\mozbuild\mozbuild\config_status.py", line 148, in config_status
    summary = the_backend.consume(definitions)
  File "c:\t1\hg\comm-central\mozilla\python\mozbuild\mozbuild\backend\base.py", line 184, in consume
    self.consume_object(obj)
  File "c:\t1\hg\comm-central\mozilla\python\mozbuild\mozbuild\backend\visualstudio.py", line 104, in consume_object
    self._add_sources(self, reldir, obj)
TypeError: _add_sources() takes exactly 3 arguments (4 given)

Updated

3 years ago
Depends on: 1113730
https://hg.mozilla.org/mozilla-central/rev/7ff7bcdb1271
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.