1417264 - Move more logic from client.mk to Python

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Gregory Szorc [:gps]]

The march continues...

Comment 1

[Gregory Szorc [:gps]]

Attachment: Bug 1417264 - Refactor code for writing mozconfig make file;

Upcoming commits need to move more logic from client.mk. It will
be easier if we have a list of lines in the mozconfig as a local
variable.

Review commit: https://reviewboard.mozilla.org/r/199986/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199986/

Comment 2

[Gregory Szorc [:gps]]

Attachment: Bug 1417264 - Move printing of mozconfig lines to Python;

We write the file that client.mk is printing from Python. We can
also log it from there pretty easily. So do that.

Review commit: https://reviewboard.mozilla.org/r/199988/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199988/

Comment 3

[Gregory Szorc [:gps]]

Attachment: Bug 1417264 - Write .mozconfig.mk file from Python;

The file is a filtered version of the make file that we previously
started generating for client.mk. Why there is special casing for
UPLOAD_EXTRA_FILES, I'm not sure. This smells fishy and is something
I'd like to take a look at once all code is ported out of client.mk.

The removal of the logic from client.mk meant that we could remove
a bunch of code from client.mk related to loading mozconfig files.
We can now simply include the auto-generated make file directly and
be done with it.

Review commit: https://reviewboard.mozilla.org/r/199990/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199990/

Comment 4

[Gregory Szorc [:gps]]

Attachment: Bug 1417264 - Remove target for autoconf.mk;

This target isn't referenced elsewhere in client.mk. It appears to be
dead.

When this target became orphaned, I don't know. I suspect at some point
some included .mk file attempted to "include autoconf.mk." But I'm not
sure what file that would have been or when it would have changed.

FWIW, autoconf.mk is generated by config.status, courtesy of it being
listed in CONFIGURE_SUBST_FILES.

Review commit: https://reviewboard.mozilla.org/r/199992/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199992/

Comment 5

[Gregory Szorc [:gps]]

Attachment: Bug 1417264 - Write objdir .mozconfig from Python;

This is a pretty straightforward port of the logic. But we
even go a step further: we delete the file in the objdir if there is
no source mozconfig!

Review commit: https://reviewboard.mozilla.org/r/199994/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199994/

Comment 6

[Gregory Szorc [:gps]]

Attachment: Bug 1417264 - Write .mozconfig.json from Python;

In order to determine if we need to re-run configure, we write
a JSON file representing the evaluated mozconfig. If this JSON
file changes, configure (and config.status for that matter) is
out of data and it is re-executed.

This commit moves the generation of that JSON file to Python.

Review commit: https://reviewboard.mozilla.org/r/199996/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199996/

Comment 7

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8928718 [details]
Bug 1417264 - Refactor code for writing mozconfig make file;

https://reviewboard.mozilla.org/r/199986/#review205468

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8928719 [details]
Bug 1417264 - Move printing of mozconfig lines to Python;

https://reviewboard.mozilla.org/r/199988/#review205470

::: python/mozbuild/mozbuild/controller/building.py:1361
(Diff revision 1)
>          mozconfig_client_mk = os.path.join(self.topobjdir,
>                                             '.mozconfig-client-mk')
>          with FileAvoidWrite(mozconfig_client_mk) as fh:
>              fh.write(b'\n'.join(mozconfig_make_lines))
>  
> +        if mozconfig_make_lines:

Consider including whether this was written by the `FileAvoidWrite` or whether it was already fresh.

::: python/mozbuild/mozbuild/controller/building.py:1365
(Diff revision 1)
>  
> +        if mozconfig_make_lines:
> +            self.log(logging.WARNING, 'mozconfig_content', {
> +                'path': mozconfig['path'],
> +                'content': '\n    '.join(mozconfig_make_lines),
> +            }, 'Adding make options from {path}\n    {content}')

I feel like there may be a nicer way to block indent than this, but whatever.

Comment 9

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8928720 [details]
Bug 1417264 - Write .mozconfig.mk file from Python;

https://reviewboard.mozilla.org/r/199990/#review205472

Re: `UPLOAD_EXTRA_FILES`, I think I was digging through mozharness code that parsed the list of UPLOAD_EXTRA_FILES not so long ago, but now I can't find it.  I wonder if that's the original use case.

::: python/mozbuild/mozbuild/controller/building.py:1360
(Diff revision 1)
>  
> +        # The .mozconfig.mk file only contains exported variables and lines with
> +        # UPLOAD_EXTRA_FILES.
> +        mozconfig_filtered_lines = [
> +            line for line in mozconfig_make_lines
> +            # TODO investigate why UPLOAD_EXTRA_FILES is special and remove it.

Link to ticket number, please.

::: python/mozbuild/mozbuild/controller/building.py:1365
(Diff revision 1)
> +            # TODO investigate why UPLOAD_EXTRA_FILES is special and remove it.
> +            if line.startswith(b'export ') or b'UPLOAD_EXTRA_FILES' in line
> +        ]
> +
>          mozconfig_client_mk = os.path.join(self.topobjdir,
>                                             '.mozconfig-client-mk')

Missed it before, but why is this `-mk` and not `.mk`?  Consider fixing what looks like a typo.

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8928721 [details]
Bug 1417264 - Remove target for autoconf.mk;

https://reviewboard.mozilla.org/r/199992/#review205476

::: commit-message-48b74:10
(Diff revision 1)
> +
> +When this target became orphaned, I don't know. I suspect at some point
> +some included .mk file attempted to "include autoconf.mk." But I'm not
> +sure what file that would have been or when it would have changed.
> +
> +FWIW, autoconf.mk is generated by config.status, courtesy of it being

