757828 - makefiles: move export logic into target_export.mk - batch #1

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Joey Armstrong [:joey]]

Attachment: move export logic into target_export.mk - batch #1

Comment 2

[Joey Armstrong [:joey]]

Comment on attachment 626443 [details] [diff] [review]
move export logic into target_export.mk - batch #1

bridge building: moved "Rule to create list of libraries for final link" block out of rules.mk and into a named export- target in config/makefiles/targets_export.mk.

Preserve existing target location within rules.mk for now.
  export:: export-gen-final-lib-link-list

Including target_export.mk will define lib-link-list logic in a new location so avoid breaking existing functionality until it can be asserted the change will not alter behavior.

A simple standalone unit test was added to verify link-list target behavior.  Room for improvement in verifying target behavior but this will be a good start.

Comment 3

[Joey Armstrong [:joey]]

Attachment: move export logic into target_export.mk - batch #1

Comment 4

[Joey Armstrong [:joey]]

Comment on attachment 626455 [details] [diff] [review]
move export logic into target_export.mk - batch #1

same patch as before but include unit test sources this time.

Comment 5

[Joey Armstrong [:joey]]

Try job: https://tbpl.mozilla.org/?tree=Try&rev=4604911d9367

Comment 6

[Joey Armstrong [:joey]]

Attachment: move export logic into target_export.mk - batch #1

Comment 7

[Joey Armstrong [:joey]]

Comment on attachment 628760 [details] [diff] [review]
move export logic into target_export.mk - batch #1

Try job: https://tbpl.mozilla.org/?tree=Try&rev=815040101f48

Refresh patch.
Same patch as before, move more libs targets into the target_libs.mk makefile and setup unit tests to begin verifying behavior.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 628760 [details] [diff] [review]
move export logic into target_export.mk - batch #1

Review of attachment 628760 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/rules.mk
@@ +625,5 @@
>  
>  include $(topsrcdir)/config/makefiles/target_export.mk
>  include $(topsrcdir)/config/makefiles/target_tools.mk
>  
> +export:: export-gen-final-lib-link-list

This should be done in target_export.mk.

Comment 9

[Joey Armstrong [:joey]]

(In reply to Mike Hommey [:glandium] from comment #8)
> Comment on attachment 628760 [details] [diff] [review]
> move export logic into target_export.mk - batch #1
> 
> Review of attachment 628760 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: config/rules.mk
> @@ +625,5 @@
> >  
> >  include $(topsrcdir)/config/makefiles/target_export.mk
> >  include $(topsrcdir)/config/makefiles/target_tools.mk
> >  
> > +export:: export-gen-final-lib-link-list
> 
> This should be done in target_export.mk.

The target was added {at least temporarily until deployment} to preserve the order export targets are being processed in when rules.mk is parsed.

If all of the target logic is migrated in a single pass and problems arise it would not be clear if the logic or target ordering were a factor.

Comment 10

[Joey Armstrong [:joey]]

(In reply to Mike Hommey [:glandium] from comment #8)
> Comment on attachment 628760 [details] [diff] [review]
> move export logic into target_export.mk - batch #1
> 
> Review of attachment 628760 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: config/rules.mk
> @@ +625,5 @@
> >  
> >  include $(topsrcdir)/config/makefiles/target_export.mk
> >  include $(topsrcdir)/config/makefiles/target_tools.mk
> >  
> > +export:: export-gen-final-lib-link-list
> 
> This should be done in target_export.mk.

Would adding a 'todo' comment documenting the transient usage be enough to cover this ?

Comment 11

[Mike Hommey [:glandium]]

(In reply to Joey Armstrong [:joey] from comment #9)
> > >  include $(topsrcdir)/config/makefiles/target_export.mk

The export targets are there, right?

> > >  include $(topsrcdir)/config/makefiles/target_tools.mk

We don't expect export targets to be in that file, right?

> > > +export:: export-gen-final-lib-link-list

Then why can't this be last in target_export.mk, since it just follows it ?

Comment 12

[Joey Armstrong [:joey]]

(In reply to Mike Hommey [:glandium] from comment #11)
> (In reply to Joey Armstrong [:joey] from comment #9)
> > > >  include $(topsrcdir)/config/makefiles/target_export.mk
> 
> The export targets are there, right?
> 
> > > >  include $(topsrcdir)/config/makefiles/target_tools.mk
> 
> We don't expect export targets to be in that file, right?
> 
> > > > +export:: export-gen-final-lib-link-list
> 
> Then why can't this be last in target_export.mk, since it just follows it ?


Well yes we can, this target is a bit of a special case

Comment 13

[Joey Armstrong [:joey]]

(In reply to Mike Hommey [:glandium] from comment #11)
> (In reply to Joey Armstrong [:joey] from comment #9)
> > > >  include $(topsrcdir)/config/makefiles/target_export.mk
> 
> The export targets are there, right?
> 
> > > >  include $(topsrcdir)/config/makefiles/target_tools.mk
> 
> We don't expect export targets to be in that file, right?
> 
> > > > +export:: export-gen-final-lib-link-list
> 
> Then why can't this be last in target_export.mk, since it just follows it ?

Well yes we can, this target was a bit of a special case with the definition near the inclusion so it should be safe.  There are a few other targets in rules.mk that we should use a temporary bridge with until the logic has migrated and settled.

Comment 14

[Joey Armstrong [:joey]]

Inbound push: https://hg.mozilla.org/integration/mozilla-inbound/rev/31fa915e4a92

moved export:: target from rules.mk into makefiles/target_export.mk

Comment 15

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/31fa915e4a92