If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Refinements to Makefiles in bug 815473

RESOLVED FIXED in mozilla21

Status

()

Core
General
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ting-Yuan Huang, Assigned: Ting-Yuan Huang)

Tracking

unspecified
mozilla21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
(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.
ehsan is fixing the gfxUtils one in bug 827934, can you please handle the others?

Updated

5 years ago
Depends on: 827934
(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

5 years ago
Created attachment 699728 [details] [diff] [review]
Setup correct dependency of jchuff.c on jpeg_nbits_table.h

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 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 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

5 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

5 years ago
Created attachment 699755 [details] [diff] [review]
Setup correct dependency of jchuff.c on jpeg_nbits_table.h. V2
Attachment #699755 - Flags: review?(ted)
Attachment #699728 - Attachment is obsolete: true
Attachment #699728 - Flags: review?(ted)
Attachment #699755 - Flags: review?(ted) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/752093e9327e

(Thanks a lot for your patch, this was driving me crazy!)

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/752093e9327e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.