Closed
Bug 1059769
Opened 10 years ago
Closed 10 years ago
Add LIBRARY_DEFINES to moz.build
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla35
People
(Reporter: jcranmer, Assigned: jcranmer)
Details
Attachments
(2 files, 2 obsolete files)
15.78 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
2.20 KB,
patch
|
glandium
:
review+
|
Details | Diff | 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 1•10 years ago
|
||
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+
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8480564 -
Attachment is obsolete: true
Attachment #8481363 -
Flags: review?(mh+mozilla)
Comment 3•10 years ago
|
||
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+
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8481363 -
Attachment is obsolete: true
Attachment #8483939 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8483940 -
Flags: review?(mh+mozilla)
Comment 6•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8483940 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 7•10 years ago
|
||
(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
Comment 8•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3997eb3154aa
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•