Open Bug 1417684 Opened 2 years ago Updated 2 years ago

Move logic for invoking configure into Python

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: gps, Assigned: gps)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 3 obsolete files)

Code for determining what `make` to use, validating it is sane, and determining additional flags to pass to make (MOZ_MAKE_FLAGS) is currently scattered around client.mk, base.py, and baseconfig.mk.

moz.configure has logic for resolving MAKE. But because moz.configure is run from client.mk and MAKE is set in the environment variable, moz.configure's logic essentially does nothing. And this fact is obscuring that the moz.configure logic is flat out wrong (it doesn't prioritize mozmake.exe on Windows for example).

Let's take a chainsaw to all this code and implement it correctly in moz.configure.
This turned into a rabbit hole. Everything is so tightly coupled that I'm going to scope bloat this to moving the logic for invoking configure out of client.mk. I apologize in advance for the patches that some unlucky soul will be asked to review.
Summary: Move make and make flags derivation to moz.configure → Move logic for invoking configure into Python
This series fails spectacularly on Try. There's obviously some work to do.
Attachment #8929297 - Attachment is obsolete: true
Attachment #8929297 - Flags: review?(core-build-config-reviews)
Attachment #8929301 - Attachment is obsolete: true
Attachment #8929301 - Flags: review?(core-build-config-reviews)
Attachment #8929302 - Attachment is obsolete: true
Attachment #8929302 - Flags: review?(core-build-config-reviews)
Comment on attachment 8929300 [details]
Bug 1417684 - Port client.mk code to run configure to Python;

Everything up to this one should be reviewable. I'm still excavating the rabbit hole for moving the configure logic. I also think I can split up this commit and make everything easier to review.
Attachment #8929300 - Flags: review?(core-build-config-reviews)
Comment on attachment 8929296 [details]
Bug 1417684 - Stop writing FOUND_MOZCONFIG variable;

https://reviewboard.mozilla.org/r/200614/#review207048
Attachment #8929296 - Flags: review+
Comment on attachment 8929295 [details]
Bug 1417684 - Functionality for evaluating file-based mtime dependencies;

https://reviewboard.mozilla.org/r/200612/#review207046

This looks sensible. Thanks for writing tests!

::: python/mozbuild/mozbuild/dependsutils.py:29
(Diff revision 2)
> +    * The mtime of all targets is newer than the newest mtime of dependencies.
> +    """
> +    if not target_files:
> +        raise ValueError('must specify targets')
> +
> +    stats = batch_stat(target_files + depends_files)

Your function documentation claims these are both iterables but you're treating them as sets here. If they're always sets then document that instead. If you intend to support arbitrary iterables, you'd presumably have to wrap them both in `set()`.
Attachment #8929295 - Flags: review+
Attachment #8929295 - Flags: review?(core-build-config-reviews)
Attachment #8929296 - Flags: review?(core-build-config-reviews)
Comment on attachment 8929298 [details]
Bug 1417684 - Always evaluate config environment;

https://reviewboard.mozilla.org/r/200618/#review207052

I'm very sure you're correct given the comment you're removing. Nice to have removed universal builds, they made so many things awkward.
Attachment #8929298 - Flags: review+
Attachment #8929298 - Flags: review?(core-build-config-reviews)
Comment on attachment 8929299 [details]
Bug 1417684 - Prepare the object directory earlier;

https://reviewboard.mozilla.org/r/200620/#review207056

::: python/mozbuild/mozbuild/controller/building.py:1318
(Diff revision 2)
>                             print_directory=False)
>          else:
>              install_test_files(mozpath.normpath(self.topsrcdir), self.topobjdir,
>                                 '_tests', test_objs)
>  
> -    def _run_client_mk(self, target=None, line_handler=None, jobs=0,
> +    def _prepare_objdir(self):

Can you document what the return value of this function means? I find it a little confusing that returning `True` is handled as the exceptional case. Also, it might be nice to make it return a value in all cases. Having a single return statement makes things a little weird.
Attachment #8929299 - Flags: review+
Attachment #8929299 - Flags: review?(core-build-config-reviews)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.