Remove INSTALL/PP_TARGETS from build/Makefile.in

RESOLVED FIXED in Firefox 48

Status

()

Core
Build Config
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mshal, Assigned: mshal)

Tracking

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Depends on: 1252931
(Assignee)

Comment 1

2 years ago
Created attachment 8734500 [details]
MozReview Request: Bug 1259554 - Remove INSTALL/PP_TARGETS from build/Makefile.in; r?ted

The lone remaining INSTALL_TARGETS is for moztt, which relies on
including a Makefile from that repository. We could potentially add a
mozbuild file there instead.

Review commit: https://reviewboard.mozilla.org/r/42289/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42289/
Attachment #8734500 - Flags: review?(ted)
Comment on attachment 8734500 [details]
MozReview Request: Bug 1259554 - Remove INSTALL/PP_TARGETS from build/Makefile.in; r?ted

https://reviewboard.mozilla.org/r/42289/#review39737

::: build/moz.build:84
(Diff revision 1)
> +# automatically by LLDB when we debug executables using either of those two
> +# directories as the current working directory.  The .lldbinit file will
> +# load $(topsrcdir)/.lldbinit, which is where the actual debugging commands are.
> +DEFINES['topsrcdir'] = TOPSRCDIR
> +FINAL_TARGET_PP_FILES += ['.lldbinit.in']
> +OBJDIR_FILES += ['!/dist/bin/.lldbinit']

This is a very weird construct! The point here is to copy the preprocessed file to the current directory?
Attachment #8734500 - Flags: review?(ted) → review+
(Assignee)

Comment 3

2 years ago
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #2)
> Comment on attachment 8734500 [details]
> MozReview Request: Bug 1259554 - Remove INSTALL/PP_TARGETS from
> build/Makefile.in; r?ted
> 
> https://reviewboard.mozilla.org/r/42289/#review39737
> 
> ::: build/moz.build:84
> (Diff revision 1)
> > +# automatically by LLDB when we debug executables using either of those two
> > +# directories as the current working directory.  The .lldbinit file will
> > +# load $(topsrcdir)/.lldbinit, which is where the actual debugging commands are.
> > +DEFINES['topsrcdir'] = TOPSRCDIR
> > +FINAL_TARGET_PP_FILES += ['.lldbinit.in']
> > +OBJDIR_FILES += ['!/dist/bin/.lldbinit']
> 
> This is a very weird construct! The point here is to copy the preprocessed
> file to the current directory?

Yeah, it is a bit weird. We need to get a copy of the preprocessed .lldbinit into $(DEPTH) and $(FINAL_TARGET). We could also go with something like:

OBJDIR_PP_FILES += ['.lldbinit.in']
FINAL_TARGET_FILES += ['!/.lldbinit']

The OBJDIR_* variables are hierarchical like FINAL_TARGET, but start at DEPTH. Technically they can put anything anywhere, but it's just intended for this random one-off cases that don't fit anything else we do in the build system.
I don't know that either of those is really better than the other, but I might have a slight preference to not copying it back out of dist/bin. (Seems somewhat more correct to generate it into the current directory and then copy it from there?)
(Assignee)

Comment 5

2 years ago
Actually, it seems doing it that way conflicts with the faster make backend. I don't think that's worth sorting out now, but feel free to file a bug for it.

Comment 6

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4030e1cd45b9

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4030e1cd45b9
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.