Summary: | [WPE][CMake] Add "dist" and "distcheck" targets | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adrian Perez <aperez> | ||||||||||
Component: | Tools / Tests | Assignee: | Adrian Perez <aperez> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | cgarcia, clopez, commit-queue, lforschler, mrobinson, webkit-bug-importer, zan | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | Other | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Adrian Perez
2017-09-06 04:56:18 PDT
Created attachment 320010 [details]
Tentative patch
This reuses the make-dist.py script used by the GTK+ port, and adds
a new manifest to be used when assembling a WPE tarball, plus the needed commands/targets.
I wonder whether it would be a good idea to factor out the common parts and use the ${PORT}
variable in the commands in order to avoid repeating the code for WPE... WDYT?
I am going to update the patch and remove the check for “pixz”. While it does (de)compress faster, I have read that splitting the work to use multiple cores makes the compressed output bigger, due to the way the XZ compression works. If you are interested, you can scroll to the end of: https://jnovy.fedorapeople.org/pxz/node1.html Created attachment 320014 [details]
Tentative patch (v2)
Updated, use always the normal xz and make it report progress
Comment on attachment 320014 [details] Tentative patch (v2) View in context: https://bugs.webkit.org/attachment.cgi?id=320014&action=review Note that I don't use the cmake commands when making WebKitGTK releases, I run the make-dist script directly, I don't even know if the commands work. > Tools/wpe/manifest.txt.in:64 > +exclude Source/WebKit/gtk/NEWS$ This should already be excluded because of the gtk dir Created attachment 320017 [details]
Patch (v3)
This puts tarring and compressing commands into the
same target, for simplicity. Also, uses "xz -f" to make it always overwrite
the output tarball.
The nit mentioned by Carlos in the manifest is also fixed in this version.
I am running a “distcheck” locally, and I found an issue with the CMake build definitions for WPE. I have landed a build fix here: http://trac.webkit.org/changeset/221681/webkit With some luck there won't be any more issues which prevent tarballs generated by “make-dist.py” to compile and we can then land this :-) (In reply to Adrian Perez from comment #6) > I am running a “distcheck” locally, and I found an issue with > the CMake build definitions for WPE. I have landed a build fix > here: http://trac.webkit.org/changeset/221681/webkit > > With some luck there won't be any more issues which prevent > tarballs generated by “make-dist.py” to compile and we can then > land this :-) This worked fine, so I would say the patch is ready to land. There is one small issue: the “install” target only copies over the JSC headers — but the build from the generated tarball completed fine, and we can handle fixing the WPE “install” target in a separate bug. Also, I think if we want to refactor the CMake bits for the “dist” and “distcheck” targets (my opinion is “yes!”), I would also prefer to do it as a separate bug+patch as well. Let's try and get this landed! ---- [100%] Built target WebProcess Install the project... -- Install configuration: "Release" -- Installing: /home/aperez/devel/wpe/WebKit/WebKitBuild/Release/wpewebkit-0.0.20170728/_install/include/wpe-0.1/WPE/JavaScriptCore/JSBase.h -- Installing: /home/aperez/devel/wpe/WebKit/WebKitBuild/Release/wpewebkit-0.0.20170728/_install/include/wpe-0.1/WPE/JavaScriptCore/JSContextRef.h -- Installing: /home/aperez/devel/wpe/WebKit/WebKitBuild/Release/wpewebkit-0.0.20170728/_install/include/wpe-0.1/WPE/JavaScriptCore/JSObjectRef.h -- Installing: /home/aperez/devel/wpe/WebKit/WebKitBuild/Release/wpewebkit-0.0.20170728/_install/include/wpe-0.1/WPE/JavaScriptCore/JSStringRef.h -- Installing: /home/aperez/devel/wpe/WebKit/WebKitBuild/Release/wpewebkit-0.0.20170728/_install/include/wpe-0.1/WPE/JavaScriptCore/JSTypedArray.h -- Installing: /home/aperez/devel/wpe/WebKit/WebKitBuild/Release/wpewebkit-0.0.20170728/_install/include/wpe-0.1/WPE/JavaScriptCore/JSValueRef.h -- Installing: /home/aperez/devel/wpe/WebKit/WebKitBuild/Release/wpewebkit-0.0.20170728/_install/include/wpe-0.1/WPE/JavaScriptCore/JavaScript.h -- Installing: /home/aperez/devel/wpe/WebKit/WebKitBuild/Release/wpewebkit-0.0.20170728/_install/include/wpe-0.1/WPE/JavaScriptCore/WebKitAvailability.h -- Installing: /home/aperez/devel/wpe/WebKit/WebKitBuild/Release/wpewebkit-0.0.20170728/_install/lib/libWPEWebInspectorResources.so -- Installing: /home/aperez/devel/wpe/WebKit/WebKitBuild/Release/wpewebkit-0.0.20170728/_install/bin/WPEStorageProcess -- Set runtime path of "/home/aperez/devel/wpe/WebKit/WebKitBuild/Release/wpewebkit-0.0.20170728/_install/bin/WPEStorageProcess" to "" -- Installing: /home/aperez/devel/wpe/WebKit/WebKitBuild/Release/wpewebkit-0.0.20170728/_install/lib/libWPEWebKit.so.0.0.20170728 -- Installing: /home/aperez/devel/wpe/WebKit/WebKitBuild/Release/wpewebkit-0.0.20170728/_install/lib/libWPEWebKit.so.0 -- Installing: /home/aperez/devel/wpe/WebKit/WebKitBuild/Release/wpewebkit-0.0.20170728/_install/lib/libWPEWebKit.so -- Installing: /home/aperez/devel/wpe/WebKit/WebKitBuild/Release/wpewebkit-0.0.20170728/_install/bin/WPEWebProcess -- Set runtime path of "/home/aperez/devel/wpe/WebKit/WebKitBuild/Release/wpewebkit-0.0.20170728/_install/bin/WPEWebProcess" to "" -- Installing: /home/aperez/devel/wpe/WebKit/WebKitBuild/Release/wpewebkit-0.0.20170728/_install/bin/WPENetworkProcess -- Set runtime path of "/home/aperez/devel/wpe/WebKit/WebKitBuild/Release/wpewebkit-0.0.20170728/_install/bin/WPENetworkProcess" to "" /home/aperez/devel/wpe/WebKit/WebKitBuild/Release/wpewebkit-0.0.20170728.tar (1/1) Comment on attachment 320017 [details] Patch (v3) View in context: https://bugs.webkit.org/attachment.cgi?id=320017&action=review > Source/PlatformWPE.cmake:2 > + find_package(Xz REQUIRED) Do we really want to force everybody to have xz installed when only maintainers make releases? I guess most of the people have it installed already, but still. > Tools/wpe/manifest.txt.in:66 > +# TODO: No NEWS file for now. Sorry I missed this in previous review, but in WebKit we don't use TODO, but always FIXME > Tools/wpe/manifest.txt.in:93 > +# TODO: We are not currently generating documentation for WPE. Ditto. (In reply to Carlos Garcia Campos from comment #8) > Comment on attachment 320017 [details] > Patch (v3) > > View in context: > https://bugs.webkit.org/attachment.cgi?id=320017&action=review > > > Source/PlatformWPE.cmake:2 > > + find_package(Xz REQUIRED) > > Do we really want to force everybody to have xz installed when only > maintainers make releases? I guess most of the people have it installed > already, but still. We are only forcing developers to have xz, because it's inside the “if (DEVELOPER_MODE)” block, but not for normal builds. As a plus point, using “find_package” allows people to override the path to the tool with “-DXZ_EXECUTABLE=<path>” instead of relying on a “$PATH” search. That may be needed on “non-Unixy” systems (Windows comes to mind). Anyway, which developer wouldn't have xz? ;-) > > Tools/wpe/manifest.txt.in:66 > > +# TODO: No NEWS file for now. > > Sorry I missed this in previous review, but in WebKit we don't use TODO, but > always FIXME > > > Tools/wpe/manifest.txt.in:93 > > +# TODO: We are not currently generating documentation for WPE. > > Ditto. I'll change those two before landing. (In reply to Adrian Perez from comment #9) > (In reply to Carlos Garcia Campos from comment #8) > > Comment on attachment 320017 [details] > > Patch (v3) > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=320017&action=review > > > > > Source/PlatformWPE.cmake:2 > > > + find_package(Xz REQUIRED) > > > > Do we really want to force everybody to have xz installed when only > > maintainers make releases? I guess most of the people have it installed > > already, but still. > > We are only forcing developers to have xz, because it's inside the > “if (DEVELOPER_MODE)” block, but not for normal builds. With everybody I meant developers, only 1 or 2 developers make releases. > As a plus point, using “find_package” allows people to override the > path to the tool with “-DXZ_EXECUTABLE=<path>” instead of relying on > a “$PATH” search. That may be needed on “non-Unixy” systems (Windows > comes to mind). > > Anyway, which developer wouldn't have xz? ;-) For me that's a reason not to include all that, and keep things simple :-) > > > Tools/wpe/manifest.txt.in:66 > > > +# TODO: No NEWS file for now. > > > > Sorry I missed this in previous review, but in WebKit we don't use TODO, but > > always FIXME > > > > > Tools/wpe/manifest.txt.in:93 > > > +# TODO: We are not currently generating documentation for WPE. > > > > Ditto. > > I'll change those two before landing. Created attachment 320112 [details]
Patch for landing
Comment on attachment 320112 [details] Patch for landing Clearing flags on attachment: 320112 Committed r221734: <http://trac.webkit.org/changeset/221734> All reviewed patches have been landed. Closing bug. |