Don't relink libxul.so when nothing changed and building against native libevent

RESOLVED FIXED in mozilla33

Status

()

Core
Build Config
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: gaston, Assigned: gaston)

Tracking

Trunk
mozilla33
Other
OpenBSD
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

6 years ago
In bug #558789 was added the possibility of building against a system libevent, which i'm using on OpenBSD (since building with the bundled libevent fails on missing sys/epoll.h which is linux only).

I noticed that even if nothing in the source tree was touched, gmake -f client.mk relinks libxul.so because message_pump_libevent.cc is rebuilt. Looking at the deps of it, .deps/message_pump_libevent.pp is regenerated, probably because dist/third_party/libevent/event.h is recreated (see bottom of ipc/chromium/Makefile.in) regardless if it already exists or not.

As relinking libxul.so takes a hell of a time(tm), it would be nice if we avoided it. Will try a fix (ie test for dist/third_party/libevent/event.h presence before overwriting it)

Comment 1

6 years ago
I doubt you will solve the libxul relinking problem by fixing just this one dependency. One of the first things the build system does is blow away a bunch of dist/, including include/. This invalidates a lot of make targets. Sad, but true.

I'd vote for morphing this bug to just support building against native libevent.
You can probably try to get rid of dist/third_party/libevent/event.h altogether.
(In reply to Gregory Szorc [:gps] from comment #1)
> I doubt you will solve the libxul relinking problem by fixing just this one
> dependency. One of the first things the build system does is blow away a
> bunch of dist/, including include/. This invalidates a lot of make targets.
> Sad, but true.

Not on unix, actually, because all files in dist/include are symbolic links.

> I'd vote for morphing this bug to just support building against native
> libevent.

That's what he is using already. Bug 558789 did that.

Comment 4

6 years ago
(In reply to Mike Hommey [:glandium] from comment #3)
> (In reply to Gregory Szorc [:gps] from comment #1)
> > I doubt you will solve the libxul relinking problem by fixing just this one
> > dependency. One of the first things the build system does is blow away a
> > bunch of dist/, including include/. This invalidates a lot of make targets.
> > Sad, but true.
> 
> Not on unix, actually, because all files in dist/include are symbolic links.

Oh, right! What about the build id? Isn't there something that changes with every build that makes its way (via header inclusion) into libxul or the application binary?
(In reply to Gregory Szorc [:gps] from comment #4)
> (In reply to Mike Hommey [:glandium] from comment #3)
> > (In reply to Gregory Szorc [:gps] from comment #1)
> > > I doubt you will solve the libxul relinking problem by fixing just this one
> > > dependency. One of the first things the build system does is blow away a
> > > bunch of dist/, including include/. This invalidates a lot of make targets.
> > > Sad, but true.
> > 
> > Not on unix, actually, because all files in dist/include are symbolic links.
> 
> Oh, right! What about the build id? Isn't there something that changes with
> every build that makes its way (via header inclusion) into libxul or the
> application binary?

No, that is built in the application binary.
(In reply to Gregory Szorc [:gps] from comment #4)
> Oh, right! What about the build id? Isn't there something that changes with
> every build that makes its way (via header inclusion) into libxul or the
> application binary?

(Note, that used to be true in android builds, but that was fixed)
(Assignee)

Comment 7

6 years ago
(In reply to Mike Hommey [:glandium] from comment #2)
> You can probably try to get rid of dist/third_party/libevent/event.h
> altogether.

What do you mean ? message_pump_libevent.cc incontionally includes it..
#include "third_party/libevent/event.h"
(In reply to Landry Breuil (:gaston) from comment #7)
> (In reply to Mike Hommey [:glandium] from comment #2)
> > You can probably try to get rid of dist/third_party/libevent/event.h
> > altogether.
> 
> What do you mean ? message_pump_libevent.cc incontionally includes it..
> #include "third_party/libevent/event.h"

That can be changed.
(Assignee)

Comment 9

6 years ago
Created attachment 630149 [details] [diff] [review]
Only generate third_party/libevent/event.h if needed

The attached patch fixes the message_pump_libevent.cc rebuild, but libxul.so is still wholly relinked although no cpp file seems rebuilt.
(Assignee)

Comment 10

6 years ago
Created attachment 630150 [details]
build log when no file seems rebuilt but libxul is relinked
(Assignee)

Comment 11

6 years ago
in a populated objdir after libxul.so was linked, rerunning gmake -d libxul.so doesnt relink it, so it's one of the other targets run before it that probably updates one of the dependencies and triggers the rebuild. I see nss and xpcom being inconditionally rebuilt too.
(Assignee)

Comment 12

4 years ago
Comment on attachment 630149 [details] [diff] [review]
Only generate third_party/libevent/event.h if needed

Stumbled upon it by accident while looking for other things, do we still want this ? seems to do what it's supposed to, but given the build system changes that occured in the past months/years...
Attachment #630149 - Flags: review?(mh+mozilla)
Attachment #630149 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 13

4 years ago
And after actually updating it to m-c (putting it into libxul.mk), and testing a no-change mach build after a successful first build, it doesnt relink libxul, so yay!

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd1a690a248d
https://hg.mozilla.org/mozilla-central/rev/cd1a690a248d
Assignee: nobody → landry
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.