Bug 103687 – [Windows, WinCairo] Generate Library Export Definition File

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Brent Fulgham]

[Windows, WinCairo] Build fix for BitmapImage Test Change

[Brent Fulgham]

Created attachment 176851 [details]
Patch

[Tim Horton]

This is because Windows exports files aren't preprocessed, so we can't put a #if in the .defs file?

I guess that's OK. Will this not cause any annoying unused function warnings on Mac release? I'd like to see that bot catch up...

[Tim Horton]

Seems sad to build code into release builds that we won't use, anyway, even if it is trivial.

[Brent Fulgham]

(In reply to comment #3)
> Seems sad to build code into release builds that we won't use, anyway, even if it is trivial.

I can only think of a few options:

1. Create (and maintain) two def files (debug and release), perhaps autogenerating them from a single file.
2. Add a macro to the "notSolidColor" definition that resolves to "__declspec(dllexport)" on Windows, and blank to everything.  You can apparently use both dllexport and def files, so this should work.
3. Inline the definition of the method to the header file so that it can be seen by all compilation units.
4. Remove the compile guards and build for everyone.

Of all of these, maybe (3) is the best since it does not add cruft to release builds, with the relatively small downside of inlining a virtual method.

[Tim Horton]

(In reply to comment #4)
> 1. Create (and maintain) two def files (debug and release), perhaps autogenerating them from a single file.

Alternatively, do what Mac does and preprocess the def file? Or is that hard for VS to do?

(see http://trac.webkit.org/browser/trunk/Source/WebCore/WebCore.exp.in)

[Brent Fulgham]

(In reply to comment #5)
> (In reply to comment #4)
> > 1. Create (and maintain) two def files (debug and release), perhaps autogenerating them from a single file.
> 
> Alternatively, do what Mac does and preprocess the def file? Or is that hard for VS to do?
> 
> (see http://trac.webkit.org/browser/trunk/Source/WebCore/WebCore.exp.in)

I tried a few tricks to get cl.exe to spit out preprocessed output, but that just generated a bunch of incompatible def file contents.

I might be able to use the Cygwin layer to try to do some preprocessing.  I'll give that a try tomorrow.

[Lucas Forschler]

*** Bug 103755 has been marked as a duplicate of this bug. ***

[Roger Fong]

Can we at least roll out the patch that's causing the build failures for now?

[Brent Fulgham]

The attached patch updates the build system to use a generator program to emit the WebKit2.def file used by the Windows builds.  This has a number of benefits:

1. We can conditionally decide which symbols to expose in the WebKit.dll.
2. We can get rid of the separately-maintained WebKit2.def and WebKit2CFLite.def files.
3. We can add 64-bit symbol export.

I am reusing the make-export-file-generator ruby script from WebCore; I copied it to WebKit2/win. It might be better to just use the version in WebCore.

[Brent Fulgham]

(In reply to comment #8)
> Can we at least roll out the patch that's causing the build failures for now?

Oh!  Yes, that's fine.  I'm not sure how long it will take to get this patch approved and landed so we might was well fix the build for now.

[Brent Fulgham]

Created attachment 177019 [details]
Patch

[Tim Horton]

Comment on attachment 177019 [details]
Patch

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

Assuming everything builds, this seems fine to me... do we *need* the additional project?

> Source/WebKit2/ChangeLog:21
> +        * win/WebKit2ExportGenerator.vcproj.WYATT.bfulgham.user: Added.

This shouldn't be added to the repo.

> Source/WebKit2/win/WebKit2.def.in:27
> +		; Deprecated re-exports from JavaScriptCore

Indentation.

> Source/WebKit2/win/WebKit2.def.in:146
> +; Re-exports from WebCore for test harnesses

We should choose an indentation and stick with it :D

> Source/WebKit2/win/make-export-file-generator:1
> +#!/usr/bin/env ruby

Would be nice to only have one copy of this in the repo.

[Brent Fulgham]

(In reply to comment #12)
> (From update of attachment 177019 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177019&action=review
> 
> Assuming everything builds, this seems fine to me... do we *need* the additional project?
> 
> > Source/WebKit2/ChangeLog:21
> > +        * win/WebKit2ExportGenerator.vcproj.WYATT.bfulgham.user: Added.

Doh!  I caught this in the diff, but forgot to update the ChangeLog.
 
> > Source/WebKit2/win/make-export-file-generator:1
> > +#!/usr/bin/env ruby
> 
> Would be nice to only have one copy of this in the repo.

Is it okay for me to just reference it from the WebCore directory?  It works locally, but I didn't know if there were any project restrictions about using local utility scripts from separate parts of the build.

I'll change back to referencing the WebCore copy, if that's okay.

[Tim Horton]

(In reply to comment #13)
> > > Source/WebKit2/win/make-export-file-generator:1
> > > +#!/usr/bin/env ruby
> > 
> > Would be nice to only have one copy of this in the repo.
> 
> Is it okay for me to just reference it from the WebCore directory?  It works locally, but I didn't know if there were any project restrictions about using local utility scripts from separate parts of the build.
> 
> I'll change back to referencing the WebCore copy, if that's okay.

I *think* that should be fine, and we'll find out pretty quickly if it's not :)

[Brent Fulgham]

(In reply to comment #12)
> (From update of attachment 177019 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177019&action=review
> 
> Assuming everything builds, this seems fine to me... do we *need* the additional project?

I do think its needed because I want access to the compile-time flags supplied by the various VS property sheets.  I originally tried tacking it on to the WebKit2Generated project, but couldn't figure out how to access that stuff.

Maybe someone else has a better idea of how to do so.

[Brent Fulgham]

Committed r136292: <http://trac.webkit.org/changeset/136292>

[Roger Fong]

12>LINK : fatal error LNK1104: cannot open file 'C:/cygwin/home/buildbot/slave/windows-release-archive/build/build-Release\Production\obj\WebKit2ExportGenerator\WebKit2.def'

\'s vs /'s?

Also the WebKit2ExportGenerator folder isn't there.
I can try kicking the bot, just for kicks, probably needs it after adding a project.
I'm skeptical that it'll fix the /'s vs \'s issue though...

[Roger Fong]

hold that thought, 
build is also failing from a different unrelated patch

[Brent Fulgham]

(In reply to comment #18)
> hold that thought, 
> build is also failing from a different unrelated patch

Yeah - I think the generation will not happen if the WebCore build fails.

The directory delimiter characters are that way because parts of the path are coming from Visual Studio and others from the Cygwin layer.  

Actually, the "C:/cygwin/home/buildbot/slave/windows-release-archive/build/build-Release\" is being passed to Visual Studio through the $(ConfigurationBuildDir) variable.  I'm not sure how that's being set, but maybe it should be run through "cygpath -w" before VS sees them.

[Brent Fulgham]

All Apple Windows bots and my WinCairo bot build cleanly, as well as local debug and release builds of both variants.

[Brent Fulgham]

Reopen to add a small cleanup.

[Brent Fulgham]

Created attachment 177085 [details]
Patch

[Tim Horton]

Comment on attachment 177085 [details]
Patch

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

> Source/WebKit2/win/WebKit2.def.in:NaN
>  EXPORTS

Why two inspector blocks? (It's fine, though!)

[Brent Fulgham]

(In reply to comment #23)
> (From update of attachment 177085 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177085&action=review
> 
> > Source/WebKit2/win/WebKit2.def.in:NaN
> >  EXPORTS
> 
> Why two inspector blocks? (It's fine, though!)

I was trying to keep the groupings as similar to the original file as possible. The second group of inspector symbols were below a comment that indicated that they were exported for use by the testing harness. I wanted to keep those symbols where they were.

[WebKit Review Bot]

Comment on attachment 177085 [details]
Patch

Clearing flags on attachment: 177085

Committed r136336: <http://trac.webkit.org/changeset/136336>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.