Bug 64734 - [Qt] Out-of-tree build using make-package.py is broken
Summary: [Qt] Out-of-tree build using make-package.py is broken
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P3 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 68616
  Show dependency treegraph
 
Reported: 2011-07-18 10:56 PDT by Heinz Wiesinger
Modified: 2013-11-22 08:53 PST (History)
7 users (show)

See Also:


Attachments
Patch (5.38 KB, patch)
2011-07-18 10:59 PDT, Heinz Wiesinger
no flags Details | Formatted Diff | Diff
Patch (5.71 KB, patch)
2011-07-18 11:22 PDT, Heinz Wiesinger
no flags Details | Formatted Diff | Diff
Patch (5.67 KB, patch)
2011-07-18 11:25 PDT, Heinz Wiesinger
no flags Details | Formatted Diff | Diff
updated patch (11.52 KB, patch)
2011-09-28 08:31 PDT, Heinz Wiesinger
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Heinz Wiesinger 2011-07-18 10:56:05 PDT
[Qt] Out-of-tree build using make-package.py is broken
Comment 1 Heinz Wiesinger 2011-07-18 10:59:47 PDT
Created attachment 101175 [details]
Patch
Comment 2 Benjamin Poulain 2011-07-18 11:04:37 PDT
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.
Comment 3 Benjamin Poulain 2011-07-18 11:16:34 PDT
Adding people in CC who know way more than me about this build process madness :)
Comment 4 Heinz Wiesinger 2011-07-18 11:22:55 PDT
Created attachment 101178 [details]
Patch
Comment 5 Alexis Menard (darktears) 2011-07-18 11:24:31 PDT
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.
Comment 6 Alexis Menard (darktears) 2011-07-18 11:24:53 PDT
(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.
Comment 7 Heinz Wiesinger 2011-07-18 11:25:43 PDT
Created attachment 101180 [details]
Patch
Comment 8 Heinz Wiesinger 2011-07-18 11:29:30 PDT
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.
Comment 9 Heinz Wiesinger 2011-07-18 11:33:37 PDT
(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
Comment 10 Alexis Menard (darktears) 2011-08-01 10:52:34 PDT
(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?
Comment 11 Heinz Wiesinger 2011-09-28 08:31:59 PDT
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
Comment 12 Heinz Wiesinger 2011-09-28 08:34:35 PDT
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.
Comment 13 Ademar Reis 2011-09-28 09:06:12 PDT
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.
Comment 14 Heinz Wiesinger 2011-09-28 12:17:56 PDT
(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.
Comment 15 Allan Sandfeld Jensen 2012-04-30 02:06:05 PDT
If this is still an open bug, I think the patch should be rebased to requested for review and commit?
Comment 16 Heinz Wiesinger 2012-05-09 02:32:00 PDT
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.
Comment 17 Allan Sandfeld Jensen 2013-11-22 08:53:35 PST
Not relevant any more