778236 - Integrate gyp->makefile translation in ConfigStatus.py

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Gregory Szorc [:gps]]

I found a way to address bug 784841 without touching this.

Comment 2

[Gregory Szorc [:gps]]

Attachment: Process GYP projects during config.status

Here's my first stab at it. My goal for this patch was getting something
that just worked. I believe I have achieved that goal.

We declare GYP files in moz.build. During backend generation time,
defined GYP files result in a process invocation to invoke GYP.

This patch is incomplete in a number of ways.

There are no tests.

There are no dependencies. Backend generation now takes about 4s longer
because of this.

We could probably invoke GYP as a native Python function call instead of
invoking a separate process.

It's not very polished.

You get the idea.

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 794349 [details] [diff] [review]
Process GYP projects during config.status

Would like to find time to take a look, but my queue is still sort of a mess.

Comment 4

[Gregory Szorc [:gps]]

Attachment: Process GYP projects during config.status

This patch is a little more robust. GYP-derived Makefiles are now fully
integrated with config.status, dependencies and all. I got rid of the
custom Makefile rule in the GYP-generated common.mk files. Dependency
files are now tracked via the existing backend.RecursiveMakeBackend.pp
rule. The method by which we communicate the set of inputs for each GYP
project is a bit hacky, but it gets the job done.

While I was working on this patch, I noticed an existing bug in the GYP
generator: we failed to include all dependency/input files for GYP
files. We were only enumerating primary "build files" and were not
including the "include files" these relied on. I'm not sure if we have a
bug on that or what.

This patch still kinda sucks. The biggest drawback is reading the GYP
files is slow - this patch adds ~4s of processing time to moz.build
processing. We had just got this down to ~2s from ~4s and now it's
around ~6s with this patch. That kinda sucks. I dare say it's too slow
and we should consider splitting GYP reading into its own mini branch
mostly independent of moz.build reading. It's possible that if we called
GYP as a native Python function call that things would be faster. I
haven't tried yet. That's definitely something we should consider doing
before landing this in any form.

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 794527 [details] [diff] [review]
Process GYP projects during config.status

Ted wants to look at this despite has massive review queue.

Comment 6

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #4)
> This patch still kinda sucks. The biggest drawback is reading the GYP
> files is slow - this patch adds ~4s of processing time to moz.build
> processing. We had just got this down to ~2s from ~4s and now it's
> around ~6s with this patch. That kinda sucks. I dare say it's too slow

Your data is kinda flawed. moz.build processing sure was ~2s, but gyp processing still had to be done at some point, whether in configure during which it was already slow, or during directory traversal under webrtc, in which case it probably had to be done more than necessary since bug 903341, which is overall slower, despite being hidden deep in the build.

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 794527 [details] [diff] [review]
Process GYP projects during config.status

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

::: configure.in
@@ +9138,5 @@
> +   fi
> +
> +   GYP_WEBRTC_OPTIONS="--format=mozmake ${WEBRTC_CONFIG} -D target_arch=${WEBRTC_TARGET_ARCH} ${EXTRA_GYP_DEFINES} --depth=${srcdir}/media/webrtc/trunk --toplevel-dir=${srcdir} -G OBJDIR=${_objdir}"
> +
> +   AC_SUBST(GYP_WEBRTC_OPTIONS)

IMHO this whole thing shouldn't be in configure.in at all, although that could be done in a followup.

Comment 8

[(not currently active) Ted Mielczarek]

bug 812311 for the .gypi issue, FYI.

Comment 9

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

Comment on attachment 794527 [details] [diff] [review]
Process GYP projects during config.status

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

