Introduce rules to install files into $(DIST)/bin/res

RESOLVED FIXED in mozilla30

Status

Firefox Build System
General
RESOLVED FIXED
6 years ago
4 months ago

People

(Reporter: Ms2ger, Assigned: bokeefe)

Tracking

(Blocks: 1 bug)

Trunk
mozilla30
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Updated

5 years ago
Depends on: 935987
(Assignee)

Comment 1

5 years ago
Created attachment 828658 [details] [diff] [review]
Part 1: Add RESOURCE_FILES to moz.build

This adds two new moz.build symbols, RESOURCE_FILES and PP_RESOURCE_FILES, that work like EXPORTS, except they install to $(DIST)/bin/res. Using the install manifest means this happens during export instead of libs (which most of the current rules are), but I don't think that makes a whole lot of difference for these guys.

I preemptively added the new symbols to the blacklist in config.mk. None of the existing makefiles use them, so that shouldn't hurt anything.
Assignee: nobody → bokeefe
Status: NEW → ASSIGNED
Attachment #828658 - Flags: review?(mshal)
(Assignee)

Comment 2

5 years ago
Created attachment 828661 [details] [diff] [review]
Part 2: Move manual $(DIST)/bin/res rules to RESOURCE_FILES in moz.build

This part is mostly straightforward moves of the manual install rules into moz.build.

I'd like to preemptively apologize for this:
> /widget/cocoa/moz.build
> +RESOURCE_FILES.__setattr__('MainMenu.nib', [
> +    'resources/MainMenu.nib/classes.nib',
> +    'resources/MainMenu.nib/info.nib',
> +    'resources/MainMenu.nib/keyedobjects.nib',
> +])

It's a terrible hack, but the directory name had a '.' in it, and there didn't seem to be a way around that.

Also, green try run: https://tbpl.mozilla.org/?tree=Try&rev=63ec57f73dbb (for parts 1+2, plus the two bugs this needs first.
Attachment #828661 - Flags: review?(mshal)
(Assignee)

Updated

5 years ago
Blocks: 915804
Comment on attachment 828658 [details] [diff] [review]
Part 1: Add RESOURCE_FILES to moz.build

