After updating and attempting to build, I get: ... 0:18.65 creating ./config.status 0:19.33 Traceback (most recent call last): 0:19.33 File "./config.status", line 885, in <module> 0:19.33 config_status(**args) 0:19.33 File "o:\src\mozilla-git\gecko-dev\python\mozbuild\mozbuild\config_status.py", line 81, in config_status 0:19.33 write_mozinfo(os.path.join(topobjdir, 'mozinfo.json'), env, os.environ) 0:19.33 File "o:\src\mozilla-git\gecko-dev\python\mozbuild\mozbuild\mozinfo.py", line 99, in write_mozinfo 0:19.33 build_conf = build_dict(config, env) 0:19.33 File "o:\src\mozilla-git\gecko-dev\python\mozbuild\mozbuild\mozinfo.py", line 30, in build_dict 0:19.33 the_mozconfig = mozconfig.MozconfigLoader(config.topsrcdir).find_mozconfig(env) 0:19.33 File "o:\src\mozilla-git\gecko-dev\python\mozbuild\mozbuild\mozconfig.py", line 102, in find_mozconfig 0:19.33 'does not exist: ' + env_path) 0:19.33 mozbuild.mozconfig.MozconfigFindException: MOZCONFIG environment variable refers to a path that does not exist: .mozconfig-release 0:19.34 *** Fix above errors and then restart with\ 0:19.34 "C:/mozilla-build/msys/local/bin/mozmake.EXE -f client.mk build" 0:19.35 o:/src/mozilla-git/gecko-dev/client.mk:361: recipe for target 'configure' failed MOZCONFIG is specified as a filename with no path component, and the file exists in the root of the source dir. $ ./mach environment platform: Windows-7-6.1.7601-SP1 python version: 2.7.4 (default, Apr 6 2013, 19:54:46) [MSC v.1500 32 bit (Intel)] python prefix: c:\mozilla-build\python mach cwd: o:\src\mozilla-git\gecko-dev os cwd: o:\src\mozilla-git\gecko-dev mach directory: o:\src\mozilla-git\gecko-dev state directory: o:/home/.mozbuild mozconfig path: o:\src\mozilla-git\gecko-dev\.mozconfig-release object directory: o:\src\mozilla-git\gecko-dev\obj-release mozconfig configure args: --enable-application=browser --enable-tests --enable-crashreporter --enable-signmar --enable-js-diagnostics mozconfig extra make args: AUTOCLOBBER=1 mozconfig make flags: - j 1 2 config topsrcdir: o:/src/mozilla-git/gecko-dev config topobjdir: o:/src/mozilla-git/gecko-dev/obj-release Specifying .mozconfig as a fully-qualified path seems to make this work.

Seems to have broken with this merge: https://hg.mozilla.org/mozilla-central/pushloghtml?startID=26155&endID=26156 Possibly this? 0234c1bf4ea0 Nathan Froyd — Bug 936555 - make mozinfo use MozconfigLoader to locate the mozconfig; r=gps

Can you attach your $objdir/mozinfo.json? This is actually expected, I think.