::: python/mozbuild/mozbuild/frontend/data.py
@@ +211,5 @@
> +
> +class GYPGeneration(SandboxDerived):
> +    """Represents a GYP invocation to process a .gyp file.
> +
> +    This isn't a completely generic GYP wrapper. Instead, we assume we are

Can we assert that somehow?

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +162,5 @@
>  
>          for ipdl in sandbox.get('IPDL_SOURCES', []):
>              yield IPDLFile(sandbox, ipdl)
>  
> +        # Our GYP handling is currently suboptimal. We eventually invoke the

Add a bug number

@@ +170,5 @@
> +        # us, effectively merging GYP's build config data into our sandbox
> +        # data. Then, the backend would consume build config data as if it came
> +        # from moz.build sandboxes.
> +        for gyp in sandbox.get('GYP_FILES', []):
> +            gyp_path, out_path, config, build_for_test = gyp

for gyp_path, out_path, config, build_for_test in ...?

::: toolkit/toolkit.mozbuild
@@ +70,5 @@
>          'media/mtransport/build',
>          'media/mtransport/standalone',
>      ])
>  
> +    gyp_file('media/webrtc/trunk/peerconnection.gyp', 'media/webrtc/trunk',

Would be nice if this could all live closer to the actual code

Comment 10

[Mike Hommey [:glandium]]

I'm starting to wonder if we shouldn't, instead, process gyp files when we import them, and generate moz.build files out of them. That would sidestep the "gyp is slow" issue, and might allow better hackability (and we wouldn't have to change the gyp files for our needs, we could do that in moz.build)

Comment 11

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #10)
> I'm starting to wonder if we shouldn't, instead, process gyp files when we
> import them, and generate moz.build files out of them. That would sidestep
> the "gyp is slow" issue, and might allow better hackability (and we wouldn't
> have to change the gyp files for our needs, we could do that in moz.build)

At the rate we're going with support for moz.build variables, that is tempting. Alternatively, we could have a gyp generator emit the mozbuild.frontend.data.* classes. Or, we could even have a generator that writes a pickle containing all those class instances. When we config.status, we just combine the streams together. That approach means the build backend has complete knowledge and we don't have to treat the gyp directories specially.

Comment 12

[Mike Hommey [:glandium]]

I'm going to get this to the next level of integration.

Comment 15

[Mike Hommey [:glandium]]

Attachment: Treat gyp files as if their content was defined in moz.build files

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

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8345920 [details] [diff] [review]
Treat gyp files as if their content was defined in moz.build files

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

::: configure.in
@@ +5074,3 @@
>  # target_arch is from {ia32|x64|arm|ppc}
>  case "$CPU_ARCH" in
> +x86_64 | arm | x86 | ppc* | ia64)

Note we explicitely set CPU_ARCH to arm, and not armsomething, so the wildcard was useless.

Comment 17

[Mike Hommey [:glandium]]

Comment on attachment 8345920 [details] [diff] [review]
Treat gyp files as if their content was defined in moz.build files

Randell, can you take a look at the .gyp/.gypi changes, there are a bit more than what i pasted on irc.

(Also, you may want to look overall what this means for your workflow)

Comment 18

[Mike Hommey [:glandium]]

Attachment: Remove EXTERNAL_MAKE_DIRS and PARALLEL_EXTERNAL_MAKE_DIRS

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8345920 [details] [diff] [review]
Treat gyp files as if their content was defined in moz.build files

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

Thanks!!  Much cleaner overall!

Comment 20

[Mike Hommey [:glandium]]

Note that one thing that's missing in the patch is a thorough documentation of the GYP_FILES variable, which i'm not sure how to articulate.

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 8345920 [details] [diff] [review]
Treat gyp files as if their content was defined in moz.build files

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

Overall this looks amazing. Just a few things to address.

Can you move gyp.mozbuild out of the root directory? Maybe build/gyp.mozbuild? I don't like seeing files in the root directory because it confuses first-time developers. This especially holds true for files that look like build system files.

Throughout gyp_reader.py, you are assigning str to the sandbox values. This includes keys (literal '' in this module are str, not unicode). Since everything in moz.build land uses unicode and not str, you should .decode('utf-8') or use u'' literals everywhere. This strictly doesn't need to be done today, but I'm not a fan of introducing technical debt.

Also, I haven't verified everything from configure.in was ported to the new configs properly. I'll post a 2nd comment to go over these. I don't want to block you on incorporating feedback so far.

::: configure.in
@@ +465,5 @@
>              _MSVS_VERSION=2013
>          else
>              AC_MSG_ERROR([This version ($CC_VERSION) of the MSVC compiler is unsupported. See https://developer.mozilla.org/en/Windows_Build_Prerequisites.])
>          fi
> +	AC_SUBST(_MSVS_VERSION)

Must this be prefixed?

::: gyp.mozbuild
@@ +89,5 @@
> +
> +if CONFIG['ARM_ARCH']:
> +    # We currently don't have a way to convert a string to an int in moz.build.
> +    # As of writing, ARM_ARCH is not going to be over 8, so a string comparison
> +    # works.

We can easily expose int, float, etc to the sandbox's __builtins__. Should be safe.

::: media/mtransport/third_party/moz.build
@@ +14,5 @@
> +GYP_DIRS['nICEr'].input = 'nICEr/nicer.gyp'
> +
> +GYP_DIRS['nrappkit'].input = 'nrappkit/nrappkit.gyp'
> +
> +GYP_DIRS['nICEr'].variables = GYP_DIRS['nrappkit'].variables = gyp_vars

Split into two statements like so:

GYP_DIRS['nICEr'].input = 'nICEr/nicer.gyp'
GYP_DIRS['nIceR'].variables = gyp_vars

GYP_DIRS['nrappkit'].input = '...'
                    .variables = gyp_vars

::: media/webrtc/trunk/tools/gyp/pylib/gyp/input.py
@@ +1424,5 @@
>      self.dependencies = []
>      self.dependents = []
>  
> +  def __hash__(self):
> +    return hash(self.ref)

Add a comment explaining this is a Mozilla change so diffs of future imports of GYP into the source tree will reveal this shouldn't be removed.

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +216,5 @@
>              OS_LIBS='OS_LIBS',
>              SDK_LIBRARY='SDK_LIBRARY',
>          )
>          for mak, moz in varmap.items():
> +            if moz in sandbox and sandbox[moz]:

Wait, are we not passing through False if a variable is set to that? Shouldn't this check be |if moz in sandbox|?

::: python/mozbuild/mozbuild/frontend/gyp_reader.py
@@ +27,5 @@
> +# chrome_src for the default includes, so go backwards from the pylib
> +# directory, which is the parent directory of gyp module.
> +chrome_src = mozpath.abspath(mozpath.join(mozpath.dirname(gyp.__file__),
> +    '../../../..'))
> +script_dir = mozpath.join(chrome_src, 'build')

Ugh. I'd feel much better if this could be passed in somehow. This will likely break when .py files are loaded from inside the virtualenv, not topsrcdir. I suppose we can tackle it later when the virtualenv is refactored.

@@ +46,5 @@
> +               'LINKER_SUPPORTS_ICF']:
> +  generator_default_variables[unused] = ''
> +
> +
> +class GypSandbox(GlobalNamespace):

Inheriting from GlobalNamespace instead of Sandbox seems wrong. That being said, Sandbox is essentially a proxy to a GlobalNamespace instance, so I suppose this is fine. You don't need the extra features Sandbox provides, after all. Please at least document that this is attempting to recreate part of the API for MozbuildSandbox that is used for ???.

@@ +60,5 @@
> +
> +
> +def encode(value):
> +    if isinstance(value, unicode):
> +        return value.encode()

.encode('utf-8') otherwise it defaults to ascii.

@@ +64,5 @@
> +        return value.encode()
> +    return value
> +
> +
> +def read_from_gyp(config, path, output, vars):

Document!

@@ +67,5 @@
> +
> +def read_from_gyp(config, path, output, vars):
> +    # gyp expects plain str instead of unicode. The frontend code gives us
> +    # unicode strings, so convert them.
> +    path = path.encode()

'utf-8'

@@ +79,5 @@
> +
> +    # Files that gyp_chromium always includes
> +    includes = [mozpath.join(script_dir, 'common.gypi')]
> +    includes.extend(mozpath.join(chrome_src, name)
> +        for name, _ in FileFinder(chrome_src).find('*/supplement.gypi'))

Should pass ignore_executables (or whatever the flag is) to make FileFinder faster. Maybe we should just change the default flag value.

@@ +127,5 @@
> +        # Derive which gyp configuration to use based on MOZ_DEBUG.
> +        if config.substs['MOZ_DEBUG']:
> +            c = 'Debug'
> +        else:
> +            c = 'Release'

c = 'Debug' if config.substs['MOZ_DEBUG'] else 'Release' ?

@@ +140,5 @@
> +            sandbox['FORCE_STATIC_LIB'] = True
> +            # Remove leading 'lib' from the target_name if any, and use as
> +            # library name.
> +            name = spec['target_name']
> +            name = name[3 if name.startswith('lib') else 0:]

Trying to make this look like Perl? ;)

@@ +142,5 @@
> +            # library name.
> +            name = spec['target_name']
> +            name = name[3 if name.startswith('lib') else 0:]
> +            # The sandbox expects an unicode string.
> +            sandbox['LIBRARY_NAME'] = name.decode()

'utf-8'

@@ +152,5 @@
> +
> +            for define in target_conf.get('defines', []):
> +                if '=' in define:
> +                    name, value = define.split('=', 1)
> +                    sandbox['DEFINES'][name] = value

Assign unicode.

@@ +157,5 @@
> +                else:
> +                    sandbox['DEFINES'][define] = True
> +
> +            for include in target_conf.get('include_dirs', []):
> +                sandbox['LOCAL_INCLUDES'] += [include]

Assign unicode.

@@ +166,5 @@
> +        else:
> +            # Ignore other types than static_library because we don't have
> +            # anything using them, and we're not testing them. They can be
> +            # added when that becomes necessary.
> +            raise RuntimeError('Unsupported gyp target type: %s' % spec['type'])

raise NotImplementedError

::: python/mozbuild/mozbuild/frontend/reader.py
@@ +722,5 @@
> +                    raise Exception('Missing value for GYP_DIRS["%s"].%s' %
> +                        (target_dir, v))
> +
> +            # The make backend assumes sandboxes for sub-directories are
> +            # emitted after their parent, so accumulate the gyp sandbox.

Why can't you just move |yield sandbox| to before GYP_DIRS processing? Document it if there's a good reason.

@@ +736,5 @@
> +        # parallelized, and the libs tier is always serialized, and will remain
> +        # so until the library linking operations are moved out of it, at which
> +        # point PARALLEL_DIRS will be irrelevant anyways.
> +        for gyp_sandbox in gyp_sandboxes:
> +            sandbox['DIRS'].append(mozpath.relpath(gyp_sandbox['OBJDIR'], sandbox['OBJDIR']))

Oh, that's the reason. Comment before the "yield sandbox| please.

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +575,5 @@
> +    'GYP_DIRS': (StrictOrderingOnAppendListWithFlagsFactory({
> +            'variables': dict,
> +            'input': unicode,
> +        }), list,
> +        """Defines a list of gyp files to process.

