Bug 135544 - [GTK] Install all unstable webkitdom headers
Summary: [GTK] Install all unstable webkitdom headers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-08-02 11:54 PDT by Michael Catanzaro
Modified: 2014-08-04 08:08 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.43 KB, patch)
2014-08-02 11:59 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Another approach (2.71 KB, patch)
2014-08-03 02:44 PDT, Carlos Garcia Campos
gns: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2014-08-02 11:54:37 PDT
We're accidentally failing to install unstable webkitdom headers for classes that have both stable and unstable functions.  In Source/WebCore/PlatformGTK.cmake we loop over all such headers that have been generated and add an install rule for each one, but do so before they've been generated so they all get missed.

(We're also going to need this patched in GNOME jhbuild; Epiphany is broken right now.)
Comment 1 Michael Catanzaro 2014-08-02 11:59:24 PDT
Created attachment 235937 [details]
Patch
Comment 2 Martin Robinson 2014-08-02 13:49:19 PDT
Comment on attachment 235937 [details]
Patch

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

> Source/WebCore/PlatformGTK.cmake:751
> +    if (EXISTS "${DERIVED_SOURCES_GOBJECT_DOM_BINDINGS_DIR}/WebKitDOM${classname}Unstable.h")

This still runs at configuration time, so won't this fail before the files are generated?
Comment 3 Carlos Garcia Campos 2014-08-03 01:03:10 PDT
Oh, I see the problem, I wonder why it works for me though. I think checking those files exist before adding them to the list was a bad idea in any case. Looking a the CMake docs, it seems we have other options, for example:

 a) Assume all stable classes have an Unstable.h header and make them optional.
    OPTIONAL: Specify that it is not an error if the file to be installed does not exist.

 b) Use a DIRECTORY install with a FILES_MATCHING option to install only the files matching the pattern *Unstable.h.

The problem I see with option b) is that we need the list of headers to generate the introspection files, so I guess we have to go with option a)
Comment 4 Carlos Garcia Campos 2014-08-03 02:17:21 PDT
(In reply to comment #3)
> Oh, I see the problem, I wonder why it works for me though. I think checking those files exist before adding them to the list was a bad idea in any case. Looking a the CMake docs, it seems we have other options, for example:
> 
>  a) Assume all stable classes have an Unstable.h header and make them optional.
>     OPTIONAL: Specify that it is not an error if the file to be installed does not exist.
> 
>  b) Use a DIRECTORY install with a FILES_MATCHING option to install only the files matching the pattern *Unstable.h.
> 
> The problem I see with option b) is that we need the list of headers to generate the introspection files, so I guess we have to go with option a)

Actually, I think we should probably not expose unstable symbols to the gir file, since there's not way to hide them by default as we do in C (or at least I don't know how to do it). The symbols are marked as unstable in the docs, but I think bindings don't use that info at all. So for now, I would only expose stable symbols to bindings.
Comment 5 Carlos Garcia Campos 2014-08-03 02:44:20 PDT
Created attachment 235944 [details]
Another approach
Comment 6 Michael Catanzaro 2014-08-03 07:34:53 PDT
(In reply to comment #2)
> This still runs at configuration time, so won't this fail before the files are generated?

Yes, my patch in bogus. I failed to removed my DerivedSources directory (maybe Carlos's problem too?) so any change at all to CMakeLists would have "fixed" this issue for me.

Carlos's patch actually works; I think that's the best approach.
Comment 7 Carlos Garcia Campos 2014-08-04 08:08:44 PDT
Committed r171991: <http://trac.webkit.org/changeset/171991>