Closed Bug 1371817 Opened 3 years ago Closed 3 years ago

Support wildcards in final target files in the tup backend

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: chmanchester, Assigned: chmanchester)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

These are needed for TEST_HARNESS_FILES, but are also emitted by jar.mn entries for browser features.
Assignee: nobody → cmanchester
Summary: Support wildcards in finall target files in the tup backend → Support wildcards in final target files in the tup backend
Blocks: 1372381
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.
Attachment #8877338 - Flags: review?(mshal)
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 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.
Attachment #8877338 - Flags: review?(mshal) → review+
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/77c937a09a97
Handle FinalTargetFiles with wildcards in the Tup backend. r=mshal
https://hg.mozilla.org/mozilla-central/rev/77c937a09a97
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.