Bug 138134

Created attachment 240543 [details] proposed wk patch

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.

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.

Created attachment 240789 [details] proposed wk patch ][ Updated version of the patch, with applied above proposal. It works fine on Windows (mingw/msys).

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 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?

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

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

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

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.

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.

Created attachment 251058 [details] proposed webkit-2.4 branch patch This is what I use with the webkit-2.4 branch at revision 182543.

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

Comment on attachment 251028 [details] proposed wk patch ]I[ This looks OK, so let's try and land it.

Comment on attachment 251028 [details] proposed wk patch ]I[ Clearing flags on attachment: 251028 Committed r184856: <http://trac.webkit.org/changeset/184856>

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

This landed already, so closing.