Bug 37701

Created attachment 53521 [details] proposed fix

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

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!

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 on attachment 53521 [details] proposed fix r? removed until fix msvc case

Created attachment 53527 [details] proposed fix MSVC case fixed following Jocelyn's comment and tested.

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?

(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?

(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.

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

This breaks compilation Windows CE; NMAKE : fatal error U1073: don't know how to make '..\JavaScriptCore\release\libjscore.a'

(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?

(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.

> 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?

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!

I hope I fixed it: http://trac.webkit.org/changeset/57885 Ismail, could you confirm if it works?

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.

(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.

(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?

(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.

(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.

(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.

P2, not a strong blocker.

(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.

<cherry-pick-for-backport: r57882>

<cherry-pick-for-backport: r57885>

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