Bug 176448 – [WPE][CMake] Add "dist" and "distcheck" targets

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Adrian Perez]

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.

[Adrian Perez]

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?

[Adrian Perez]

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

[Adrian Perez]

Created attachment 320014 [details]
Tentative patch (v2)

Updated, use always the normal xz and make it report progress

[Carlos Garcia Campos]

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

[Adrian Perez]

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.

[Adrian Perez]

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 :-)

[Adrian Perez]

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

[Carlos Garcia Campos]

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.

[Adrian Perez]

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

[Carlos Garcia Campos]

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

[Adrian Perez]

Created attachment 320112 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 320112 [details]
Patch for landing

Clearing flags on attachment: 320112

Committed r221734: <http://trac.webkit.org/changeset/221734>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34693532>