Bug 138134 – [GTK] Expand wildcards inside generate-inspector-gresource-manifest.py

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Milan Crha]

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.

[Milan Crha]

Created attachment 240543 [details]
proposed wk patch

[Carlos Garcia Campos]

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.

[Milan Crha]

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.

[Milan Crha]

Created attachment 240789 [details]
proposed wk patch ][

Updated version of the patch, with applied above proposal. It works fine on Windows (mingw/msys).

[Carlos Garcia Campos]

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

[Martin Robinson]

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?

[Martin Robinson]

This seems like an okay approach if it's necessary to fix compilation somewhere.

[Carlos Garcia Campos]

Milan, would patch attached to bug #143755 fix this issue?

[Milan Crha]

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

[Milan Crha]

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.

[WebKit Commit Bot]

Attachment 251028 [details] did not pass style-queue:


ERROR: Source/WebKit2/PlatformGTK.cmake:425:  Alphabetical sorting problem. ""        <file alias=\"images/missingImage@2x\">[e-mail]</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\">[e-mail]</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\">[e-mail]</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.

[Milan Crha]

Created attachment 251058 [details]
proposed webkit-2.4 branch patch

This is what I use with the webkit-2.4 branch at revision 182543.

[Carlos Garcia Campos]

Committed to 2.4 http://trac.webkit.org/changeset/184556

[Zan Dobersek]

Comment on attachment 251028 [details]
proposed wk patch ]I[

This looks OK, so let's try and land it.

[WebKit Commit Bot]

Comment on attachment 251028 [details]
proposed wk patch ]I[

Clearing flags on attachment: 251028

Committed r184856: <http://trac.webkit.org/changeset/184856>

[Zan Dobersek]

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

[Zan Dobersek]

This landed already, so closing.