>@@ -905,16 +909,40 @@ class RecursiveMakeBackend(CommonBackend
>             if not os.path.exists(source):
>                 raise Exception('File listed in EXPORTS does not exist: %s' % source)
> 
>         children = exports.get_children()
>         for subdir in sorted(children):
>             self._process_exports(obj, children[subdir], backend_file,
>                 namespace=namespace + subdir)
> 
>+    def _process_resources(self, obj, resources, backend_file, namespace=""):
>+        strings = resources.get_strings()
>+        if namespace:
>+            namespace += '/'
>+
>+        for s in strings:
>+            source = os.path.normpath(os.path.join(obj.srcdir, s))
>+            dest = 'res/%s%s' % (namespace, os.path.basename(s))
>+
>+            if obj.preprocess:
>+                if dest.endswith('.in'):
>+                    dest = dest[:-3]
>+                self._install_manifests['dist_bin'].add_preprocess(source, dest, marker='%', defines=obj.defines)
>+            else:
>+                self._install_manifests['dist_bin'].add_symlink(source, dest)
>+
>+            if not os.path.exists(source):
>+                raise Exception('File listed in RESOURCE_FILES does not exist: %s' % source)
>+
>+        children = resources.get_children()
>+        for subdir in sorted(children):
>+            self._process_resources(obj, children[subdir], backend_file,
>+                namespace=namespace + subdir)
>+

Is there an easy way to pull out the common bits of this function and _process_exports()? The get_strings() function (or some new function) should probably be doing the subdir iteration. I'd be happy with just a followup bug for now if it will delay this too much.

>diff --git a/python/mozbuild/mozbuild/frontend/data.py b/python/mozbuild/mozbuild/frontend/data.py
>+class Resources(SandboxDerived):
>+    """Sandbox container object for RESOURCE_FILES, which is a HierarchicalStringList.
>+
>+    If the resource needs to be preproccessed, the local defines plus anything in

preproccessed -> preprocessed

>diff --git a/python/mozbuild/mozbuild/test/backend/data/resources/test.manifest b/python/mozbuild/mozbuild/test/backend/data/resources/test.manifest
>new file mode 100644
>diff --git a/python/mozbuild/mozbuild/test/backend/test_recursivemake.py b/python/mozbuild/mozbuild/test/backend/test_recursivemake.py
>--- a/python/mozbuild/mozbuild/test/backend/test_recursivemake.py
>+++ b/python/mozbuild/mozbuild/test/backend/test_recursivemake.py
>@@ -367,16 +367,32 @@ class TestRecursiveMakeBackend(BackendTe
>         # EXPORTS files should appear in the dist_include install manifest.
>         m = InstallManifest(path=os.path.join(env.topobjdir,
>             '_build_manifests', 'install', 'dist_include'))
>         self.assertEqual(len(m), 7)
>         self.assertIn('foo.h', m)
>         self.assertIn('mozilla/mozilla1.h', m)
>         self.assertIn('mozilla/dom/dom2.h', m)
> 
>+    def test_resources(self):
>+        """Ensure RESOURCE_FILES and PP_RESOURCE_FILES are handled properly."""
>+        env = self._consume('resources', RecursiveMakeBackend)
>+
>+        # RESOURCE_FILES should appear in the dist_include install manifest.

Is this comment supposed to say 'dist_bin install manifest' instead of 'dist_include'?

Also, the new test seems to be failing with:

  File "/home/marf/mozilla-central-git/python/mozbuild/mozbuild/backend/recursivemake.py", line 929, in _process_resources
    self._install_manifests['dist_bin'].add_preprocess(source, dest, marker='%', defines=obj.defines)

I tried adding the dependent bug (935987), but that fails differently :). I can r+ once that is sorted out.
Attachment #828658 - Flags: review?(mshal) → feedback+
Comment on attachment 828661 [details] [diff] [review]
Part 2: Move manual $(DIST)/bin/res rules to RESOURCE_FILES in moz.build

>diff --git a/layout/mathml/Makefile.in b/layout/mathml/Makefile.in
>-$(DIST)/bin/res/fonts/$(math_properties) $(DESTDIR)$(mozappdir)/res/fonts/$(math_properties): $(math_properties) Makefile
>-	test -d $(@D) || $(NSINSTALL) -D $(@D)
>-	rm -f $@
>-	$(PYTHON) $(MOZILLA_DIR)/config/Preprocessor.py --marker=% $(DEFINES) $(ACDEFINES) $< > $@

I didn't notice this in the other patch, but do all PP_RESOURCE_FILES use the same '%' marker? Or will we need some way to specify that in the moz.build file? Looks like it'll work for now since this is the only preprocessed one in the patch.

>+RESOURCE_FILES.__setattr__('MainMenu.nib', [
>+    'resources/MainMenu.nib/classes.nib',
>+    'resources/MainMenu.nib/info.nib',
>+    'resources/MainMenu.nib/keyedobjects.nib',
>+])

I don't have a suggestion here, unfortunately - maybe just add a comment to explain the funky setattr call?

Looks good!
Attachment #828661 - Flags: review?(mshal) → review+
(Assignee)

Comment 5

5 years ago
(In reply to Michael Shal [:mshal] from comment #3)
> Also, the new test seems to be failing with:
> 
>   File
> "/home/marf/mozilla-central-git/python/mozbuild/mozbuild/backend/
> recursivemake.py", line 929, in _process_resources
>     self._install_manifests['dist_bin'].add_preprocess(source, dest,
> marker='%', defines=obj.defines)
> 
> I tried adding the dependent bug (935987), but that fails differently :). I
> can r+ once that is sorted out.

Yeah, this definitely needs bug 935987, but that conflicted with glandium's changes in bug 935305. Once I get that squared away, I'll post updated patches.

Thanks!
I've been on working a slightly more generic version of this in bug 910781, which installs anything into $(FINAL_TARGET) [but doesn't do any preprocessing].
See Also: → bug 910781
(Assignee)

Comment 7

4 years ago
Created attachment 8370885 [details] [diff] [review]
Part 1: Add RESOURCE_FILES to moz.build

I finally got around to updating these patches. Part 1 changed a bit:
Instead of using two variables, RESOURCE_FILES and PP_RESOURCE_FILES, I added magic elements to HierarchicalStringList, so it works like StrictOrderingOnAppendListWithFlags. That lets me pretty up the preprocessed files (all one of them) by saying RESOURCE_FILES['foo'].preprocess = True.

I also pulled out the common HierarchicalStringList processing in the backend. gps's patch for walk() over in bug 853594 would make that even nicer, really.

(In reply to Joshua Cranmer [:jcranmer] from comment #6)
> I've been on working a slightly more generic version of this in bug 910781,
> which installs anything into $(FINAL_TARGET) [but doesn't do any
> preprocessing].

I'll leave it up to the build config guys which way to go here, but you're welcome to steal the HierarchicalStringList changes, which gives you preprocessing support in the other bug for just about free.
Attachment #828658 - Attachment is obsolete: true
Attachment #8370885 - Flags: review?(mshal)
(Assignee)

Comment 8

4 years ago
Created attachment 8370888 [details] [diff] [review]
Part 2: Move manual $(DIST)/bin/res rules to RESOURCE_FILES in moz.build

(In reply to Michael Shal [:mshal] from comment #4)
> Comment on attachment 828661 [details] [diff] [review]
> Part 2: Move manual $(DIST)/bin/res rules to RESOURCE_FILES in moz.build
> 
> >diff --git a/layout/mathml/Makefile.in b/layout/mathml/Makefile.in
> >-$(DIST)/bin/res/fonts/$(math_properties) $(DESTDIR)$(mozappdir)/res/fonts/$(math_properties): $(math_properties) Makefile
> >-	test -d $(@D) || $(NSINSTALL) -D $(@D)
> >-	rm -f $@
> >-	$(PYTHON) $(MOZILLA_DIR)/config/Preprocessor.py --marker=% $(DEFINES) $(ACDEFINES) $< > $@
> 
> I didn't notice this in the other patch, but do all PP_RESOURCE_FILES use
> the same '%' marker? Or will we need some way to specify that in the
> moz.build file? Looks like it'll work for now since this is the only
> preprocessed one in the patch.

For the moment, there's only one file that needs to be preprocessed, and it uses the '%' marker. Although, with the new flavor of preprocessing from part 1, we could add a '.marker' property if it's an issue in the future.
Attachment #828661 - Attachment is obsolete: true
Attachment #8370888 - Flags: review?(mshal)
(Reporter)

Comment 9

4 years ago
Comment on attachment 8370885 [details] [diff] [review]
Part 1: Add RESOURCE_FILES to moz.build

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +875,5 @@
>  
>          for tier in set(self._may_skip.keys()) - affected_tiers:
>              self._may_skip[tier].add(backend_file.relobjdir)
>  
> +    def _process_heirarchy(self, obj, element, namespace, action):

heirarchy isn't written that way
(Assignee)

Comment 10

4 years ago
Created attachment 8371600 [details] [diff] [review]
Part 1: Add RESOURCE_FILES to moz.build

(In reply to :Ms2ger from comment #9)
> Comment on attachment 8370885 [details] [diff] [review]
> Part 1: Add RESOURCE_FILES to moz.build
> 
> Review of attachment 8370885 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mozbuild/mozbuild/backend/recursivemake.py
> @@ +875,5 @@
> >  
> >          for tier in set(self._may_skip.keys()) - affected_tiers:
> >              self._may_skip[tier].add(backend_file.relobjdir)
> >  
> > +    def _process_heirarchy(self, obj, element, namespace, action):
> 
> heirarchy isn't written that way

*sigh* Thanks. I need spell checking for my code, apparently.
Attachment #8370885 - Attachment is obsolete: true
Attachment #8370885 - Flags: review?(mshal)
Attachment #8371600 - Flags: review?(mshal)
jcranmer: Do you think the preprocessing support (and general implementation) in this bug could also be used by 910781? If so I'm inclined to r+ this now and then we can change RESOURCES_FILES to FINAL_TARGET_FILES.res pretty easily in 910781 (or as another followup).
Flags: needinfo?(Pidgeot18)
(In reply to Michael Shal [:mshal] from comment #11)
> jcranmer: Do you think the preprocessing support (and general
> implementation) in this bug could also be used by 910781? If so I'm inclined
> to r+ this now and then we can change RESOURCES_FILES to
> FINAL_TARGET_FILES.res pretty easily in 910781 (or as another followup).

Probably. The one thing that concerns me is in the use of DEFINES present in Makefiles (including things in config.mk!) that are not present in the moz.build files. I'm leery of adding any significant preprocessing support without having something that can catch that issue.
Flags: needinfo?(Pidgeot18)
(In reply to Joshua Cranmer [:jcranmer] from comment #12)
> Probably. The one thing that concerns me is in the use of DEFINES present in
> Makefiles (including things in config.mk!) that are not present in the
> moz.build files. I'm leery of adding any significant preprocessing support
> without having something that can catch that issue.

I think we can probably get by with diffing the before/after for any preprocessed files moved into moz.build (that's what I did to review this, anyway). Once bug 930703 is finished, the concern should go away.

For now I think it's best to land this, and if we want to make it a part of FINAL_TARGET_FILES it should be pretty easy to do.
Comment on attachment 8371600 [details] [diff] [review]
Part 1: Add RESOURCE_FILES to moz.build

>diff --git a/config/config.mk b/config/config.mk
>--- a/config/config.mk
>+++ b/config/config.mk
>@@ -56,16 +56,18 @@ endif
>   JS_MODULES_PATH \
>   LIBRARY_NAME \
>   LIBXUL_LIBRARY \
>   MODULE \
>   MSVC_ENABLE_PGO \
>   NO_DIST_INSTALL \
>   PARALLEL_DIRS \
>   PROGRAM \
>+  PP_RESOURCE_FILES \

PP_RESOURCE_FILES no longer exists, so it doesn't need to go here.

>+class Resources(SandboxDerived):
>+    """Sandbox container object for RESOURCE_FILES, which is a HierarchicalStringList,
>+    with an extra ``.preprocessed`` property on each entry.

Should this be ``.preprocess`` instead of ``.preprocessed`` ?
Attachment #8371600 - Flags: review?(mshal) → review+
Comment on attachment 8370888 [details] [diff] [review]
Part 2: Move manual $(DIST)/bin/res rules to RESOURCE_FILES in moz.build

>diff --git a/layout/mathml/Makefile.in b/layout/mathml/Makefile.in
>deleted file mode 100644
>--- a/layout/mathml/Makefile.in
>+++ /dev/null
>-$(DIST)/bin/res/fonts/$(math_properties) $(DESTDIR)$(mozappdir)/res/fonts/$(math_properties): $(math_properties) Makefile
>-	test -d $(@D) || $(NSINSTALL) -D $(@D)
>-	rm -f $@
>-	$(call py_action,preprocessor,--marker=% $(DEFINES) $(ACDEFINES) $< -o $@)
>-
>-libs:: $(DIST)/bin/res/fonts/$(math_properties)
>-install:: $(DESTDIR)$(mozappdir)/res/fonts/$(math_properties)

Are we sure the install:: rule for $(DESTDIR)$(mozappdir)/res/fonts/$(math_properties) isn't needed? I don't see how that is handled inside of moz.build. (I also don't see how this rule gets used at all from a 'make' or 'make install', though).

Also was try happy with this version?

Everything else looks good to me - if the install:: rule isn't necessary I can r+
Comment on attachment 8370888 [details] [diff] [review]
Part 2: Move manual $(DIST)/bin/res rules to RESOURCE_FILES in moz.build

f+, may convert to r+ based on above questions.
Flags: needinfo?(bokeefe)
Attachment #8370888 - Flags: review?(mshal) → feedback+
A long time ago, make install was doing a full recursion of the tree. It doesn't anymore. Except for js/src, all install rules in the tree are remnants of that ancient past.
(Assignee)

Comment 18

4 years ago
Created attachment 8373986 [details] [diff] [review]
Part 1: Add RESOURCE_FILES to moz.build, v3 (r=mshal)

(In reply to Michael Shal [:mshal] from comment #14)
> Comment on attachment 8371600 [details] [diff] [review]
> Part 1: Add RESOURCE_FILES to moz.build
> 
> >diff --git a/config/config.mk b/config/config.mk
> >--- a/config/config.mk
> >+++ b/config/config.mk
> >@@ -56,16 +56,18 @@ endif
> >   JS_MODULES_PATH \
> >   LIBRARY_NAME \
> >   LIBXUL_LIBRARY \
> >   MODULE \
> >   MSVC_ENABLE_PGO \
> >   NO_DIST_INSTALL \
> >   PARALLEL_DIRS \
> >   PROGRAM \
> >+  PP_RESOURCE_FILES \
> 
> PP_RESOURCE_FILES no longer exists, so it doesn't need to go here.

Ah, yes, I forgot I added it there.

> >+class Resources(SandboxDerived):
> >+    """Sandbox container object for RESOURCE_FILES, which is a HierarchicalStringList,
> >+    with an extra ``.preprocessed`` property on each entry.
> 
> Should this be ``.preprocess`` instead of ``.preprocessed`` ?

Yes, no 'ed'.

New patch, with both issues fixed. I carried forward the r+.
Attachment #8371600 - Attachment is obsolete: true
Attachment #8373986 - Flags: review+
(Assignee)

Comment 19

4 years ago
Created attachment 8373990 [details] [diff] [review]
Part 2: Move manual $(DIST)/bin/res rules to RESOURCE_FILES in moz.build

(In reply to Michael Shal [:mshal] from comment #15)
> Comment on attachment 8370888 [details] [diff] [review]
> Part 2: Move manual $(DIST)/bin/res rules to RESOURCE_FILES in moz.build
> 
> >diff --git a/layout/mathml/Makefile.in b/layout/mathml/Makefile.in
> >deleted file mode 100644
> >--- a/layout/mathml/Makefile.in
> >+++ /dev/null
> >-$(DIST)/bin/res/fonts/$(math_properties) $(DESTDIR)$(mozappdir)/res/fonts/$(math_properties): $(math_properties) Makefile
> >-	test -d $(@D) || $(NSINSTALL) -D $(@D)
> >-	rm -f $@
> >-	$(call py_action,preprocessor,--marker=% $(DEFINES) $(ACDEFINES) $< -o $@)
> >-
> >-libs:: $(DIST)/bin/res/fonts/$(math_properties)
> >-install:: $(DESTDIR)$(mozappdir)/res/fonts/$(math_properties)
> 
> Are we sure the install:: rule for
> $(DESTDIR)$(mozappdir)/res/fonts/$(math_properties) isn't needed? I don't
> see how that is handled inside of moz.build. (I also don't see how this rule
> gets used at all from a 'make' or 'make install', though).

Yeah, like glandium said, nothing ran this rule, so I didn't try to keep it around. FWIW, glandium removed a handful of other install:: rules in bug 935387.

> Also was try happy with this version?

I had thought it was, but I couldn't find my last push, so I re-pushed. It turns out it OSX didn't like this version, because some of the cursors were removed. So, I fixed those and pushed to try again, and that was green: https://tbpl.mozilla.org/?tree=Try&rev=99b9c97a9674
Attachment #8370888 - Attachment is obsolete: true
Attachment #8373990 - Flags: review?(mshal)
Flags: needinfo?(bokeefe)
Comment on attachment 8373990 [details] [diff] [review]
Part 2: Move manual $(DIST)/bin/res rules to RESOURCE_FILES in moz.build

Sounds good!
Attachment #8373990 - Flags: review?(mshal) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c9450b0ced26
https://hg.mozilla.org/mozilla-central/rev/8bc655300999
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(Assignee)

Updated

4 years ago
Duplicate of this bug: 883093

Updated

4 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.