1371817 - Support wildcards in final target files in the tup backend

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Chris Manchester (limited bugmail, email directly)]

These are needed for TEST_HARNESS_FILES, but are also emitted by jar.mn entries for browser features.

Comment 1

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1371817 - Handle FinalTargetFiles with wildcards in the Tup backend.

Review commit: https://reviewboard.mozilla.org/r/148698/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148698/

Comment 2

[Michael Shal [:mshal]]

Comment on attachment 8877338 [details]
Bug 1371817 - Handle FinalTargetFiles with wildcards in the Tup backend.

https://reviewboard.mozilla.org/r/148698/#review153562

::: python/mozbuild/mozbuild/backend/tup.py:313
(Diff revision 1)
>                  'dist/sdk',
>              ))
>              if not path:
>                  raise Exception("Cannot install to " + target)
>  
> +        if target.startswith('_tests'):

It looks like the problem is that the '**' wildcards aren't handled yet. Maybe we could just skip the '**' files for now down below?

::: python/mozbuild/mozbuild/backend/tup.py:338
(Diff revision 1)
> -                            # TODO: This is needed for tests
> -                            pass
> +                            def _prefix(s):
> +                                for p in mozpath.split(s):
> +                                    if '*' not in p:
> +                                        yield p + '/'
> +                            prefix = ''.join(_prefix(f.full_path))
> +                            finder = FileFinder(prefix)

The downside with using FileFinder() to get the list of files in dir/* is that mozbuild doesn't seem to automatically regenerate the build backend if a new file is added. For example:

$ ./mach build
$ touch browser/extensions/pdfjs/content/newpdf.jsm
$ ./mach build

Here I'd expect a newpdf.jsm symlink to be created in the objdir, but that doesn't happen unless you force regenerating the build backend.

Worse is if you then remove the extra file:

$ rm browser/extensions/pdfjs/content/newpdf.jsm
$ ./mach build
 0:01.06 * 100% 2) [0.002s] obj-x86_64-pc-linux-gnu/dist/bin/browser/chrome/pdfjs/content
 0:01.06 tup error: Explicitly named file 'newpdf.jsm' not found in subdir 'browser/extensions/pdfjs/content'
 0:01.06 tup error: Error parsing Tupfile line 12

Again manually rebuilding the backend fixes the problem, but of course we shouldn't have to do that in the first place.

It looks like the recursive make backend works around this by clobbering this directory and recreating the symlinks on every build. Some possibilities here are:

1) Add the directories as dependencies on the backend, so mozbuild will re-generate the backend when files are added are removed

2) Use tup's "foreach" rule, so that tup is expanding the wildcard instead of mozbuild. In this case tup will automatically re-parse the Tupfile and generate new rules when files are added or removed. However, this does not work recursively, so I'm not sure if this is a solution

3) Symlink only the directory instead of every file in the directory

Feel free to reach out if none of these pan out or you get stuck.

Comment 3

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8877338 [details]
Bug 1371817 - Handle FinalTargetFiles with wildcards in the Tup backend.

https://reviewboard.mozilla.org/r/148698/#review153562

> It looks like the problem is that the '**' wildcards aren't handled yet. Maybe we could just skip the '**' files for now down below?

Yes, that seems to work fine.

> The downside with using FileFinder() to get the list of files in dir/* is that mozbuild doesn't seem to automatically regenerate the build backend if a new file is added. For example:
> 
> $ ./mach build
> $ touch browser/extensions/pdfjs/content/newpdf.jsm
> $ ./mach build
> 
> Here I'd expect a newpdf.jsm symlink to be created in the objdir, but that doesn't happen unless you force regenerating the build backend.
> 
> Worse is if you then remove the extra file:
> 
> $ rm browser/extensions/pdfjs/content/newpdf.jsm
> $ ./mach build
>  0:01.06 * 100% 2) [0.002s] obj-x86_64-pc-linux-gnu/dist/bin/browser/chrome/pdfjs/content
>  0:01.06 tup error: Explicitly named file 'newpdf.jsm' not found in subdir 'browser/extensions/pdfjs/content'
>  0:01.06 tup error: Error parsing Tupfile line 12
> 
> Again manually rebuilding the backend fixes the problem, but of course we shouldn't have to do that in the first place.
> 
> It looks like the recursive make backend works around this by clobbering this directory and recreating the symlinks on every build. Some possibilities here are:
> 
> 1) Add the directories as dependencies on the backend, so mozbuild will re-generate the backend when files are added are removed
> 
> 2) Use tup's "foreach" rule, so that tup is expanding the wildcard instead of mozbuild. In this case tup will automatically re-parse the Tupfile and generate new rules when files are added or removed. However, this does not work recursively, so I'm not sure if this is a solution
> 
> 3) Symlink only the directory instead of every file in the directory
> 
> Feel free to reach out if none of these pan out or you get stuck.

Thank you for catching this, and for the explanation. Making the backend depend on the directories in question seems to work just fine!

Comment 4

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8877338 [details]
Bug 1371817 - Handle FinalTargetFiles with wildcards in the Tup backend.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148698/diff/1-2/

Comment 5

[Michael Shal [:mshal]]

Comment on attachment 8877338 [details]
Bug 1371817 - Handle FinalTargetFiles with wildcards in the Tup backend.

https://reviewboard.mozilla.org/r/148698/#review154510

::: python/mozbuild/mozbuild/backend/tup.py:333
(Diff revision 2)
> +                            def _prefix(s):
> +                                for p in mozpath.split(s):
> +                                    if '*' not in p:
> +                                        yield p + '/'
> +                            prefix = ''.join(_prefix(f.full_path))
> +                            self.backend_input_files.add(prefix)

This does fix that case. Thanks!

Though it seems the directory timestamps are updated just when creating temporary files (eg: an editor like vim will create a .swp file in the same directory as the file, so editing an existing file will cause the directory timestamp to change). This means we run mozbuild more than strictly necessary. It does produce the correct result though, so we can revisit it later if it is problematic.

Comment 6

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8877338 [details]
Bug 1371817 - Handle FinalTargetFiles with wildcards in the Tup backend.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148698/diff/2-3/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77c937a09a97
Handle FinalTargetFiles with wildcards in the Tup backend. r=mshal

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/77c937a09a97