Bug 32680 - [Qt] webkitdirs.pm function builtDylibPathForName returns wrong path to library
Summary: [Qt] webkitdirs.pm function builtDylibPathForName returns wrong path to library
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
Depends on:
Blocks: 32683
  Show dependency treegraph
 
Reported: 2009-12-17 15:30 PST by Daniel Bates
Modified: 2009-12-22 23:55 PST (History)
9 users (show)

See Also:


Attachments
Patch (1.76 KB, patch)
2009-12-17 16:06 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (1.83 KB, patch)
2009-12-18 09:37 PST, Daniel Bates
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2009-12-17 15:30:06 PST
The function builtDylibPathForName returns a path with respect to the file named QtWebKit.dll. But this files does not exist. Instead, the file is named QtWebKit4.dll.
Comment 1 Daniel Bates 2009-12-17 16:06:33 PST
Created attachment 45107 [details]
Patch
Comment 2 WebKit Review Bot 2009-12-17 16:09:25 PST
style-queue ran check-webkit-style on attachment 45107 [details] without any errors.
Comment 3 Zoltan Horvath 2009-12-18 00:27:01 PST
I think we should put either a new if isQt() inside isWindows() or elsif with isQt() && isWindows() for better readability.

Btw, Simon why do we use other lib name on Windows?
Comment 4 Simon Hausmann 2009-12-18 02:49:28 PST
(In reply to comment #3)
> I think we should put either a new if isQt() inside isWindows() or elsif with
> isQt() && isWindows() for better readability.
> 
> Btw, Simon why do we use other lib name on Windows?

I agree, this needs an isQt() somewhere :)

It is a convention in Qt to add the major number to the name of the DLL, similar to how the major version on Linux for example is part of the name and respected by the dynamic linker. An identical major version number indicates binary compatibility towards newer versions.
Comment 5 Daniel Bates 2009-12-18 07:43:15 PST
Oops. Yeah, I forgot to add an if isQt(). Will update patch.

(In reply to comment #4)
> (In reply to comment #3)
> > I think we should put either a new if isQt() inside isWindows() or elsif with
> > isQt() && isWindows() for better readability.
> > 
> > Btw, Simon why do we use other lib name on Windows?
> 
> I agree, this needs an isQt() somewhere :)
> 
> It is a convention in Qt to add the major number to the name of the DLL,
> similar to how the major version on Linux for example is part of the name and
> respected by the dynamic linker. An identical major version number indicates
> binary compatibility towards newer versions.
Comment 6 Daniel Bates 2009-12-18 07:54:39 PST
Never mind, there is already an outer "if (isQt())", <http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitdirs.pm?rev=51932#L528>. Hence, we add the QT_MAJOR_VERSION to the DLL name if we are using Qt and are on the Windows or Cygwin platform.

So, Simon, Zoltan, are you asking to fold in this outer "if (isQt())" into the inner if, elsif, else block?

(In reply to comment #5)
> Oops. Yeah, I forgot to add an if isQt(). Will update patch.
Comment 7 Zoltan Horvath 2009-12-18 09:28:24 PST
(In reply to comment #6)
> Never mind, there is already an outer "if (isQt())",
> <http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitdirs.pm?rev=51932#L528>.
> Hence, we add the QT_MAJOR_VERSION to the DLL name if we are using Qt and are
> on the Windows or Cygwin platform.
> 
> So, Simon, Zoltan, are you asking to fold in this outer "if (isQt())" into the
> inner if, elsif, else block?

You're right. Why there is a isCygwin(), normally you compile Qt with MinGW... Please remove isCygwin() and it will be okay in that block.
Comment 8 Daniel Bates 2009-12-18 09:37:22 PST
Created attachment 45158 [details]
Patch

Updated patch based on Zoltan's comment.
Comment 9 WebKit Review Bot 2009-12-18 09:39:46 PST
style-queue ran check-webkit-style on attachment 45158 [details] without any errors.
Comment 10 Eric Seidel (no email) 2009-12-19 12:26:42 PST
Comment on attachment 45158 [details]
Patch

I'm confused.  Why wouldn't this break other windows ports?
Comment 11 Daniel Bates 2009-12-19 12:33:25 PST
We only do this for the Qt Windows port. Notice, the outer "if isQt()" on line 528 <http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitdirs.pm?rev=51932#L528>.

Hence, this will not interfere with the other Windows ports.

(In reply to comment #10)
> (From update of attachment 45158 [details])
> I'm confused.  Why wouldn't this break other windows ports?
Comment 12 Eric Seidel (no email) 2009-12-20 23:06:52 PST
Comment on attachment 45158 [details]
Patch

I'm going to trust you that this is sane.  I know next to nothing about the Qt build, but I expect that you do given your recent job.  If this is wrong for Qt we can always roll it out.  rs=me
Comment 13 Daniel Bates 2009-12-22 23:55:24 PST
Committed in <http://trac.webkit.org/changeset/52522>.