Thanks for this comment, 'cuz this is not easy to follow and I was about to r- this patch.

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8928722 [details]
Bug 1417264 - Write objdir .mozconfig from Python;

https://reviewboard.mozilla.org/r/199994/#review205478

Fine by me.

Comment 12

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8928723 [details]
Bug 1417264 - Write .mozconfig.json from Python;

https://reviewboard.mozilla.org/r/199996/#review205480

::: python/mozbuild/mozbuild/controller/building.py:1376
(Diff revision 1)
>          with FileAvoidWrite(mozconfig_mk) as fh:
>              fh.write(b'\n'.join(mozconfig_filtered_lines))
>  
> +        mozconfig_json = os.path.join(self.topobjdir, '.mozconfig.json')
> +        with FileAvoidWrite(mozconfig_json) as fh:
> +            json.dump({

There's a potential for this to diverge from |mach environment --json| at https://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/mach_commands.py#1097.  Can we prevent this?

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8928719 [details]
Bug 1417264 - Move printing of mozconfig lines to Python;

https://reviewboard.mozilla.org/r/199988/#review205470

> Consider including whether this was written by the `FileAvoidWrite` or whether it was already fresh.

Follow-up.

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 8928720 [details]
Bug 1417264 - Write .mozconfig.mk file from Python;

https://reviewboard.mozilla.org/r/199990/#review205472

> Missed it before, but why is this `-mk` and not `.mk`?  Consider fixing what looks like a typo.

It wasn't a typo. The file will go away when client.mk is removed. I'm inclined to let it live out the rest of its short, miserable life.

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 8928723 [details]
Bug 1417264 - Write .mozconfig.json from Python;

https://reviewboard.mozilla.org/r/199996/#review205480

> There's a potential for this to diverge from |mach environment --json| at https://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/mach_commands.py#1097.  Can we prevent this?

Eh? Which part diverges? We never actually call `mach environment --json` after this patch. I left the code in because it seems like it could be useful. As long as the output is deterministic and captures everything of importance in the state of the mozconfig, that's all we care about.

(mozconfig.json is used as a dependency to force configure to run again.)

Comment 16

[Gregory Szorc [:gps]]

Comment on attachment 8928718 [details]
Bug 1417264 - Refactor code for writing mozconfig make file;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199986/diff/1-2/

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 8928719 [details]
Bug 1417264 - Move printing of mozconfig lines to Python;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199988/diff/1-2/

Comment 18

[Gregory Szorc [:gps]]

Comment on attachment 8928720 [details]
Bug 1417264 - Write .mozconfig.mk file from Python;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199990/diff/1-2/

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 8928721 [details]
Bug 1417264 - Remove target for autoconf.mk;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199992/diff/1-2/

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 8928722 [details]
Bug 1417264 - Write objdir .mozconfig from Python;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199994/diff/1-2/

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 8928723 [details]
Bug 1417264 - Write .mozconfig.json from Python;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199996/diff/1-2/

Comment 22

[Nick Alexander :nalexander [he/him]]

(In reply to Gregory Szorc [:gps] from comment #15)
> Comment on attachment 8928723 [details]
> Bug 1417264 - Write .mozconfig.json from Python;
> 
> https://reviewboard.mozilla.org/r/199996/#review205480
> 
> > There's a potential for this to diverge from |mach environment --json| at https://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/mach_commands.py#1097.  Can we prevent this?
> 
> Eh? Which part diverges?

_Potential_ to diverge.  Folks will add to `mach environment --json` and might expect it to be involved here.  At the very least, it's repetition.

 We never actually call `mach environment --json`
> after this patch.

We do: https://searchfox.org/mozilla-central/source/settings.gradle#5, and if we were to change the output format significantly the Gradle integration would suffer.

 I left the code in because it seems like it could be
> useful. As long as the output is deterministic and captures everything of
> importance in the state of the mozconfig, that's all we care about.
> 
> (mozconfig.json is used as a dependency to force configure to run again.)

Fair enough.

Comment 23

[Gregory Szorc [:gps]]

Comment on attachment 8928723 [details]
Bug 1417264 - Write .mozconfig.json from Python;

https://reviewboard.mozilla.org/r/199996/#review205480

> Eh? Which part diverges? We never actually call `mach environment --json` after this patch. I left the code in because it seems like it could be useful. As long as the output is deterministic and captures everything of importance in the state of the mozconfig, that's all we care about.
> 
> (mozconfig.json is used as a dependency to force configure to run again.)

Doh. I missed the Gradle code. I'll file a follow-up to converge the code.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ca22a0120c4
Refactor code for writing mozconfig make file; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/7e4030a95e89
Move printing of mozconfig lines to Python; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/308804a30cd3
Write .mozconfig.mk file from Python; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/523f39e23b7f
Remove target for autoconf.mk; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/41492b2ba4e1
Write objdir .mozconfig from Python; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/249a8177ad91
Write .mozconfig.json from Python; r=nalexander

Comment 25

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/5ca22a0120c4
https://hg.mozilla.org/mozilla-central/rev/7e4030a95e89
https://hg.mozilla.org/mozilla-central/rev/308804a30cd3
https://hg.mozilla.org/mozilla-central/rev/523f39e23b7f
https://hg.mozilla.org/mozilla-central/rev/41492b2ba4e1
https://hg.mozilla.org/mozilla-central/rev/249a8177ad91