Bug 176448

Summary: [WPE][CMake] Add "dist" and "distcheck" targets
Product: WebKit Reporter: Adrian Perez <aperez>
Component: Tools / TestsAssignee: 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 Flags
Tentative patch
none
Tentative patch (v2)
none
Patch (v3)
none
Patch for landing none

Description Adrian Perez 2017-09-06 04:56:18 PDT
We need^W want to have targets to create release tarballs.
Ideally we would reuse the scripts and infrastructure which
already exists for the GTK+ port.
Comment 1 Adrian Perez 2017-09-06 05:04:39 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?
Comment 2 Adrian Perez 2017-09-06 05:35:12 PDT
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
Comment 3 Adrian Perez 2017-09-06 06:30:44 PDT
Created attachment 320014 [details]
Tentative patch (v2)

Updated, use always the normal xz and make it report progress
Comment 4 Carlos Garcia Campos 2017-09-06 06:37:03 PDT
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
Comment 5 Adrian Perez 2017-09-06 06:57:41 PDT
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.
Comment 6 Adrian Perez 2017-09-06 10:07:53 PDT
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 :-)
Comment 7 Adrian Perez 2017-09-06 13:30:21 PDT
(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 8 Carlos Garcia Campos 2017-09-07 04:53:52 PDT
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.
Comment 9 Adrian Perez 2017-09-07 07:55:01 PDT
(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.
Comment 10 Carlos Garcia Campos 2017-09-07 08:00:12 PDT
(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.
Comment 11 Adrian Perez 2017-09-07 08:19:23 PDT
Created attachment 320112 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2017-09-07 09:00:27 PDT
Comment on attachment 320112 [details]
Patch for landing

Clearing flags on attachment: 320112

Committed r221734: <http://trac.webkit.org/changeset/221734>
Comment 13 WebKit Commit Bot 2017-09-07 09:00:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2017-09-27 12:35:01 PDT
<rdar://problem/34693532>