Closed
Bug 823351
Opened 12 years ago
Closed 12 years ago
Refinements to Makefiles in bug 815473
Categories
(Core :: General, defect)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: laszio.bugzilla, Assigned: laszio.bugzilla)
References
Details
Attachments
(1 file, 1 obsolete file)
702 bytes,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
(In reply to Gregory Szorc [:gps] from comment #38)
> Comment on attachment 691682 [details] [diff] [review]
> Part 2/3: Replace runtime computed sUnpremultiplyTable/sPremultiplyTable
> with constants. r=roc
>
> Review of attachment 691682 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: gfx/thebes/Makefile.in
> @@ +397,5 @@
> > +.PHONY: CONSTANT_TABLES
> > +CONSTANT_TABLES:
> > + $(PYTHON) $(srcdir)/genTables.py
> > +
> > +gfxUtils.$(OBJ_SUFFIX): CONSTANT_TABLES
>
> This is less than ideal for a few reasons.
>
> 1) genTables.py actually produces files. So, the use of a .PHONY target is
> inappropriate. You should instead list the files it produces explicitly in
> the Makefile.
>
> 2) The CONSTANT_TABLES rule will be evaluated needlessly. This isn't too bad
> because it doesn't take long. Still, it's something we like to avoid.
>
> I'd do something like:
>
> gen_tables_output := sPremultiplyTable.h sUnpremultiplyTable.h
>
> $(gen_tables_output): $(srcdir)/genTables.py
> $(PYTHON) $(srcdir)/genTables.py
>
> gfxUtils.$(OBJ_SUFFIX): $(gen_tables_output)
>
> ---
>
> What you have will work, it just isn't fully proper. I think a follow-up bug
> should be sufficient if you don't want to deal with it now.
To refine the Makefile targets.
Comment 1•12 years ago
|
||
ehsan is fixing the gfxUtils one in bug 827934, can you please handle the others?
Comment 2•12 years ago
|
||
(In reply to :Ms2ger from comment #1)
> ehsan is fixing the gfxUtils one in bug 827934, can you please handle the
> others?
That patch is on inbound now.
Assignee | ||
Comment 3•12 years ago
|
||
The patch prevents jchuff.c from being built and genTables.py being called even when nothing changed.
Should we also check this and
"Bug 827934 - We build gfxUtils.cpp every time even if nothing has changed"
into b2g18?
Attachment #699728 -
Flags: review?(justin.lebar+bug)
Comment 4•12 years ago
|
||
Comment on attachment 699728 [details] [diff] [review]
Setup correct dependency of jchuff.c on jpeg_nbits_table.h
Review of attachment 699728 [details] [diff] [review]:
-----------------------------------------------------------------
::: media/libjpeg/Makefile.in
@@ +163,5 @@
> FORCE_STATIC_LIB = 1
>
> include $(topsrcdir)/config/rules.mk
>
> +jpeg_nbits_table.h:
Needs to depend of genTables.py
Comment 5•12 years ago
|
||
Comment on attachment 699728 [details] [diff] [review]
Setup correct dependency of jchuff.c on jpeg_nbits_table.h
I erred in reviewing this in the first place.
I don't think we need this on b2g18.
Attachment #699728 -
Flags: review?(justin.lebar+bug) → review?(ted)
Assignee | ||
Comment 6•12 years ago
|
||
(In reply to :Ms2ger from comment #4)
> Comment on attachment 699728 [details] [diff] [review]
> Setup correct dependency of jchuff.c on jpeg_nbits_table.h
>
> Review of attachment 699728 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: media/libjpeg/Makefile.in
> @@ +163,5 @@
> > FORCE_STATIC_LIB = 1
> >
> > include $(topsrcdir)/config/rules.mk
> >
> > +jpeg_nbits_table.h:
>
> Needs to depend of genTables.py
You are right, I just followed Bug 827934 and didn't notice that.
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #699755 -
Flags: review?(ted)
Updated•12 years ago
|
Attachment #699728 -
Attachment is obsolete: true
Attachment #699728 -
Flags: review?(ted)
Updated•12 years ago
|
Attachment #699755 -
Flags: review?(ted) → review+
Comment 8•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/752093e9327e
(Thanks a lot for your patch, this was driving me crazy!)
Comment 9•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in
before you can comment on or make changes to this bug.
Description
•