Bug 37701 - [Qt] Target(WebCore,jsc,...) must depends on static library of JavaScriptCore
Summary: [Qt] Target(WebCore,jsc,...) must depends on static library of JavaScriptCore
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Blocker
Assignee: QtWebKit Unassigned
URL:
Keywords: Qt
Depends on:
Blocks: 35784 36590
  Show dependency treegraph
 
Reported: 2010-04-16 00:46 PDT by Csaba Osztrogonác
Modified: 2010-05-16 01:10 PDT (History)
5 users (show)

See Also:


Attachments
proposed fix (1.76 KB, patch)
2010-04-16 00:47 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
proposed fix (1.76 KB, patch)
2010-04-16 05:35 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2010-04-16 00:46:36 PDT
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
Comment 1 Csaba Osztrogonác 2010-04-16 00:47:52 PDT
Created attachment 53521 [details]
proposed fix
Comment 2 Csaba Osztrogonác 2010-04-16 00:49:35 PDT
This unpleasant bug was discovered by my co-worker Peter Varga, thx Peter.
Comment 3 Jocelyn Turcotte 2010-04-16 01:15:57 PDT
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!
Comment 4 Csaba Osztrogonác 2010-04-16 03:06:24 PDT
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!
Comment 5 Csaba Osztrogonác 2010-04-16 03:07:14 PDT
Comment on attachment 53521 [details]
proposed fix

r? removed until fix msvc case
Comment 6 Csaba Osztrogonác 2010-04-16 05:35:22 PDT
Created attachment 53527 [details]
proposed fix

MSVC case fixed following Jocelyn's comment and tested.
Comment 7 Simon Hausmann 2010-04-16 06:30:09 PDT
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?
Comment 8 Csaba Osztrogonác 2010-04-16 07:04:53 PDT
(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?
Comment 9 Simon Hausmann 2010-04-16 13:42:17 PDT
(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.
Comment 10 Csaba Osztrogonác 2010-04-20 04:27:11 PDT
Landed in http://trac.webkit.org/changeset/57882
Comment 11 Ismail Donmez 2010-04-20 04:47:13 PDT
This breaks compilation Windows CE;

NMAKE : fatal error U1073: don't know how to make '..\JavaScriptCore\release\libjscore.a'
Comment 12 Csaba Osztrogonác 2010-04-20 04:50:49 PDT
(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?
Comment 13 Ismail Donmez 2010-04-20 04:51:54 PDT
(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.
Comment 14 Csaba Osztrogonác 2010-04-20 04:58:00 PDT
> 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?
Comment 15 Ismail Donmez 2010-04-20 04:59:32 PDT
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!
Comment 16 Csaba Osztrogonác 2010-04-20 05:49:42 PDT
I hope I fixed it: http://trac.webkit.org/changeset/57885

Ismail, could you confirm if it works?
Comment 17 Simon Hausmann 2010-04-20 07:03:41 PDT
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.
Comment 18 Ismail Donmez 2010-04-20 07:08:02 PDT
(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.
Comment 19 Csaba Osztrogonác 2010-04-20 07:14:12 PDT
(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?
Comment 20 Csaba Osztrogonác 2010-04-20 07:16:08 PDT
(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.
Comment 21 Jocelyn Turcotte 2010-04-20 10:06:35 PDT
(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.
Comment 22 Simon Hausmann 2010-04-20 11:11:39 PDT
(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.
Comment 23 Simon Hausmann 2010-05-14 09:06:55 PDT
P2, not a strong blocker.
Comment 24 Csaba Osztrogonác 2010-05-14 09:14:02 PDT
(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.
Comment 25 Simon Hausmann 2010-05-16 01:05:53 PDT
<cherry-pick-for-backport: r57882>
Comment 26 Simon Hausmann 2010-05-16 01:06:11 PDT
<cherry-pick-for-backport: r57885>
Comment 27 Simon Hausmann 2010-05-16 01:10:13 PDT
Revision r57882 cherry-picked into qtwebkit-2.0 with commit 99054d09cb5df235575f572848e109bfa8dc819d
Revision r57885 cherry-picked into qtwebkit-2.0 with commit cacbdf18fc917122834042d77a9164a490aafde4