Closed Bug 1059769 Opened 5 years ago Closed 5 years ago

Add LIBRARY_DEFINES to moz.build

Categories

(Firefox Build System :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla35

People

(Reporter: jcranmer, Assigned: jcranmer)

Details

Attachments

(2 files, 2 obsolete files)

Attached patch Add LIBRARY_DEFINES (obsolete) — Splinter Review
I ended up trying a few different ways of resolving the LIBRARY_DEFINES to constituent libraries. This method ended up working the best IMHO, but it also makes it more difficult to make LIBRARY_DEFINES apply to libraries that are not final.
Attachment #8480564 - Flags: review?(mh+mozilla)
Comment on attachment 8480564 [details] [diff] [review]
Add LIBRARY_DEFINES

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

::: python/mozbuild/mozbuild/frontend/data.py
@@ +343,5 @@
>      def __init__(self, context):
>          ContextDerived.__init__(self, context)
>          self.linked_libraries = []
>          self.linked_system_libs = []
> +        self.defines = Defines(context, {})

I think this is reasonable to do, for now, but I'll most probably change how this works in the future.

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +639,5 @@
>                      'Please remove one.', context)
> +            if lib_defines:
> +                raise SandboxValidationError(
> +                    'LIBRARY_DEFINES cannot be used on a non-final library. '
> +                    'Please add to the FINAL_LIBRARY instead.', context)

There are FINAL_LIBRARY of something that have a FINAL_LIBRARY. While it's a theoretical issue, we could want to apply LIBRARY_DEFINES, to e.g. gkmedias, and that wouldn't work.
Attachment #8480564 - Flags: review?(mh+mozilla) → feedback+
Attached patch Add LIBRARY_DEFINES (obsolete) — Splinter Review
Attachment #8480564 - Attachment is obsolete: true
Attachment #8481363 - Flags: review?(mh+mozilla)
Comment on attachment 8481363 [details] [diff] [review]
Add LIBRARY_DEFINES

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

Please separate python/mozbuild changes from the rest when landing.

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +1161,5 @@
>      def _build_target_for_obj(self, obj):
>          return '%s/%s' % (mozpath.relpath(obj.objdir,
>              self.environment.topobjdir), obj.KIND)
>  
> +    def _process_linkable(self, obj, backend_file):

Not sure why you're renaming the function. I actually prefer the previous name because it's less ambiguous (one would thing process_linkable handles the whole linkage, when it only deals with its linked libraries)

::: python/mozbuild/mozbuild/frontend/data.py
@@ +372,5 @@
>  
> +    def propagate_defines(self, defines):
> +        self.defines.update(defines)
> +        for lib in self.linked_libraries:
> +            # Only propagate defines if it only links into this library.

Too many only.

@@ +374,5 @@
> +        self.defines.update(defines)
> +        for lib in self.linked_libraries:
> +            # Only propagate defines if it only links into this library.
> +            if isinstance(lib, StaticLibrary) and lib.link_into == self.basename:
> +                lib.propagate_defines(defines)

I think this would be better in emitter.py, like it was before.

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +717,5 @@
>                  self._libs[libname].append(lib)
>                  self._linkage.append((context, lib, 'USE_LIBS'))
>  
> +            if lib_defines:
> +                lib.defines.update(lib_defines)

Please raise an exception when LIBRARY_DEFINES is defined with no LIBRARY_NAME.
Attachment #8481363 - Flags: review?(mh+mozilla) → feedback+
Attachment #8481363 - Attachment is obsolete: true
Attachment #8483939 - Flags: review?(mh+mozilla)
Comment on attachment 8483939 [details] [diff] [review]
Add LIBRARY_DEFINES

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

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ -224,5 @@
> -                            continue
> -                        sent_passthru.add(p)
> -                        passthru = VariablePassthru(contexts[p])
> -                        passthru.variables['FINAL_LIBRARY'] = lib.basename
> -                        yield passthru

So, the sad thing is that this breaks the build until the second patch is applied. So in the end, might be better to fold them than to keep support for FINAL_LIBRARY passthrough in this patch to remove it in the next.
Attachment #8483939 - Flags: review?(mh+mozilla) → review+
Attachment #8483940 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] (out from Sep 6 to Sep 22) from comment #6)
> So, the sad thing is that this breaks the build until the second patch is
> applied. So in the end, might be better to fold them than to keep support
> for FINAL_LIBRARY passthrough in this patch to remove it in the next.

That must have been why I never tried to separate them in the first place :-)

 https://hg.mozilla.org/integration/mozilla-inbound/rev/3997eb3154aa
https://hg.mozilla.org/mozilla-central/rev/3997eb3154aa
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.