823351 - Refinements to Makefiles in bug 815473

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Ting-Yuan Huang]

(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

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

ehsan is fixing the gfxUtils one in bug 827934, can you please handle the others?

Comment 2

[(no longer active)]

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

Comment 3

[Ting-Yuan Huang]

Attachment: 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?

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

[Justin Lebar (not reading bugmail)]

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.

Comment 6

[Ting-Yuan Huang]

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

Comment 7

[Ting-Yuan Huang]

Attachment: Setup correct dependency of jchuff.c on jpeg_nbits_table.h. V2

Comment 8

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/752093e9327e

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

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/752093e9327e