1234439 - ./mach build faster broken on Windows

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[Mark Hammond [:markh] [:mhammond]]

Since I updated yesterday I see:

> $ ./mach build faster
> 0:01.56 c:\mozilla-build\mozmake\mozmake.EXE -C o:/src/mozilla-git/gecko-dev/obj-release -j12 -s backend.RecursiveMakeBackend
> 0:01.89 c:\mozilla-build\mozmake\mozmake.EXE -C faster -j12 -s
> 0:06.17 C:/Users/skip/AppData/Local/Temp/make8268-8.sh: line 3: unexpected EOF while looking for matching `"'
> 0:06.18 C:/Users/skip/AppData/Local/Temp/make8268-8.sh: line 4: syntax error: unexpected end of file
> 0:06.18 o:/src/mozilla-git/gecko-dev/config/faster/rules.mk:126: recipe for target 'o:/src/mozilla-git/gecko-dev/obj-release/dist/bin/browser/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/chrome.manifest' failed
> 0:06.18 mozmake.EXE: *** [o:/src/mozilla-git/gecko-dev/obj-release/dist/bin/browser/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/chrome.manifest] Error 258
> 0:06.18 mozmake.EXE: *** Waiting for unfinished jobs....

Specifying a directory to build works OK. The make*.sh file is removed after the above is printed so I can't tell you what that file contains.

Comment 1

[Mike Hommey [:glandium]]

Try pasting the output from `mach build faster -v`

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: "./mach build faster -v" output

Nothing obvious I can see, but I'm not sure what I'm looking for :)

Comment 3

[Mike Hommey [:glandium]]

Please try this:

diff --git a/python/mozbuild/mozbuild/backend/fastermake.py b/python/mozbuild/mozbuild/backend/fastermake.py
index 78c80f4..342fc26 100644
--- a/python/mozbuild/mozbuild/backend/fastermake.py
+++ b/python/mozbuild/mozbuild/backend/fastermake.py
@@ -239,17 +239,17 @@ class FasterMakeBackend(CommonBackend):
 
         # Add information for chrome manifest generation
         manifest_targets = []
 
         for target, entries in self._manifest_entries.iteritems():
             manifest_targets.append(target)
             target = '$(TOPOBJDIR)/%s' % target
             mk.create_rule([target]).add_dependencies(
-                ['content = %s' % ' '.join('"%s"' % e for e in entries)])
+                ['content = %s' % ' '.join("'%s'" % e for e in entries)])
 
         mk.add_statement('MANIFEST_TARGETS = %s' % ' '.join(manifest_targets))
 
         # Add information for install manifests.
         mk.add_statement('INSTALL_MANIFESTS = %s'
                          % ' '.join(self._install_manifests.keys()))
 
         # Add dependencies we infered:

Comment 4

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mike Hommey [:glandium] from comment #3)
> Please try this:

That doesn't fix the issue for me :( Best I can tell, that code isn't hit before the error throws. I made some futile attempts to inspect that temp .sh before it vanished but failed - if you can tell me where it is written I can probably arrange to see the contents.

Comment 5

[Mike Hommey [:glandium]]

Try this, with and without the patch from comment 3, and please tell me which combinations work if any works:

diff --git a/config/faster/rules.mk b/config/faster/rules.mk
index cf665a6..af634da 100644
--- a/config/faster/rules.mk
+++ b/config/faster/rules.mk
@@ -119,17 +119,17 @@ $(addprefix install-,$(INSTALL_MANIFESTS)): install-%: $(TOPOBJDIR)/config/build
 #
 # The list of chrome manifests is given in MANIFEST_TARGETS, relative to the
 # top object directory. The content for those manifests is given in the
 # `content` variable associated with the target. For example:
 #   MANIFEST_TARGETS = foo
 #   $(TOPOBJDIR)/foo: content = "manifest foo.manifest" "manifest bar.manifest"
 $(addprefix $(TOPOBJDIR)/,$(MANIFEST_TARGETS)): FORCE
        $(PYTHON) -m mozbuild.action.buildlist \
-               $@ \
+               '$@' \
                $(content)
 
 # ============================================================================
 # Below is a set of additional dependencies and variables used to build things
 # that are not supported by data in moz.build.
 
 # Files to build with the recursive backend and simply copy
 $(TOPOBJDIR)/dist/bin/platform.ini: $(TOPOBJDIR)/toolkit/xre/platform.ini

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Attachment: git diff && ./mach build faster -v

Comment 7

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mike Hommey [:glandium] from comment #5)
> Try this, with and without the patch from comment 3, and please tell me
> which combinations work if any works:

I responded on IRC, but for completeness, no combination worked.

Comment 8

[Mike Hommey [:glandium]]

Okay, found the root cause, which is that we're not quoting things in faster/Makefile, and bug 1191230 added things that break without quoting.

Comment 9

[Mike Hommey [:glandium]]

Attachment: Add support for files with a given content to install manifests

This will be used for chrome manifests in the faster make backend.

Comment 10

[Mike Hommey [:glandium]]

Attachment: Use new install manifests feature for chrome manifests in the FasterMake backend

Bug 1191230 added override lines with # characters to chrome manifests
for Windows.

So far, chrome manifests were handled with buildlist.py like in the
RecursiveMake backend, fed with Make variables. Without proper quoting,
those Make variables are just truncated by Make on the first # character,
and this results in mach build faster failing because of that.

However, the reason why chrome manifests were handled with buildlist.py
originally is that not all chrome manifest entries were known to the
FasterMake backend, but they now all are.

So instead of relying on Make variables and buildlist.py, we can now
rely on the newly added install manifests feature allowing to create files
with a given content.

Comment 11

[Mike Hommey [:glandium]]

Attachment: Add a jar.mn to the end-to-end test

Also add the corner case that broke mach build faster on Windows in bug 1191230.

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8701360 [details] [diff] [review]
Add support for files with a given content to install manifests

Review of attachment 8701360 [details] [diff] [review]:
-----------------------------------------------------------------

We should just put the whole source tree in install manifests :P

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

I wonder does adding '#XXX' in jar.mn really works? Shouldn't only the part before '#' involve in target locating?

Comment 14

[Mike Hommey [:glandium]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #13)
> I wonder does adding '#XXX' in jar.mn really works? Shouldn't only the part
> before '#' involve in target locating?

cf. bug 1232421 comment 5

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7c70c4d4ba4
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1bd6b86e5a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/b247d8f58538

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f7c70c4d4ba4
https://hg.mozilla.org/mozilla-central/rev/b1bd6b86e5a0
https://hg.mozilla.org/mozilla-central/rev/b247d8f58538