948275 - Refactor handling of CONFIGURE_SUBST_FILES and CONFIGURE_DEFINE_FILES

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

As being inherited from the first python implementation of config.status, ConfigEnvironment is not really nicely integrated with build backend stuff. Moreover, it assumes source directories and object directories are always equal, which doesn't work out for bug 778236.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Refactor handling of CONFIGURE_SUBST_FILES and CONFIGURE_DEFINE_FILES

Comment 2

[Mike Hommey [:glandium]]

Attachment: Refactor handling of CONFIGURE_SUBST_FILES and CONFIGURE_DEFINE_FILES

Forgot to add a comment I wanted to add about the CommonBackend stuff.

Comment 3

[Mike Hommey [:glandium]]

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

Comment 4

[Mike Hommey [:glandium]]

Attachment: Use mozpack.path instead of os.path in mozbuild.frontend and mozbuild.backend modules

Comment 5

[Mike Hommey [:glandium]]

Attachment: Refactor handling of CONFIGURE_SUBST_FILES and CONFIGURE_DEFINE_FILES

Rebased on top of the mozpack.path change.

Comment 6

[Mike Hommey [:glandium]]

https://hg.mozilla.org/try/rev/20a9f634e095

Comment 7

[Mike Hommey [:glandium]]

Note these patches are on top of https://bugzilla.mozilla.org/attachment.cgi?id=828531 which didn't land because in the end i didn't need it at the time. While that patch is not necessary here, it's necessary for bug 778236 anyways.

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8345640 [details] [diff] [review]
Refactor handling of CONFIGURE_SUBST_FILES and CONFIGURE_DEFINE_FILES

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

::: python/mozbuild/mozbuild/backend/base.py
@@ +294,5 @@
> +        pp.context.update(self.environment.substs)
> +        pp.context.update(top_srcdir = self.environment.topsrcdir)
> +        pp.context.update(srcdir = srcdir)
> +        pp.context.update(relativesrcdir = mozpath.relpath(srcdir, self.environment.topsrcdir) or '.')
> +        pp.context.update(DEPTH = mozpath.relpath(self.environment.topobjdir, mozpath.dirname(obj.output_path)) or '.')

Nit: cuddle =

Can we just call update() with multiple arguments? That seems like it would be easier to read.

::: python/mozbuild/mozbuild/backend/common.py
@@ +106,5 @@
> +            if mozpath.basename(obj.output_path) == 'Makefile':
> +                return
> +            with self._get_preprocessor(obj) as pp:
> +                pp.do_include(obj.input_path)
> +            self.backend_input_files.add(obj.input_path)

File a bug to capture included files in backend_input_files and depend it on bug 935987.

@@ +159,5 @@
> +                                    + ' ' \
> +                                    + str(self.environment.defines[name]) \
> +                                    + l[m.end('name'):]
> +                        elif cmd == 'undef':
> +                           l = '/* ' + l[:m.end('name')] + ' */' + l[m.end('name'):]

Holy indentation, batman!

Can you do "early return" instead?

if not m:
    continue

...

if not name:
    continue

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +1158,5 @@
> +            # with a backslash.
> +            pp.handleLine(b'\n')
> +            pp.handleLine(b'include $(topsrcdir)/config/recurse.mk\n')
> +        if not stub:
> +            self.backend_input_files.add(obj.input_path)

Please copy the comment about why this is necessary.

::: python/mozbuild/mozbuild/test/backend/test_configenvironment.py
@@ -316,5 @@
> -        self.assertEqual(env.get_input('/absolute/build/dir/file'), os.path.join(absolute, 'dir', 'file.in'))
> -        self.assertEqual(env.get_top_srcdir('/absolute/build/file'), '/some/path')
> -        self.assertEqual(env.get_top_srcdir('/absolute/build/dir/file'), '/some/path')
> -        self.assertEqual(env.get_file_srcdir('/absolute/build/file'), '/some/path')
> -        self.assertEqual(env.get_file_srcdir('/absolute/build/dir/file'), '/some/path/dir')

The number of deleted tests in this file is somewhat unnerving. Feels like a regression in test coverage.

Comment 9

[Mike Hommey [:glandium]]

Attachment: Use mozpack.path instead of os.path in mozbuild.frontend and mozbuild.backend modules

I think this one is good.

Comment 10

[Mike Hommey [:glandium]]

Attachment: Refactor handling of CONFIGURE_SUBST_FILES and CONFIGURE_DEFINE_FILES.

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

Comment 11

