Closed Bug 1417264 Opened 3 years ago Closed 3 years ago

Move more logic from client.mk to Python

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox59 fixed)

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: gps, Assigned: gps)

References

(Blocks 3 open bugs)

Details

Attachments

(6 files)

The march continues...
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 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 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 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 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 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)
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.
Blocks: 1418122
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 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.)
(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 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.
Blocks: 1418129
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
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.