Closed
Bug 1371817
Opened 7 years ago
Closed 7 years ago
Support wildcards in final target files in the tup backend
Categories
(Firefox Build System :: General, enhancement)
Firefox Build System
General
Tracking
(firefox56 fixed)
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: chmanchester, Assigned: chmanchester)
References
Details
Attachments
(1 file)
These are needed for TEST_HARNESS_FILES, but are also emitted by jar.mn entries for browser features.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → cmanchester
Assignee | ||
Updated•7 years ago
|
Summary: Support wildcards in finall target files in the tup backend → Support wildcards in final target files in the tup backend
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 3•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment 5•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/77c937a09a97
Handle FinalTargetFiles with wildcards in the Tup backend. r=mshal
Comment 8•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•