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

RESOLVED FIXED in mozilla16

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: joey, Assigned: joey)

Tracking

unspecified
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

5 years ago
Blocks: 668861
(Assignee)

Comment 1

5 years ago
Created attachment 626443 [details] [diff] [review]
move export logic into target_export.mk - batch #1
(Assignee)

Updated

5 years ago
Assignee: nobody → joey
(Assignee)

Comment 2

5 years ago
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.
Attachment #626443 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 3

5 years ago
Created attachment 626455 [details] [diff] [review]
move export logic into target_export.mk - batch #1
(Assignee)

Updated

5 years ago
Attachment #626443 - Attachment is obsolete: true
Attachment #626443 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 4

5 years ago
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.
Attachment #626455 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

5 years ago
Blocks: 757855
(Assignee)

Updated

5 years ago
No longer blocks: 757855
(Assignee)

Comment 5

5 years ago
Try job: https://tbpl.mozilla.org/?tree=Try&rev=4604911d9367
(Assignee)

Updated

5 years ago
Attachment #626455 - Flags: review?(ted.mielczarek) → review?
(Assignee)

Comment 6

5 years ago
Created attachment 628760 [details] [diff] [review]
move export logic into target_export.mk - batch #1
(Assignee)

Updated

5 years ago
Attachment #626455 - Attachment is obsolete: true
Attachment #626455 - Flags: review?
(Assignee)

Comment 7

5 years ago
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.
Attachment #628760 - Flags: review?(mh+mozilla)
Attachment #628760 - Flags: review?(mh+mozilla) → review+
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.
(Assignee)

Comment 9

5 years ago
(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.
(Assignee)

Comment 10

5 years ago
(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 ?
(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 ?
(Assignee)

Comment 12

5 years ago
(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
(Assignee)

Comment 13

5 years ago
(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.
(Assignee)

Comment 14

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

moved export:: target from rules.mk into makefiles/target_export.mk
https://hg.mozilla.org/mozilla-central/rev/31fa915e4a92
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.