Closed Bug 823351 Opened 7 years ago Closed 7 years ago

Refinements to Makefiles in bug 815473

Categories

(Core :: General, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: laszio.bugzilla, Assigned: laszio.bugzilla)

References

Details

Attachments

(1 file, 1 obsolete file)

(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?
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.
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)
(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.
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!)
https://hg.mozilla.org/mozilla-central/rev/752093e9327e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.