As you mentioned, this needs to be documented better.

Comment 22

[Gregory Szorc [:gps]]

Comment on attachment 8345920 [details] [diff] [review]
Treat gyp files as if their content was defined in moz.build files

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

My Mercurial hook that checks Python style is complaining:

python/mozbuild/mozbuild/frontend/gyp_reader.py:106:1: F841 local variable 'topobjdir' is assigned to but never used
python/mozbuild/mozbuild/frontend/reader.py:713:1: F812 list comprehension redefines 'var' from line 693

Comment 23

[Gregory Szorc [:gps]]

Looks like GYP processing time isn't tracked, so it's getting reported as "untracked." Previously, my machine had about 0.1s of untracked time during config.status. So, I guess GYP processing added about 1.4s to config.status time. Or, processing about 1/20th of the tree now consumes ~77% of the CPU time required to process the remaining parts of the tree. And people wonder why we aren't using GYP to declare our build config data!

---

Finished reading 2946 moz.build files in 0.33s
Processed into 6520 build config descriptors in 1.48s
Backend executed in 1.70s
2757 total backend files. 0 created; 0 updated; 2757 unchanged
Total wall time: 5.01s; CPU time: 4.55s; Efficiency: 91%; Untracked: 1.50s

Comment 24

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] (in southeast Asia until Dec 14) from comment #21)
> ::: python/mozbuild/mozbuild/frontend/emitter.py
> @@ +216,5 @@
> >              OS_LIBS='OS_LIBS',
> >              SDK_LIBRARY='SDK_LIBRARY',
> >          )
> >          for mak, moz in varmap.items():
> > +            if moz in sandbox and sandbox[moz]:
> 
> Wait, are we not passing through False if a variable is set to that?
> Shouldn't this check be |if moz in sandbox|?

