The march continues...

Comment on attachment 8928718 [details] Bug 1417264 - Refactor code for writing mozconfig make file; https://reviewboard.mozilla.org/r/199986/#review205468

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 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 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 on attachment 8928722 [details] Bug 1417264 - Write objdir .mozconfig from Python; https://reviewboard.mozilla.org/r/199994/#review205478 Fine by me.

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 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 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.

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

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