(In reply to Nathan Froyd (:froydnj) from comment #2) > Can you attach your $objdir/mozinfo.json? I deleted that file, then re-ran "./mach configure" - the exception was thrown before that file was created. > This is actually expected, I think. How so? I've been using an unqualified filename for MOZCONFIG for ever (and it allows me to change directories and have the correct file picked up from that new directory). This has only stopped working in the last day or so.

(In reply to Mark Hammond [:markh] from comment #3) > > This is actually expected, I think. > > How so? I've been using an unqualified filename for MOZCONFIG for ever (and > it allows me to change directories and have the correct file picked up from > that new directory). This has only stopped working in the last day or so. Right, because before bug 936555, a relative-path MOZCONFIG was assumed to live in $srcdir. But this is not correct in general (e.g. I was caught doing |cd $objdir; MOZCONFIG=.mozconfig $srcdir/configure| and having things go horribly wrong later), so bug 936555 changed it to look up relative-path MOZCONFIG in the current directory. I can understand how this broke your use case, but what I don't understand is the "it allows me to change directories and have the correct file picked up from that new directory" bit. Can you expand on that with an example? I guess we could change the lookup algorithm to look in the cwd first, then check the srcdir. gps, does that seem reasonable?

For me this is broken even from within $srcdir. This is my usual workflow: start-msvc10.bat cd /path/to/sources export MOZCONFIG=.mozconfig_opt mach build ...and now I get this error. Was there something wrong with what I had been doing?

(In reply to Nathan Froyd (:froydnj) from comment #4) > (In reply to Mark Hammond [:markh] from comment #3) > > > This is actually expected, I think. > > > > How so? I've been using an unqualified filename for MOZCONFIG for ever (and > > it allows me to change directories and have the correct file picked up from > > that new directory). This has only stopped working in the last day or so. > > Right, because before bug 936555, a relative-path MOZCONFIG was assumed to > live in $srcdir. Yes, the MOZCONFIG is in the root of my srcdir. > But this is not correct in general (e.g. I was caught > doing |cd $objdir; MOZCONFIG=.mozconfig $srcdir/configure| and having things > go horribly wrong later), so bug 936555 changed it to look up relative-path > MOZCONFIG in the current directory. But my MOZCONFIG *is* in the cwd - I'm running mach from the root of the srcdir, which is the same dir as the MOZCONFIG. > I can understand how this broke your use case, but what I don't understand > is the "it allows me to change directories and have the correct file picked > up from that new directory" bit. Can you expand on that with an example? Sure. I have multiple clones, each with .mozconfig files with the same name in the root. So I can say "export MOZCONFIG=.mozconfig" and the appropriate one is picked up whenever I change the cwd to be the root of any of these clones. > I guess we could change the lookup algorithm to look in the cwd first, then > check the srcdir. gps, does that seem reasonable? I'm still a bit confused as my mozconfig is *both* in the cwd and the srcdir.

So, Nathan makes a change because his workflow was broken. This breaks Mark's and David's workflow. Takeaway: you can't please everybody. I'm currently proposing an overhaul to how mach and the build system detects things. See https://etherpad.mozilla.org/o5igfRCiJv. I'd like to think that we can arrive at a compromise for how this should work. How about this for an immediate proposal: 1) Look for ENV['MOZCONFIG'] in both getcwd() and topsrcdir. 2) If only 1 present, use it. 3) If both are present, abort due to ambiguity. The consensus in the etherpad so far seems to be that ambiguity is bad and we should abort immediately.

(In reply to Gregory Szorc [:gps] from comment #7) > So, Nathan makes a change because his workflow was broken. This breaks > Mark's and David's workflow. Takeaway: you can't please everybody. ... > 1) Look for ENV['MOZCONFIG'] in both getcwd() and topsrcdir. > 2) If only 1 present, use it. > 3) If both are present, abort due to ambiguity. But I don't see the ambiguity here - I'm working exclusively with my cwd the same as the topsrcdir - so my MOZCONFIG is in *both* simultaneously.

(In reply to Mark Hammond [:markh] from comment #8) > But I don't see the ambiguity here - I'm working exclusively with my cwd the > same as the topsrcdir - so my MOZCONFIG is in *both* simultaneously. Mine is too. And I suspect this is a common configuration.

This appears to only be a problem on Windows? I can build just fine with: cd $srcdir echo 'ac_add_options --enable-optimize' > .mconfig export MOZCONFIG=.mconfig mach build on Linux. But wait, doing that same sequence of steps on my Windows box gets me through 'mach configure' as well. And that seems to be essentially what dmajor was doing in comment 5. So I'm not sure what's going wrong here. Will somebody apply this patch and report what the new, improved output is?

Comment on attachment 8373382 [details] [diff] [review] more informative error message for MozconfigLoader Whoops, I had not updated my tree before testing this. Ah, I think I sort of understand what the problem is here. We have a bit of code in client.mk that's supposed to re-export the MOZCONFIG to subprocesses: http://mxr.mozilla.org/mozilla-central/source/client.mk#114 In this case specifically, we want to export it to: http://mxr.mozilla.org/mozilla-central/source/client.mk#357 Which is fine. But the environment variable has always taken precedence over anything we can find in the cwd. And before bug 936555, we always checked $(topsrcdir)/$(MOZCONFIG). Now we don't do that. So we can change MozconfigLoader or we can change client.mk to make the save-mozconfig rule really work. I don't really understand why I don't see this on Linux, though.

This patch implements the proposal from comment 7. It even adds some tests so we ideally won't bust this behavior in the future.

Comment on attachment 8373446 [details] [diff] [review] try harder to resolve relative paths in MozconfigLoader Review of attachment 8373446 [details] [diff] [review]: ----------------------------------------------------------------- Looking good! Great tests. Just want the mozconfig.py code cleaned up a little before landing. There's one too many nits for me to grant r+. ::: python/mozbuild/mozbuild/mozconfig.py @@ +96,5 @@ > > env_path = env.get('MOZCONFIG', None) > if env_path is not None: > + if not os.path.isabs(env_path): > + potential_roots = [self.topsrcdir, os.curdir] You might as well use os.getcwd() instead of os.curdir (. - which will just resolve to os.getcwd()) @@ +103,5 @@ > + potential_roots = [os.path.abspath(p) for p in potential_roots] > + potential_roots = list(set(potential_roots)) > + existence = [os.path.exists(os.path.join(root, env_path)) > + for root in potential_roots] > + how_many = existence.count(True) This code is quite difficult to read. How about: potential_roots = set(os.path.abspath(p) for p in potential_root]) existing = [root for root in potential_roots if os.path.exists(os.path.join(root, env_path))] if len(existing) > 1: ... elif not existing: ... @@ +109,5 @@ > + raise MozconfigFindException( > + 'MOZCONFIG environment variable refers to a path that ' + > + 'exists in more than one of ' + ', '.join(potential_roots) + > + '. Remove all but one.') > + if how_many == 0: if not how_many: @@ +121,3 @@ > raise MozconfigFindException( > 'MOZCONFIG environment variable refers to a path that ' > 'does not exist: ' + env_path) I don't think it's possible to reach this elif not os.path.exists(env_path) block any more since path validation is performed above.

(In reply to Gregory Szorc [:gps] from comment #14) > Looking good! Great tests. Just want the mozconfig.py code cleaned up a > little before landing. There's one too many nits for me to grant r+. Thanks for the review. I'll clean these up tomorrow. > @@ +121,3 @@ > > raise MozconfigFindException( > > 'MOZCONFIG environment variable refers to a path that ' > > 'does not exist: ' + env_path) > > I don't think it's possible to reach this elif not os.path.exists(env_path) > block any more since path validation is performed above. I thought so too! Then tests started failing. The path validation above only applies to relative paths. So it's possible to have a non-existent absolute path that hits this case.

Review comments applied. Extra comment attached to the os.path.exists() check to make it more obvious what's going on. Can take it out if you like.

So, paths are a lot of fun. This patch fixes two problems observed on try: 1. Valgrind builds seem to have a different srcdir/objdir layout that we need to handle. It is explained in the mozconfig.py comment. 2. os.path.samefile does not exist on Windows. I love cross-platform code. Ideally this is not too gross.

Comment on attachment 8375600 [details] [diff] [review] try harder to resolve relative paths in MozconfigLoader Review of attachment 8375600 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. This code is painful. Thanks for staying the course.

Landed with one small tweak to the test: we need to use os.path.realpath to see through links, rather than os.path.normpath. Perhaps we should have just checked that we got good content from the mozconfig... https://hg.mozilla.org/integration/mozilla-inbound/rev/24548c52a802