Last Comment Bug 757828 - makefiles: move export logic into target_export.mk - batch #1
: makefiles: move export logic into target_export.mk - batch #1
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Joey Armstrong [:joey]
:
Mentors:
Depends on:
Blocks: 668861
  Show dependency treegraph
 
Reported: 2012-05-23 06:47 PDT by Joey Armstrong [:joey]
Modified: 2012-06-12 18:32 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
move export logic into target_export.mk - batch #1 (11.14 KB, patch)
2012-05-23 08:02 PDT, Joey Armstrong [:joey]
no flags Details | Diff | Splinter Review
move export logic into target_export.mk - batch #1 (11.18 KB, patch)
2012-05-23 08:19 PDT, Joey Armstrong [:joey]
no flags Details | Diff | Splinter Review
move export logic into target_export.mk - batch #1 (6.72 KB, patch)
2012-05-31 08:39 PDT, Joey Armstrong [:joey]
mh+mozilla: review+
Details | Diff | Splinter Review

Description Joey Armstrong [:joey] 2012-05-23 06:47:10 PDT

    
Comment 1 Joey Armstrong [:joey] 2012-05-23 08:02:10 PDT
Created attachment 626443 [details] [diff] [review]
move export logic into target_export.mk - batch #1
Comment 2 Joey Armstrong [:joey] 2012-05-23 08:18:22 PDT
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] 2012-05-23 08:19:38 PDT
Created attachment 626455 [details] [diff] [review]
move export logic into target_export.mk - batch #1
Comment 4 Joey Armstrong [:joey] 2012-05-23 08:21:00 PDT
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] 2012-05-24 06:42:12 PDT
Try job: https://tbpl.mozilla.org/?tree=Try&rev=4604911d9367
Comment 6 Joey Armstrong [:joey] 2012-05-31 08:39:50 PDT
Created attachment 628760 [details] [diff] [review]
move export logic into target_export.mk - batch #1
Comment 7 Joey Armstrong [:joey] 2012-05-31 08:42:35 PDT
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] 2012-06-07 05:07:49 PDT
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] 2012-06-12 07:04:30 PDT
(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] 2012-06-12 07:07:52 PDT
(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] 2012-06-12 07:10:42 PDT
(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] 2012-06-12 07:28:47 PDT
(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] 2012-06-12 07:31:05 PDT
(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] 2012-06-12 10:28:54 PDT
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) 2012-06-12 18:32:31 PDT
https://hg.mozilla.org/mozilla-central/rev/31fa915e4a92

Note You need to log in before you can comment on or make changes to this bug.