Bug 37701 – [Qt] Target(WebCore,jsc,...) must depends on static library of JavaScriptCore

• Status: CLOSED
• Resolution: FIXED
• Component: Tools / Tests

[Csaba Osztrogonác]

From http://trac.webkit.org/changeset/56623 JavaScriptCore 
is builded as static library. But unfortunately libQtWebKit.so, 
jsc, QtLauncher,... don't depends on libjscore.a

[Csaba Osztrogonác]

Created attachment 53521 [details]
proposed fix

[Csaba Osztrogonác]

This unpleasant bug was discovered by my co-worker Peter Varga, thx Peter.

[Jocelyn Turcotte]

On MSVC the library name should be "$${JAVASCRIPTCORE_TARGET}.lib" instead of "lib$${JAVASCRIPTCORE_TARGET}.a"

If you tested it and it works because of some qmake behind-the-scene magic then just forget my comment.

The rest looks great to me!

[Csaba Osztrogonác]

Ooops, I didn't test it with MSVC, I'll try it in an hour
following your advice.

Laszlo, could you check if the symbian case is correct?


(In reply to comment #3)
> On MSVC the library name should be "$${JAVASCRIPTCORE_TARGET}.lib" instead of
> "lib$${JAVASCRIPTCORE_TARGET}.a"
> 
> If you tested it and it works because of some qmake behind-the-scene magic then
> just forget my comment.
> 
> The rest looks great to me!

[Csaba Osztrogonác]

Comment on attachment 53521 [details]
proposed fix

r? removed until fix msvc case

[Csaba Osztrogonác]

Created attachment 53527 [details]
proposed fix

MSVC case fixed following Jocelyn's comment and tested.

[Simon Hausmann]

Ossy, can you explain _why_ this works? :)

From what I can see this adds a dependency for the target rule:

WebCoreLibrary: <the objects to for linking> ../path/to/JSC.a
    <link webcore command>

So it seems that with POST_TARGETDEPS we get that additional dependency at the end. According to grep that's the case for the mingw/nmake and unix makefile generators. For these generators there's the toplevel Makefile that first calls into JavaScriptCore and then into WebCore's Makefile.

If the JSC.a exists at the time WebCore's Makefile is called, then everything will be fine. But if it does _not_ exist, then I don't see how anything is fixed with the proposed patch, given that there's no _rule_ in WebCore's Makefile to actually generate the JSC.a.

I must be missing something, please enlighten me :)

Otherwise the patch looks fine, although I don't see POST_TARGETDEPS being used by the Symbian sbsv1/v2 generators. But then again, it'll be used by the symbian makefile based builds.

Ossy, what's the actual problem you're hitting?

[Csaba Osztrogonác]

(In reply to comment #7)
> Ossy, can you explain _why_ this works? :)

I forgot to mention why it works. :)

The answer is simple. Because in Webkit.pro JavaScriptCore subdir 
is in the first element of SUBDIRS, so the Makefile generated from
JavaScriptCore.pro contain the build rules for libjscore.a (or 
jscore.lib in Symbian and MSVC case).

Our problem isn't the missing libjscore.a building rules, but
missing dependency I mentioned before:

TARGET = libQtWebKit.so.4.6.0
../lib/$(TARGET):  $(OBJECTS) $(SUBLIBS) $(OBJCOMP)  ../JavaScriptCore/libjscore.a

It is useful for incremental builds. If something changed in libjscore.a,
jsc binary, libQtWebKit.so _must_ be link again with the new libjscore.a.
Now if you change something in a cpp of JavaScriptCore, libQtWebKit.so 
and jsc binary won't be linked again, but they should.

> If the JSC.a exists at the time WebCore's Makefile is called, then everything
> will be fine. But if it does _not_ exist, then I don't see how anything is
> fixed with the proposed patch, given that there's no _rule_ in WebCore's
> Makefile to actually generate the JSC.a.
Absolutely correct. If the developer don't use the recommended WebKitTools/Scripts/build-webkit script, libjscore.a may not be
exist before compiling WebCore. I don't think we should handle
this case. :)

> Otherwise the patch looks fine, although I don't see POST_TARGETDEPS being used
> by the Symbian sbsv1/v2 generators. But then again, it'll be used by the
> symbian makefile based builds.

Unfortunately I don't know anything about Symbian build system.
Is it possible that it has same problem? Does this patch help
for Symbian incremental build?

[Simon Hausmann]

(In reply to comment #8)
> (In reply to comment #7)
> > Ossy, can you explain _why_ this works? :)
> 
> I forgot to mention why it works. :)
> 
> The answer is simple. Because in Webkit.pro JavaScriptCore subdir 
> is in the first element of SUBDIRS, so the Makefile generated from
> JavaScriptCore.pro contain the build rules for libjscore.a (or 
> jscore.lib in Symbian and MSVC case).
> 
> Our problem isn't the missing libjscore.a building rules, but
> missing dependency I mentioned before:
> 
> TARGET = libQtWebKit.so.4.6.0
> ../lib/$(TARGET):  $(OBJECTS) $(SUBLIBS) $(OBJCOMP) 
> ../JavaScriptCore/libjscore.a
> 
> It is useful for incremental builds. If something changed in libjscore.a,
> jsc binary, libQtWebKit.so _must_ be link again with the new libjscore.a.
> Now if you change something in a cpp of JavaScriptCore, libQtWebKit.so 
> and jsc binary won't be linked again, but they should.

