Bug 138134

Summary: [GTK] Expand wildcards inside generate-inspector-gresource-manifest.py
Product: WebKit Reporter: Milan Crha <mcrha>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cgarcia, commit-queue, mrobinson, rniwa, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 137488, 133028    
Attachments:
Description Flags
proposed wk patch
none
proposed wk patch ][
cgarcia: commit-queue-
proposed wk patch ]I[
none
proposed webkit-2.4 branch patch cgarcia: review+, cgarcia: commit-queue-

Description Milan Crha 2014-10-28 09:00:32 PDT
The command line which invokes generate-inspector-gresource-manifest.py is 69790 bytes long on my system, which is way beyond my system (or build) environment limits. A better option is to expand the wildcards inside the script.
Comment 1 Milan Crha 2014-10-28 09:02:55 PDT
Created attachment 240543 [details]
proposed wk patch
Comment 2 Carlos Garcia Campos 2014-10-28 09:22:33 PDT
Comment on attachment 240543 [details]
proposed wk patch

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

> Source/WebKit2/PlatformGTK.cmake:377
> -file(GLOB InspectorFiles
> +list(APPEND InspectorFiles

I think this would break the build dependencies, we really want to rebuild the inspector gresource xml file when any of the inspector files changes.
Comment 3 Milan Crha 2014-10-29 00:16:08 PDT
Hmm, I see, the build also failed:
> ninja: error: '../../Source/WebInspectorUI/UserInterface/*.html', needed by
> 'DerivedSources/webkit2gtk/InspectorGResourceBundle.xml', missing and no
> known rule to make it

What do you suggest, have two variables, one for dependencies, one for passing to the .py script?

Something like:

--------------------------------------------------------------------------

set(InspectorFiles
    ${CMAKE_SOURCE_DIR}/Source/WebInspectorUI/UserInterface/*.html
    ....
)

file(GLOB InspectorFilesDeps
   ${InspectorFiles}
)

add_custom_command(
    OUTPUT ${DERIVED_SOURCES_WEBKIT2GTK_DIR}/InspectorGResourceBundle.xml
    DEPENDS ${InspectorFilesDeps}
            ${TOOLS_DIR}/gtk/generate-inspector-gresource-manifest.py
    COMMAND ${TOOLS_DIR}/gtk/generate-inspector-gresource-manifest.py --output=${DERIVED_SOURCES_WEBKIT2GTK_DIR}/InspectorGResourceBundle.xml ${InspectorFiles}
    VERBATIM
)

--------------------------------------------------------------------------

I think this might work, though might look awkward in the .cmake file.
Comment 4 Milan Crha 2014-11-01 10:12:21 PDT
Created attachment 240789 [details]
proposed wk patch ][

Updated version of the patch, with applied above proposal. It works fine on Windows (mingw/msys).
Comment 5 Carlos Garcia Campos 2015-04-09 01:58:28 PDT
Comment on attachment 240789 [details]
proposed wk patch ][

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

I'm fine with the general idea. Martin, could you confirm you also agree with this patch?

> Source/WebKit2/ChangeLog:7
> +

Please, explain here why we need this.

> Tools/gtk/generate-inspector-gresource-manifest.py:31
> +        names = glob.glob(pattern)

patterns are not just names, but paths, so I would use paths or filenames instead.

> Tools/gtk/generate-inspector-gresource-manifest.py:33
> +            base_dir_index = filename.rfind(BASE_DIR)

BASE_DIR contains a /, isn't that a problem when os.sep is not /?

> Tools/gtk/generate-inspector-gresource-manifest.py:37
> +                if os.sep != '/':
> +                    name = name.replace(os.sep, '/')

I would add a comment here explaining that glob.glob changes the separator to the os one, but we need the paths with / in the xml file
Comment 6 Martin Robinson 2015-04-09 07:49:42 PDT
Comment on attachment 240789 [details]
proposed wk patch ][

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

> Source/WebKit2/PlatformGTK.cmake:399
> +file(GLOB InspectorFilesDeps
> +    ${InspectorFiles}
> +)

Maybe call this InspectorManifestDependencies?
Comment 7 Martin Robinson 2015-04-09 07:50:06 PDT
This seems like an okay approach if it's necessary to fix compilation somewhere.
Comment 8 Carlos Garcia Campos 2015-04-15 09:52:23 PDT
Milan, would patch attached to bug #143755 fix this issue?
Comment 9 Milan Crha 2015-04-15 10:00:46 PDT
(In reply to comment #8)
> Milan, would patch attached to bug #143755 fix this issue?

I currently lack of free time, but I'll let you know as soon as I get to it. Initial thought would be no, because the issue here is with the command line length, not with the python being used (I faced the same issue when running the make command from a '.bat' file).
Comment 10 Milan Crha 2015-04-17 10:56:25 PDT
Created attachment 251028 [details]
proposed wk patch ]I[

So, the command in the msys/mingw looks like this here:

cd /C/MinGW/msys/1.0/data/develop/local/WebKitBuild/Source/WebKit2 && /C/MinGW/msys/1.0/data/develop/local/WebKit/Tools/gtk/generate-inspector-gresource-manifest.py --output=C:/MinGW/msys/1.0/data/develop/local/WebKitBuild/DerivedSources/webkit2gtk/InspectorGResourceBundle.xml "C:/MinGW/msys/1.0/data/develop/local/WebKit/Source/WebInspectorUI/UserInterface/*.html" ....

and it generates two first entries in the .xml file:

            <file compressed="true">UserInterface\Main.html</file>
            <file compressed="true">UserInterface\Test.html</file>

Hmm, it's not shown here, but only the last forward slash from the wildcard is replaced with the backslash, other deeper forward slashes are kept there, thus the BASE_DIR is not a problem.

I hope I addressed all the queries above. I do not have a variant of the patch for 2.4.9, I'd appreciate if you could make it and I'll give it a try.
Comment 11 WebKit Commit Bot 2015-04-17 10:58:51 PDT
Attachment 251028 [details] did not pass style-queue:


ERROR: Source/WebKit2/PlatformGTK.cmake:425:  Alphabetical sorting problem. ""        <file alias=\"images/missingImage@2x\">missingImage@2x.png</file>\n"" should be before ""        <file alias=\"images/missingImage\">missingImage.png</file>\n"".  [list/order] [5]
ERROR: Source/WebKit2/PlatformGTK.cmake:426:  There should be exactly one empty line instead of 0 between ""        <file alias=\"images/missingImage@2x\">missingImage@2x.png</file>\n"" and ""        <file alias=\"images/panIcon\">panIcon.png</file>\n"".  [list/emptyline] [5]
ERROR: Source/WebKit2/PlatformGTK.cmake:427:  There should be exactly one empty line instead of 0 between ""        <file alias=\"images/panIcon\">panIcon.png</file>\n"" and ""        <file alias=\"images/textAreaResizeCorner\">textAreaResizeCorner.png</file>\n"".  [list/emptyline] [5]
ERROR: Source/WebKit2/PlatformGTK.cmake:428:  Alphabetical sorting problem. ""        <file alias=\"images/textAreaResizeCorner@2x\">textAreaResizeCorner@2x.png</file>\n"" should be before ""        <file alias=\"images/textAreaResizeCorner\">textAreaResizeCorner.png</file>\n"".  [list/order] [5]
Total errors found: 4 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Milan Crha 2015-04-17 15:32:08 PDT
Created attachment 251058 [details]
proposed webkit-2.4 branch patch

This is what I use with the webkit-2.4 branch at revision 182543.
Comment 13 Carlos Garcia Campos 2015-05-19 00:26:54 PDT
Committed to 2.4 http://trac.webkit.org/changeset/184556
Comment 14 Zan Dobersek 2015-05-26 03:37:48 PDT
Comment on attachment 251028 [details]
proposed wk patch ]I[

This looks OK, so let's try and land it.
Comment 15 WebKit Commit Bot 2015-05-26 04:27:06 PDT
Comment on attachment 251028 [details]
proposed wk patch ]I[

Clearing flags on attachment: 251028

Committed r184856: <http://trac.webkit.org/changeset/184856>
Comment 16 Zan Dobersek 2015-07-06 11:54:08 PDT
*** Bug 145375 has been marked as a duplicate of this bug. ***
Comment 17 Zan Dobersek 2015-09-25 04:22:29 PDT
This landed already, so closing.