Closed Bug 762358 Opened 11 years ago Closed 9 years ago

.mozconfig changes don't cause configure to run

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox-esr31 fixed, b2g-v2.0 fixed, b2g-v2.0M fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla33
Tracking Flags:
Tracking Status
firefox-esr31 --- fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed

People

(Reporter: jesup, Assigned: glandium)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 6 obsolete files)

Don't create a .mozconfig.mk file, and re-run configure when .mozconfig changed
4.39 KB, patch
ted
: review+
Details | Diff | Splinter Review
Re-run configure when mozconfig changed in a significant way
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
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
It's a feature, not a bug :)  See bug 731985 comment 10.
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.
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)
(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 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).
(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 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+
Assignee: nobody → mh+mozilla
https://hg.mozilla.org/mozilla-central/rev/aa9532ebf77d
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
So, with a recent (revision 114573) checkout of m-c, changing .mozconfig is still not causing configure to run.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
This was intentionally done in bug 715048 aiui, see bug 715048 comment 2.
(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.
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).
(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. :-/
(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
(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.
Target Milestone: mozilla16 → ---
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)
Blocks: 998104
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.
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.
Depends on: 1030717
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)
... 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
And I'm going to have to fix bug 832112 at the same time.
Blocks: 832112
Depends on: 1031132
Depends on: 1031180
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)
Attachment #8446456 - Attachment is obsolete: true
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)
Attachment #8447094 - Attachment is obsolete: true
Attachment #8447094 - Flags: review?(gps)
Depends on: 1031291
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)
Attachment #8447115 - Attachment is obsolete: true
Attachment #8447115 - Flags: review?(gps)
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)
Attachment #8447170 - Attachment is obsolete: true
Attachment #8447170 - Flags: review?(gps)
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+
> ::: 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.
(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.
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)
Attachment #8447573 - Attachment is obsolete: true
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+
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)
Attachment #8449068 - Attachment is obsolete: true
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 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+
And another fixup for windows orange caused by previous fixup.
https://hg.mozilla.org/integration/mozilla-inbound/rev/56b54954e7fb
Depends on: 1033958
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: 11 years ago → 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Thanks so much for fixing this bug!
Depends on: 1034815
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).
comm-central's client.mk is still using mozconfig2client-mk to get the objdir.
Depends on: 837846
No longer depends on: 1034815
Blocks: Woodduck
Blocks: 1195331
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.