[Qt] Out-of-tree build using make-package.py is broken

• Component: New Bugs
• Status: RESOLVED INVALID

[Heinz Wiesinger]

[Qt] Out-of-tree build using make-package.py is broken

[Heinz Wiesinger]

Created attachment 101175 [details]
Patch

[Benjamin Poulain]

Comment on attachment 101175 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101175&action=review

You still need a small description.

The point is that someone should be able to understand why some code is like it is. This is valuable for the reviewer, and for developers in the future who will modify that code.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

You should remove this line in this case.

[Benjamin Poulain]

Adding people in CC who know way more than me about this build process madness :)

[Heinz Wiesinger]

Created attachment 101178 [details]
Patch

[Alexis Menard (darktears)]

Comment on attachment 101175 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101175&action=review

This patch was tested outside the make-package case? Normal build with build-webkit --qt --release and WEBKIT_OUTPUT_DIR=/my/path build-webkit --qt --release. What about inside Qt?

> Source/WebKit/qt/Api/DerivedSources.pro:47
> +    CONFIG(standalone_package): PATH_TO_HEADER = ../../Source/WebKit/qt/Api/$$basename(HEADER)

As I said on IRC, I don't think this is a good idea. How this will work with let say shadow build.

[Alexis Menard (darktears)]

(In reply to comment #5)
> (From update of attachment 101175 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101175&action=review
> 
> This patch was tested outside the make-package case? Normal build with build-webkit --qt --release and WEBKIT_OUTPUT_DIR=/my/path build-webkit --qt --release. What about inside Qt?
> 
> > Source/WebKit/qt/Api/DerivedSources.pro:47
> > +    CONFIG(standalone_package): PATH_TO_HEADER = ../../Source/WebKit/qt/Api/$$basename(HEADER)
> 
> As I said on IRC, I don't think this is a good idea. How this will work with let say shadow build.

WEBKITOUTPUTDIR sorry.

[Heinz Wiesinger]

Created attachment 101180 [details]
Patch

[Heinz Wiesinger]

I tested this patch with a standalone tarball (generated by make-package.py, after applying the patch) as well as an internal build using build-webkit --qt --release.
Didn't test anything else yet, but if required I can do so.

[Heinz Wiesinger]

(In reply to comment #5)
> (From update of attachment 101175 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101175&action=review
> 
> This patch was tested outside the make-package case? Normal build with build-webkit --qt --release and WEBKIT_OUTPUT_DIR=/my/path build-webkit --qt --release. What about inside Qt?
> 
> > Source/WebKit/qt/Api/DerivedSources.pro:47
> > +    CONFIG(standalone_package): PATH_TO_HEADER = ../../Source/WebKit/qt/Api/$$basename(HEADER)
> 
> As I said on IRC, I don't think this is a good idea. How this will work with let say shadow build.

No, that was a different issue (or not?). This change doesn't move the generated headers at all. It just changes the content of those headers to refer to an existing path. Currently they'd point to "../../WebKit" which would be a root-level WebKit folder, which doesn't exist after running make-package.py

[Alexis Menard (darktears)]

(In reply to comment #9)
> (In reply to comment #5)
> > (From update of attachment 101175 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=101175&action=review
> > 
> > This patch was tested outside the make-package case? Normal build with build-webkit --qt --release and WEBKIT_OUTPUT_DIR=/my/path build-webkit --qt --release. What about inside Qt?
> > 
> > > Source/WebKit/qt/Api/DerivedSources.pro:47
> > > +    CONFIG(standalone_package): PATH_TO_HEADER = ../../Source/WebKit/qt/Api/$$basename(HEADER)
> > 
> > As I said on IRC, I don't think this is a good idea. How this will work with let say shadow build.
> 
> No, that was a different issue (or not?). This change doesn't move the generated headers at all. It just changes the content of those headers to refer to an existing path. Currently they'd point to "../../WebKit" which would be a root-level WebKit folder, which doesn't exist after running make-package.py

Could you apply it on the 4.8 branch and test to see if it doesn't break anything?

[Heinz Wiesinger]

Created attachment 109022 [details]
updated patch

Updated patch, fixes tests to link to the just built libQtWebKit, rather than using the system wide installed one

[Heinz Wiesinger]

I've tested this now on various configurations:
 - external build against Qt 4.7
 - internal build for Qt 4.7
 - external build against Qt 4.8
 - internal build for Qt 4.8
 - build-webkit against Qt 4.7

It works fine for me in all of the above cases.

[Ademar Reis]

Thanks for catching this!

Unfortunately 2.2.0 is virtually out already (everything but the public announcement is ready, it's with the marketing department), but this fix will be included in 2.2.1.

It's an important bug, but not critical enough to make us cancel the release: my understanding is that building the whole Qt package with a replaced QtWebKit works fine (standalone_package = false) and so does the build with build-webkit --qt.

BTW, Qt-4.8.0 may include QtWebKit-2.2.0, QtWebKit-2.2.1 or even QtWebKit-2.2.0 + a couple of patches. It'll be up to them.

[Heinz Wiesinger]

(In reply to comment #13)
> Unfortunately 2.2.0 is virtually out already (everything but the public announcement is ready, it's with the marketing department), but this fix will be included in 2.2.1.

That's great news :)

> It's an important bug, but not critical enough to make us cancel the release: my understanding is that building the whole Qt package with a replaced QtWebKit works fine (standalone_package = false) and so does the build with build-webkit --qt.

Yes, of course. This bug is in no way a showstopper as it is only affecting an edge case. 2.2.1 is totally sufficient for that.
I'd suppose the only people who stumble over this are packagers who want to ship it together with Qt 4.7.x, and those can just as easily be pointed to this bugreport/patch until 2.2.1 is out.

[Allan Sandfeld Jensen]

If this is still an open bug, I think the patch should be rebased to requested for review and commit?

[Heinz Wiesinger]

I can rebase the patch against latest QtWebKit 2.2, but I wonder how much use it still is.
Will there be another release of the 2.2 branch?
What about Qt5? I can imagine the patch is fully obsolete there, but I haven't looked at the modularization yet.

[Allan Sandfeld Jensen]

Not relevant any more