Closed Bug 948275 Opened 10 years ago Closed 10 years ago

Refactor handling of CONFIGURE_SUBST_FILES and CONFIGURE_DEFINE_FILES

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla29

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
Forgot to add a comment I wanted to add about the CommonBackend stuff.
Attachment #8345123 - Flags: review?(gps)
Attachment #8345120 - Attachment is obsolete: true
Attachment #8345120 - Flags: review?(gps)
Rebased on top of the mozpack.path change.
Attachment #8345640 - Flags: review?(gps)
Attachment #8345123 - Attachment is obsolete: true
Attachment #8345123 - Flags: review?(gps)
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 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.
Attachment #8345640 - Flags: review?(gps) → review+
Attachment #8345639 - Attachment is obsolete: true
Attachment #8345639 - Flags: review?(gps)
Attachment #8345640 - Attachment is obsolete: true
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 on attachment 8345672 [details] [diff] [review]
Refactor handling of CONFIGURE_SUBST_FILES and CONFIGURE_DEFINE_FILES.

Carrying over r+.
Attachment #8345672 - Flags: review+
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

!
Attachment #8345670 - Flags: review?(gps) → review+
And a fixup to avoid clobber on windows

https://hg.mozilla.org/integration/mozilla-inbound/rev/3381980228ed
(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.
Depends on: 949265
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.