moz in sandbox always is true for things in allowed_variables, because __getitem__ returns an empty item. So the sandbox[moz] test is still necessary if we don't want to pollute backend.mk with useless assignments.

But moz in sandbox is necessary because it raises for things not in allowed_variables, which EXTRA_*_FLAGS are (I don't want to expose them to moz.build)

> ::: python/mozbuild/mozbuild/frontend/gyp_reader.py
> @@ +27,5 @@
> > +# chrome_src for the default includes, so go backwards from the pylib
> > +# directory, which is the parent directory of gyp module.
> > +chrome_src = mozpath.abspath(mozpath.join(mozpath.dirname(gyp.__file__),
> > +    '../../../..'))
> > +script_dir = mozpath.join(chrome_src, 'build')
> 
> Ugh. I'd feel much better if this could be passed in somehow. This will
> likely break when .py files are loaded from inside the virtualenv, not
> topsrcdir. I suppose we can tackle it later when the virtualenv is
> refactored.

Note they are not paths for .py loading, they are paths to find the default .gypi files.

Comment 25

[Gregory Szorc [:gps]]

Comment on attachment 8345920 [details] [diff] [review]
Treat gyp files as if their content was defined in moz.build files

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

I looked over the configure.in to new world order conversions and they seem fine AFAICT. They are the kind of thing I think we'd notice pretty quickly if we got wrong anyway.

Comment 26

[Mike Hommey [:glandium]]

Attachment: Treat gyp files as if their content was defined in moz.build files

Comment 27

[Mike Hommey [:glandium]]

Pushed to try for good measure, because of the unicode changes in gyp_reader:
https://tbpl.mozilla.org/?tree=Try&rev=3f460c562531

Comment 28

[Mike Hommey [:glandium]]

Attachment: Remove EXTERNAL_MAKE_DIRS and PARALLEL_EXTERNAL_MAKE_DIRS

Fixes a small indentation problem.

Comment 29

[Gregory Szorc [:gps]]

Comment on attachment 8346383 [details] [diff] [review]
Treat gyp files as if their content was defined in moz.build files

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

There are still some implicit str <-> unicode conversions. But, we have plenty of these elsewhere in the build system. When this becomes an issue, at least we can isolate gyp_reader.py as the source.

Let's land this [after a green try run].

::: python/mozbuild/mozbuild/frontend/gyp_reader.py
@@ +110,5 @@
> +
> +    # Process all targets from the given gyp files and its dependencies.
> +    # The path given to AllTargets needs to use os.sep, while the frontend code
> +    # gives us paths normalized with forward slash separator.
> +    for target in gyp.common.AllTargets(flat_list, targets, path.replace('/', os.sep)):

The '' literal will leak a unicode into GYP land.

@@ +143,5 @@
> +        spec = targets[target]
> +
> +        # Derive which gyp configuration to use based on MOZ_DEBUG.
> +        c = 'Debug' if config.substs['MOZ_DEBUG'] else 'Release'
> +        if c not in spec['configurations']:

I guess Python coerces the unicode literal into a str for the "in" comparison. Oh, Python.

@@ +148,5 @@
> +            raise RuntimeError('Missing %s gyp configuration for target %s '
> +                               'in %s' % (c, target_name, build_file))
> +        target_conf = spec['configurations'][c]
> +
> +        if spec['type'] == 'none':

b'none'

@@ +150,5 @@
> +        target_conf = spec['configurations'][c]
> +
> +        if spec['type'] == 'none':
> +            continue
> +        elif spec['type'] == 'static_library':

b'type' b'static_library'

@@ +155,5 @@
> +            sandbox['FORCE_STATIC_LIB'] = True
> +            # Remove leading 'lib' from the target_name if any, and use as
> +            # library name.
> +            name = spec['target_name']
> +            if name.startswith('lib'):

More implicit conversions.

@@ +165,5 @@
> +            # gyp files contain headers in sources lists.
> +            sandbox['SOURCES'] = \
> +                [f for f in sources if mozpath.splitext(f)[-1] != '.h']
> +
> +            for define in target_conf.get('defines', []):

b'defines'

@@ +166,5 @@
> +            sandbox['SOURCES'] = \
> +                [f for f in sources if mozpath.splitext(f)[-1] != '.h']
> +
> +            for define in target_conf.get('defines', []):
> +                if '=' in define:

b'='

@@ +173,5 @@
> +                else:
> +                    sandbox['DEFINES'][define] = True
> +
> +            for include in target_conf.get('include_dirs', []):
> +                sandbox['LOCAL_INCLUDES'] += [include]

I'll stop writing string conversion foo.

Comment 30

[Gregory Szorc [:gps]]

Comment on attachment 8346386 [details] [diff] [review]
Remove EXTERNAL_MAKE_DIRS and PARALLEL_EXTERNAL_MAKE_DIRS

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

Beautiful.

Comment 31

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/688d526f9313
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5bb944954a5

Comment 32

[Gregory Szorc [:gps]]

\o/

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/688d526f9313
https://hg.mozilla.org/mozilla-central/rev/f5bb944954a5