[Mike Hommey [:glandium]]

Discussed over the air, but for posterity:

(In reply to Gregory Szorc [:gps] (in southeast Asia until Dec 14) from comment #8)

> ::: python/mozbuild/mozbuild/backend/common.py
> @@ +106,5 @@
> > +            if mozpath.basename(obj.output_path) == 'Makefile':
> > +                return
> > +            with self._get_preprocessor(obj) as pp:
> > +                pp.do_include(obj.input_path)
> > +            self.backend_input_files.add(obj.input_path)
> 
> File a bug to capture included files in backend_input_files and depend it on
> bug 935987.

Those preprocessed file only receive substitutions, not #includes or whatever else involving a marker. So there aren't any included files to capture :)
 
> @@ +159,5 @@
> > +                                    + ' ' \
> > +                                    + str(self.environment.defines[name]) \
> > +                                    + l[m.end('name'):]
> > +                        elif cmd == 'undef':
> > +                           l = '/* ' + l[:m.end('name')] + ' */' + l[m.end('name'):]
> 
> Holy indentation, batman!
> 
> Can you do "early return" instead?
> 
> if not m:
>     continue
> 
> ...
> 
> if not name:
>     continue

Sadly, there's a fh.write(l) at the end of the loop that needs to happen for each line.
 
> ::: python/mozbuild/mozbuild/test/backend/test_configenvironment.py
> @@ -316,5 @@
> > -        self.assertEqual(env.get_input('/absolute/build/dir/file'), os.path.join(absolute, 'dir', 'file.in'))
> > -        self.assertEqual(env.get_top_srcdir('/absolute/build/file'), '/some/path')
> > -        self.assertEqual(env.get_top_srcdir('/absolute/build/dir/file'), '/some/path')
> > -        self.assertEqual(env.get_file_srcdir('/absolute/build/file'), '/some/path')
> > -        self.assertEqual(env.get_file_srcdir('/absolute/build/dir/file'), '/some/path/dir')
> 
> The number of deleted tests in this file is somewhat unnerving. Feels like a
> regression in test coverage.

They are tests that are now irrelevant, so it's fine.

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8345672 [details] [diff] [review]
Refactor handling of CONFIGURE_SUBST_FILES and CONFIGURE_DEFINE_FILES.

Carrying over r+.

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8345670 [details] [diff] [review]
Use mozpack.path instead of os.path in mozbuild.frontend and mozbuild.backend modules

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

You just bit rotted like 10 patches :(

::: python/mozbuild/mozbuild/test/frontend/test_reader.py
@@ +115,5 @@
>          with self.assertRaises(BuildReaderError) as bre:
>              list(reader.read_topsrcdir())
>  
>          e = bre.exception
> +        print e.actual_file

!

Comment 14

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d148626b568b
https://hg.mozilla.org/integration/mozilla-inbound/rev/504eb03d80bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/64fdc0eed099

Comment 15

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #14)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/d148626b568b

This is https://bugzilla.mozilla.org/attachment.cgi?id=828531

Comment 16

[Mike Hommey [:glandium]]

And a fixup to avoid clobber on windows

https://hg.mozilla.org/integration/mozilla-inbound/rev/3381980228ed

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d148626b568b
https://hg.mozilla.org/mozilla-central/rev/504eb03d80bc
https://hg.mozilla.org/mozilla-central/rev/64fdc0eed099
https://hg.mozilla.org/mozilla-central/rev/3381980228ed

Comment 18

[Joshua Cranmer [:jcranmer]]

(In reply to Gregory Szorc [:gps] (in southeast Asia until Dec 14) from comment #8)
> ::: python/mozbuild/mozbuild/test/backend/test_configenvironment.py
> @@ -316,5 @@
> > -        self.assertEqual(env.get_input('/absolute/build/dir/file'), os.path.join(absolute, 'dir', 'file.in'))
> > -        self.assertEqual(env.get_top_srcdir('/absolute/build/file'), '/some/path')
> > -        self.assertEqual(env.get_top_srcdir('/absolute/build/dir/file'), '/some/path')
> > -        self.assertEqual(env.get_file_srcdir('/absolute/build/file'), '/some/path')
> > -        self.assertEqual(env.get_file_srcdir('/absolute/build/dir/file'), '/some/path/dir')
> 
> The number of deleted tests in this file is somewhat unnerving. Feels like a
> regression in test coverage.

Perhaps too much of a regression: these patches broke the comm-central build system horribly, by assuming that all of our moz.build files have a single topsrcdir and topobjdir.