Move more logic from client.mk to Python

RESOLVED FIXED in Firefox 59

Status

enhancement
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: gps, Assigned: gps)

Tracking

(Blocks 3 bugs)

unspecified
mozilla59
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(6 attachments)

Assignee

Description

2 years ago
The march continues...
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 7

2 years ago
mozreview-review
Comment on attachment 8928718 [details]
Bug 1417264 - Refactor code for writing mozconfig make file;

https://reviewboard.mozilla.org/r/199986/#review205468
Attachment #8928718 - Flags: review+

Comment 8

2 years ago
mozreview-review
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.
Attachment #8928719 - Flags: review+

Comment 9

2 years ago
mozreview-review
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.
Attachment #8928720 - Flags: review+

Comment 10

2 years ago
mozreview-review
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.
Attachment #8928721 - Flags: review+

Comment 11

2 years ago
mozreview-review
Comment on attachment 8928722 [details]
Bug 1417264 - Write objdir .mozconfig from Python;

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

Fine by me.
Attachment #8928722 - Flags: review+

Comment 12

2 years ago
mozreview-review
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?
Attachment #8928723 - Flags: review+
Attachment #8928718 - Flags: review?(core-build-config-reviews)
Attachment #8928719 - Flags: review?(core-build-config-reviews)
Attachment #8928720 - Flags: review?(core-build-config-reviews)
Attachment #8928721 - Flags: review?(core-build-config-reviews)
Attachment #8928722 - Flags: review?(core-build-config-reviews)
Attachment #8928723 - Flags: review?(core-build-config-reviews)
Assignee

Comment 13

2 years ago
mozreview-review-reply
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.
Assignee

Updated

2 years ago
Blocks: 1418122
Assignee

Comment 14

2 years ago
mozreview-review-reply
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.
Assignee

Comment 15

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(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.
Assignee

Comment 23

2 years ago
mozreview-review-reply
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.
Assignee

Updated

2 years ago
Blocks: 1418129

Comment 24

2 years ago
Pushed by gszorc@mozilla.com:
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

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.