Closed
Bug 762358
Opened 12 years ago
Closed 10 years ago
.mozconfig changes don't cause configure to run
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox-esr31 fixed, b2g-v2.0 fixed, b2g-v2.0M fixed)
RESOLVED
FIXED
mozilla33
People
(Reporter: jesup, Assigned: glandium)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 6 obsolete files)
4.39 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
61.22 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
I assume this is a regression - if you change/touch .mozconfig, configure doesn't get run. Very confusing why turning off a build option didn't turn it off
Comment 1•12 years ago
|
||
This was done intentionally by bug 715048. See the similar (but for comm-central) bug 731985 comment 12 for Ted's comments on this.
Blocks: 715048
![]() |
||
Comment 2•12 years ago
|
||
It's a feature, not a bug :) See bug 731985 comment 10.
Assignee | ||
Comment 3•12 years ago
|
||
Note, we could actually entirely get rid of the problem by *not* creating a .mozconfig.mk, and "parse" .mozconfig instead. We could then re-establish the dependency that makes configure be run when .mozconfig changes.
Assignee | ||
Comment 4•12 years ago
|
||
I was thinking about something along these lines in comment 3. I don't know how much impact this has on windows, but compared to the whole build, it's probably negligible. <none>
Attachment #630908 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 5•12 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #4) > Created attachment 630908 [details] [diff] [review] > Don't create a .mozconfig.mk file, and re-run configure when .mozconfig > changed > > I was thinking about something along these lines in comment 3. I don't know > how much impact this has on windows, but compared to the whole build, it's > probably negligible. Actually, it doesn't have any impact since build/autoconf/mozconfig2client-mk was already run every time anyway.
Comment 6•12 years ago
|
||
Comment on attachment 630908 [details] [diff] [review] Don't create a .mozconfig.mk file, and re-run configure when .mozconfig changed Should you remove the hgignore and gitignore entries as well? I'm not sure how this would impact people using multiple mozconfig files, however (I can't remember how all that works).
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #6) > Should you remove the hgignore and gitignore entries as well? It would be tempting to do that, but that would expose the file previously created, with the risk of people adding it somehow in the tree.
Comment 8•12 years ago
|
||
Comment on attachment 630908 [details] [diff] [review] Don't create a .mozconfig.mk file, and re-run configure when .mozconfig changed Review of attachment 630908 [details] [diff] [review]: ----------------------------------------------------------------- ::: client.mk @@ +99,5 @@ > > + > +endef > + > +$(eval $(subst ||,$(CR),$(shell $(TOPSRCDIR)/$(MOZCONFIG_LOADER) $(TOPSRCDIR) 2> $(TOPSRCDIR)/.mozconfig.out | sed 's/$$/||/'))) As I mentioned on IRC, a comment here explaining the subst/sed stuff would be useful. This line is a bit hard to read without knowing what it does.
Attachment #630908 -
Flags: review?(ted.mielczarek) → review+
Updated•12 years ago
|
Assignee: nobody → mh+mozilla
Assignee | ||
Comment 9•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa9532ebf77d
Target Milestone: --- → mozilla16
Comment 10•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/aa9532ebf77d
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 11•11 years ago
|
||
So, with a recent (revision 114573) checkout of m-c, changing .mozconfig is still not causing configure to run.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 12•11 years ago
|
||
I can confirm this, even without using mach :) 1) make -f client.mk 2) Wait to get past configure stage then CTRL+c 3) touch .mozconfig 4) make -f client.mk 5) configure not executed
Comment 13•11 years ago
|
||
This was intentionally done in bug 715048 aiui, see bug 715048 comment 2.
Comment 14•11 years ago
|
||
(In reply to Ed Morley [UTC+0; email:edmorley@moco] from comment #13) > This was intentionally done in bug 715048 aiui, see bug 715048 comment 2. Oh sorry read patch landing dates backwards, that bug preceeded this.
Comment 15•11 years ago
|
||
Even better, changing the CC=... path in my .mozconfig and touching configure.in _did_ cause it to re-run configure... with the old CC value (which didn't work, as that binary no longer existed).
Assignee | ||
Comment 16•11 years ago
|
||
(In reply to Timothy B. Terriberry (:derf) from comment #15) > Even better, changing the CC=... path in my .mozconfig and touching > configure.in _did_ cause it to re-run configure... with the old CC value > (which didn't work, as that binary no longer existed). Yeah, that's because of config.cache. :-/
Assignee | ||
Comment 17•11 years ago
|
||
(In reply to Timothy B. Terriberry (:derf) from comment #15) > Even better, changing the CC=... path in my .mozconfig and touching > configure.in _did_ cause it to re-run configure... with the old CC value > (which didn't work, as that binary no longer existed). Note that if you're building on mac, this could be because of bug 818092
Comment 18•11 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #17) > Note that if you're building on mac, this could be because of bug 818092 This was on Linux.
Updated•11 years ago
|
Target Milestone: mozilla16 → ---
Assignee | ||
Comment 22•10 years ago
|
||
This adds a format option to mach environment and uses it in client.mk to create a .mozconfig.json in the objdir, containing all the relevant data from mozconfig. If the mozconfig doesn't change in a way that alters that data, we still skip configure. At the same time, use mach environment in place of mozconfig2configure and mozconfig2client-mk, which makes us now have only one mozconfig reader. Finally, in the mozconfig reader, keep track of environment variables (as opposed to shell variables), so that changes such as a variable that was exported that is not exported anymore is spotted. At the opposite, in order for irrelevant environment variable changes not to incur in re-running configure, only a set of environment variables are stored when they are unmodified. Otherwise, changes such as using a different terminal window, or even rebooting, would trigger reconfigures. This doesn't address the config.cache problem, which is a separate problem. As a side effect, this kind of implements bug 998104, at least partially. Only tested locally on Linux.
Attachment #8446456 -
Flags: review?(gps)
Assignee | ||
Comment 23•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=e04264b0dec8
Assignee | ||
Comment 24•10 years ago
|
||
Comment on attachment 8446456 [details] [diff] [review] Re-run configure when mozconfig changed in a significant way Review of attachment 8446456 [details] [diff] [review]: ----------------------------------------------------------------- ::: client.mk @@ +350,5 @@ > $(NULL) > > +CREATE_MOZCONFIG_JSON = $(shell $(MAKE) --no-print-directory -f $(TOPSRCDIR)/client.mk $(OBJDIR)/.mozconfig.json CREATE_MOZCONFIG_JSON=1) > +$(OBJDIR)/.mozconfig.json: $(call mkdir_deps,$(OBJDIR)) $(if $(CREATE_MOZCONFIG_JSON),FORCE) > + $(PYTHON) $(TOPSRCDIR)/mach environment --format=json -o $@ Missing a @ here. Fixed locally while editing the bzexport message, I thought it would pick the updated patch, I was wrong.
Assignee | ||
Comment 25•10 years ago
|
||
sigh... this is failing on try because of this: mach and the build system store shared state in a common directory on the filesystem. The following directory will be created: /Users/cltbld/.mozbuild If you would like to use a different directory, hit CTRL+c and set the MOZBUILD_STATE_PATH environment variable to the directory you would like to use and re-run mach. For this change to take effect forever, you'll likely want to export this environment variable from your shell's init scripts. 20 19 18 17 16 15 14 13 12 11 10 9 8 7 6 5 4 3 2 1 0 Creating default state directory: /Users/cltbld/.mozbuild Please re-run mach.
Assignee | ||
Comment 26•10 years ago
|
||
New attempt with bug 1030717 applied: https://tbpl.mozilla.org/?tree=Try&rev=c7a212094887
Assignee | ||
Comment 27•10 years ago
|
||
Comment on attachment 8446456 [details] [diff] [review] Re-run configure when mozconfig changed in a significant way There are still rough edges on mac.
Attachment #8446456 -
Flags: review?(gps)
Assignee | ||
Comment 28•10 years ago
|
||
... and windows, where there's something fishy with .mozconfig.mk, and linux, where it fails because of: Python 2.7 or above (but not Python 3) is required to run mach. You are running Python 2.6.6
Assignee | ||
Comment 29•10 years ago
|
||
And I'm going to have to fix bug 832112 at the same time.
Blocks: 832112
Assignee | ||
Comment 30•10 years ago
|
||
This adds a format option to mach environment and uses it in client.mk to create a .mozconfig.json in the objdir, containing all the relevant data from mozconfig. If the mozconfig doesn't change in a way that alters that data, we still skip configure. At the same time, use mach environment in place of mozconfig2configure and mozconfig2client-mk, which makes us now have only one mozconfig reader. Also, in the mozconfig reader, keep track of environment variables (as opposed to shell variables), so that changes such as a variable that was exported not being exported anymore is spotted. At the opposite, in order for irrelevant environment variable changes not to incur in re-running configure, only a set of environment variables are stored when they are unmodified. Otherwise, changes such as using a different terminal window, or even rebooting, would trigger reconfigures. Finally, make mach environment emit both MOZ_OBJDIR and OBJDIR for client.mk, and cleanup some objdir-related things in client.mk.. At the same time, make the mozconfig reader take MOZ_OBJDIR from the environment if it is defined there and not in the mozconfig. https://tbpl.mozilla.org/?tree=Try&rev=907faff24da7 Note the OSX orange. I don't expect this to take a big interdiff, so I'd rather have a review of this big chunk early, and will send fixups later.
Attachment #8447094 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8446456 -
Attachment is obsolete: true
Assignee | ||
Comment 31•10 years ago
|
||
Turns out it didn't take long to reproduce and find a fix (only a test change): https://tbpl.mozilla.org/?tree=Try&rev=3f9fb448aeab
Attachment #8447115 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8447094 -
Attachment is obsolete: true
Attachment #8447094 -
Flags: review?(gps)
Assignee | ||
Comment 32•10 years ago
|
||
This one is good (again, a test-only change): https://tbpl.mozilla.org/?tree=Try&rev=cc935da8325e There's a remaining issue building comm-central, though, but don't let that stop you from reviewing :)
Attachment #8447170 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8447115 -
Attachment is obsolete: true
Attachment #8447115 -
Flags: review?(gps)
Assignee | ||
Comment 33•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=3fb4348fa9c4 This is the one. And it doesn't break c-c more than it currently is as far as my testing goes.
Attachment #8447573 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8447170 -
Attachment is obsolete: true
Attachment #8447170 -
Flags: review?(gps)
Comment 34•10 years ago
|
||
Comment on attachment 8447573 [details] [diff] [review] Re-run configure when mozconfig changed in a significant way Review of attachment 8447573 [details] [diff] [review]: ----------------------------------------------------------------- This is essentially r+. Just want to see some minor feedback addressed. ::: python/mozbuild/mozbuild/base.py @@ +187,5 @@ > + _config_topobjdir = config_topobjdir > + if current_project: > + _config_topobjdir = os.path.join(config_topobjdir, current_project) > + mozilla_dir = os.path.join(_config_topobjdir, 'mozilla') > + if not samepath(topobjdir, _config_topobjdir) \ Given how many bugs we've had with this code in the past, I'd *really* like to see test coverage of this. ::: python/mozbuild/mozbuild/mach_commands.py @@ +917,5 @@ > + > + if output: > + from mozbuild.util import FileAvoidWrite > + with FileAvoidWrite(output) as out: > + return func(out, verbose) I'm guessing we can't use shell redirection for file writing because we want to preserve mtimes if output hasn't changed? @@ +1008,5 @@ > + for key in self.mozconfig['env']['removed'].keys() + \ > + self.mozconfig['vars']['removed'].keys(): > + print("unset %s" % key, file=out) > + > + def _environment_json(self, out, verbose): I can't remember who, but someone was recently asking about machine readable output for this. I'm glad we have it! ::: python/mozbuild/mozbuild/mozconfig.py @@ +259,5 @@ > + if vars_before[key] != vars_after[key]: > + changed['modified'][key] = ( > + vars_before[key], vars_after[key]) > + elif key in self.ENVIRONMENT_VARIABLES: > + changed['unmodified'][key] = vars_after[key] It's not entirely obvious why the ENVIRONMENT_VARIABLES whitelist was added. Please add a comment here. ::: python/mozbuild/mozbuild/test/test_base.py @@ +36,5 @@ > def setUp(self): > self._old_cwd = os.getcwd() > self._old_env = dict(os.environ) > os.environ.pop('MOZCONFIG', None) > + os.environ.pop('MOZ_OBJDIR', None) There are a bunch of unittest.skip properties in this file. Could you try enabling these tests after making this change? See also bug 853954. http://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/test/test_base.py#74
Attachment #8447573 -
Flags: review?(gps) → feedback+
Assignee | ||
Comment 35•10 years ago
|
||
> ::: python/mozbuild/mozbuild/mach_commands.py
> @@ +917,5 @@
> > +
> > + if output:
> > + from mozbuild.util import FileAvoidWrite
> > + with FileAvoidWrite(output) as out:
> > + return func(out, verbose)
>
> I'm guessing we can't use shell redirection for file writing because we want
> to preserve mtimes if output hasn't changed?
Yes. Will add a comment.
Assignee | ||
Comment 36•10 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #34) > I can't remember who, but someone was recently asking about machine readable > output for this. I'm glad we have it! I guess you are thinking of bug 998104.
Assignee | ||
Comment 37•10 years ago
|
||
This adds a format option to mach environment and uses it in client.mk to create a .mozconfig.json in the objdir, containing all the relevant data from mozconfig. If the mozconfig doesn't change in a way that alters that data, we still skip configure. At the same time, use mach environment in place of mozconfig2configure and mozconfig2client-mk, which makes us now have only one mozconfig reader. Also, in the mozconfig reader, keep track of environment variables (as opposed to shell variables), so that changes such as a variable that was exported not being exported anymore is spotted. At the opposite, in order for irrelevant environment variable changes not to incur in re-running configure, only a set of environment variables are stored when they are unmodified. Otherwise, changes such as using a different terminal window, or even rebooting, would trigger reconfigures. Finally, make mach environment emit both MOZ_OBJDIR and OBJDIR for client.mk, and cleanup some objdir-related things in client.mk.. At the same time, make the mozconfig reader take MOZ_OBJDIR from the environment if it is defined there and not in the mozconfig.
Attachment #8449068 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8447573 -
Attachment is obsolete: true
Assignee | ||
Comment 38•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=7dfd58d8ef8a
Comment 39•10 years ago
|
||
Comment on attachment 8449068 [details] [diff] [review] Re-run configure when mozconfig changed in a significant way Review of attachment 8449068 [details] [diff] [review]: ----------------------------------------------------------------- Intradiff looks sane.
Attachment #8449068 -
Flags: review?(gps) → review+
Assignee | ||
Comment 40•10 years ago
|
||
Turns out I had to revert a part of the last interdiff because of universal builds on c-c, and modify that code slightly to accomodate. After much fight with the tests you made me enable, I think this is it. https://tbpl.mozilla.org/?tree=Try&rev=b5b3e73fd635
Attachment #8449211 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8449068 -
Attachment is obsolete: true
Assignee | ||
Comment 41•10 years ago
|
||
So, for python/mozbuild/mozbuild/base.py, this interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=8447573&action=interdiff&newid=8449211&headers=1 is more readable than the one comparing the last two patches.
Comment 42•10 years ago
|
||
Comment on attachment 8449211 [details] [diff] [review] Re-run configure when mozconfig changed in a significant way Review of attachment 8449211 [details] [diff] [review]: ----------------------------------------------------------------- Intradiff looks sane.
Attachment #8449211 -
Flags: review?(gps) → review+
Assignee | ||
Comment 43•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce1c57e03b88
Assignee | ||
Comment 44•10 years ago
|
||
Fixup for linux32 orange: https://hg.mozilla.org/integration/mozilla-inbound/rev/7f14b0641458
Assignee | ||
Comment 45•10 years ago
|
||
And another fixup for windows orange caused by previous fixup. https://hg.mozilla.org/integration/mozilla-inbound/rev/56b54954e7fb
Comment 46•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ce1c57e03b88 https://hg.mozilla.org/mozilla-central/rev/7f14b0641458 https://hg.mozilla.org/mozilla-central/rev/56b54954e7fb
Status: REOPENED → RESOLVED
Closed: 12 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment 47•10 years ago
|
||
Thanks so much for fixing this bug!
Comment 48•10 years ago
|
||
The old mozconfig2configure script used to know both the srcdir and the objdir and didn't have to try very hard to find my mozconfig. It then allowed my mozconfig to detect the objdir so the latter could then automatically tweak the configure options appropriately. However nobody bothers to tell mach environment where the comm-central srcdir is so the first time you want to build an objdir with this change in you have to either build with the srcdir as your working directory or explicitly specify the location of the mozconfig, which is at least possible on the command line (I wasn't able to find a way of overriding the location of the objdir from the command line).
Assignee | ||
Comment 49•10 years ago
|
||
comm-central's client.mk is still using mozconfig2client-mk to get the objdir.
Updated•9 years ago
|
status-firefox-esr31:
--- → fixed
Updated•9 years ago
|
status-b2g-v2.0:
--- → fixed
Comment 50•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/a17e01a85356 https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/9d49015d2ae7 https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/fa79733768c5
status-b2g-v2.0M:
--- → fixed
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•