Bug 32680 – [Qt] webkitdirs.pm function builtDylibPathForName returns wrong path to library

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Daniel Bates]

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.

[Daniel Bates]

Created attachment 45107 [details]
Patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45107 [details] without any errors.

[Zoltan Horvath]

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?

[Simon Hausmann]

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

[Daniel Bates]

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.

[Daniel Bates]

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.

[Zoltan Horvath]

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

[Daniel Bates]

Created attachment 45158 [details]
Patch

Updated patch based on Zoltan's comment.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45158 [details] without any errors.

[Eric Seidel (no email)]

Comment on attachment 45158 [details]
Patch

I'm confused.  Why wouldn't this break other windows ports?

[Daniel Bates]

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?

[Eric Seidel (no email)]

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

[Daniel Bates]

Committed in <http://trac.webkit.org/changeset/52522>.