Aha! Thanks for the explanation, that makes sense.

> > If the JSC.a exists at the time WebCore's Makefile is called, then everything
> > will be fine. But if it does _not_ exist, then I don't see how anything is
> > fixed with the proposed patch, given that there's no _rule_ in WebCore's
> > Makefile to actually generate the JSC.a.
> Absolutely correct. If the developer don't use the recommended
> WebKitTools/Scripts/build-webkit script, libjscore.a may not be
> exist before compiling WebCore. I don't think we should handle
> this case. :)
> 
> > Otherwise the patch looks fine, although I don't see POST_TARGETDEPS being used
> > by the Symbian sbsv1/v2 generators. But then again, it'll be used by the
> > symbian makefile based builds.
> 
> Unfortunately I don't know anything about Symbian build system.
> Is it possible that it has same problem? Does this patch help
> for Symbian incremental build?

It might not help Symbian builds directly, the sbs based builds have their own dependency tracking that is out of our hands anyway.

[Csaba Osztrogonác]

Landed in http://trac.webkit.org/changeset/57882

[Ismail Donmez]

This breaks compilation Windows CE;

NMAKE : fatal error U1073: don't know how to make '..\JavaScriptCore\release\libjscore.a'

[Csaba Osztrogonác]

(In reply to comment #11)
> This breaks compilation Windows CE;
> 
> NMAKE : fatal error U1073: don't know how to make
> '..\JavaScriptCore\release\libjscore.a'
Could we talk on #qtwebkit or on #webkit? What is your nick?

[Ismail Donmez]

(In reply to comment #12)
> (In reply to comment #11)
> > This breaks compilation Windows CE;
> > 
> > NMAKE : fatal error U1073: don't know how to make
> > '..\JavaScriptCore\release\libjscore.a'
> Could we talk on #qtwebkit or on #webkit? What is your nick?

cartman, when I am around. Currently I can't IRC.

[Csaba Osztrogonác]

> cartman, when I am around. Currently I can't IRC.
Do you build with build-webkit script?

MakeFile generated from WebKit.pro should contain the
rule to make '..\JavaScriptCore\release\libjscore.a'
or anything similar to this.

Could you tell me if there is any jscore lib in your build dir?

[Ismail Donmez]

I had to revert to an earlier commit (because I need a working build atm.), I will re-report when I try trunk again. Thanks!

[Csaba Osztrogonác]

I hope I fixed it: http://trac.webkit.org/changeset/57885

Ismail, could you confirm if it works?

[Simon Hausmann]

Hmm, is this really a release blocker?

AFAICS this is useful for incremental builds, in particular the bot. If you guys agree I'd like to remove the blocking dependency.

[Ismail Donmez]

(In reply to comment #16)
> I hope I fixed it: http://trac.webkit.org/changeset/57885
> 
> Ismail, could you confirm if it works?

This seems to fix the build error, thank you.

[Csaba Osztrogonác]

(In reply to comment #17)
> Hmm, is this really a release blocker?
> 
> AFAICS this is useful for incremental builds, in particular the bot. If you
> guys agree I'd like to remove the blocking dependency.

I don't think so if it should be a release blocker. Jocelyn?

[Csaba Osztrogonác]

(In reply to comment #18)
> This seems to fix the build error, thank you.
Cool. So we can close this bug after you decided if it is a release blocker bug or not.

[Jocelyn Turcotte]

(In reply to comment #19)
> (In reply to comment #17)
> > Hmm, is this really a release blocker?
> > 
> > AFAICS this is useful for incremental builds, in particular the bot. If you
> > guys agree I'd like to remove the blocking dependency.
> 
> I don't think so if it should be a release blocker. Jocelyn?

People would probably wouldn't mind since they will probably use release tarballs and do a clean rebuild anyway.

If we don't get that patch in the release branch, the bot that tests qtwebkit-2.0 won't test JavaScriptCore's changes until a following commit touch one of WebCore's cpp file. If you didn't see this happen yet I guess we can leave it out, else I guess its up to you.

[Simon Hausmann]

(In reply to comment #21)
> If we don't get that patch in the release branch, the bot that tests
> qtwebkit-2.0 won't test JavaScriptCore's changes until a following commit touch
> one of WebCore's cpp file. If you didn't see this happen yet I guess we can
> leave it out, else I guess its up to you.

Very convincing point :). Ok, kept as blocker.

[Simon Hausmann]

P2, not a strong blocker.

[Csaba Osztrogonác]

(In reply to comment #23)
> P2, not a strong blocker.

(It was reopened based on comment of Ismail "cartman" Donmez.)

The patch is landed a long time ago. Is there any reason why
this bug is still open? I think we can close it with resolved/fixed.

[Simon Hausmann]

<cherry-pick-for-backport: r57882>

[Simon Hausmann]

<cherry-pick-for-backport: r57885>

[Simon Hausmann]

Revision r57882 cherry-picked into qtwebkit-2.0 with commit 99054d09cb5df235575f572848e109bfa8dc819d
Revision r57885 cherry-picked into qtwebkit-2.0 with commit cacbdf18fc917122834042d77a9164a490aafde4