993448 - Escape spaces in dependencies generated by makeutil

Status: NEW

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

I got annoyed by the fact that the ANGLE libs (libGLESv2.dll and libEGL.dll) were rebuilt every time I did a build. It turns out that this is because they link in some D3D libs from the SDK and the paths to those libs include spaces. Since Make has no concept of spaces this leads to screwed up deps. Conveniently, expandlibs_exec writes those deps using makeutil, so fixing this is trivial.

This patch uses the single-character '?' wildcard in place of spaces when writing deps into Makefiles. This is hacky but the alternative is pretty clearly broken, since if you do:
r = Rule('foo')
r.add_dependencies(['abc xyz'])

You'll currently get:
foo: abc xyz

which is broken. With this patch you'll get:

foo: abc?xyz

which looks weird but works.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: Use wildcards in place of spaces in dependencies generated by makeutil

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8403348 [details] [diff] [review]
Use wildcards in place of spaces in dependencies generated by makeutil

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

Just use backslashes. Last time i tried, it worked.

Comment 3

[(not currently active) Ted Mielczarek]

So I tried that, but SimpleOrderedSet does path normalization:
http://hg.mozilla.org/mozilla-central/annotate/43cd629879c2/python/mozbuild/mozbuild/makeutil.py#l80

This breaks putting backslash escapes in at the level I did. I guess I can move this down the stack into SimpleOrderedSet itself.

Comment 4

[(not currently active) Ted Mielczarek]

Attachment: Escape spaces in dependencies generated by makeutil

Okay, I switched it to use backslash-escaping instead. This revealed a problem whereby one of the ipdlsrcs rules was abusing makeutils to create a static pattern rule. Escaping spaces broke that rule, so I had to fix it. I chose to make an explicit StaticPatternRule class and use that so the meaning was clear. It wound up making the patch a bit more complicated, but should avoid issues there in the future.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8403990 [details] [diff] [review]
Escape spaces in dependencies generated by makeutil

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +687,5 @@
>              makefile.add_statement('all_absolute_unified_files := \\\n'
>                                     '  $(addprefix $(CURDIR)/,$(%s))'
>                                     % unified_files_makefile_variable)
> +            rule = makefile.create_static_pattern_rule(['$(all_absolute_unified_files)'], '$(CURDIR)/%')
> +            rule.add_dependencies(['%'])

I'd be fine with not adding an API for static pattern rules, and just adding '$(CURDIR)/%:' and  '%' as separate dependencies.

::: python/mozbuild/mozbuild/makeutil.py
@@ +85,5 @@
>      def update(self, iterable):
>          def _add(iterable):
>              emitted = set()
>              for i in iterable:
> +                i = i.replace(os.sep, '/').replace(' ', '\\ ')

This should be done when dumping instead, so that things that do get dependencies get the right ones (link_deps.py does, for instance).

@@ +171,5 @@
>  # colon followed by anything except a slash (Windows path detection)
>  _depfilesplitter = re.compile(r':(?![\\/])')
>  
>  
>  def read_dep_makefile(fh):

You also need to update the reader so that it can read those properly.

::: python/mozbuild/mozbuild/test/test_makeutil.py
@@ +37,5 @@
> +        rule.add_targets(['foo zzz'])
> +        rule.add_dependencies(['abc xyz', 'bar', 'def 123'])
> +        rule.dump(out)
> +        self.assertEqual(out.getvalue(), 'foo\\ zzz: abc\\ xyz bar def\\ 123\n')
> +        out.truncate(0)

Please add a test that rule.targets() and rules.dependencies() do contain the right things cf. previous comment.