Closed Bug 1259554 Opened 8 years ago Closed 8 years ago

Remove INSTALL/PP_TARGETS from build/Makefile.in

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: mshal, Assigned: mshal)

References

Details

Attachments

(1 file)

      No description provided.
Depends on: 1252931
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+
(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?)
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.
https://hg.mozilla.org/mozilla-central/rev/4030e1cd45b9
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.