Bug 180137 – Build should fail early if needed Perl modules are not installed

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Tomas Popela]

While compiling latest WebKitGTK+ release (2.19.2) people started to complain, that the build failed because they were missing Perl's File-Copy-Recursive module (as WebKitGTK+ started to use copy-user-interface-resources.pl recently). The current situation is not optimal as we are not checking for mandatory Perl modules presence during configure so the build will fail after tens of minutes and not immediately.

[Tomas Popela]

Created attachment 327849 [details]
Patch

[Tomas Popela]

Hmm webkit-patch is broken for me and it didn't included the ChangeLog..

[Tomas Popela]

Created attachment 327850 [details]
Proposed patch

Uploading the patch manually (for the webkit-patch issues I filled bug 180138)

[Konstantin Tokarev]

Comment on attachment 327850 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=327850&action=review

> ChangeLog:13
> +        made it WebKit style compliant.

Please make it clear where is upstream copy of imported script

Also, I'm not sure it's a good idea to re-format 3rd party code, it complicates importing new versions from upstreams

> Source/cmake/WebKitCommon.cmake:25
> +    find_package(PerlModules COMPONENTS version REQUIRED)

AFAIU, only File::Copy::Recursive is actually required when you are not using build-webkit and don't run testing scripts

[Carlos Alberto Lopez Perez]

(In reply to Tomas Popela from comment #0)
> While compiling latest WebKitGTK+ release (2.19.2) people started to
> complain, that the build failed because they were missing Perl's
> File-Copy-Recursive module (as WebKitGTK+ started to use
> copy-user-interface-resources.pl recently). The current situation is not
> optimal as we are not checking for mandatory Perl modules presence during
> configure so the build will fail after tens of minutes and not immediately.

Seems we started to require this after r224566 <https://trac.webkit.org/r224566>
File::Copy::Recursive is required by copy-user-interface-resources.pl

The module is this one: http://search.cpan.org/dist/File-Copy-Recursive/

I wonder if it will be instead a better idea to copy the module in the tree (or reimplement it) rather than adding an extra external dependency for something as simple as "file copy recursive" :\

[Konstantin Tokarev]

I don't think it makes sense to bundle modules which are easily available

It might make sense to add autoinstall functionality similar to autoinstall which is used by our python code. I once shared this though with Daniel Bates and he agreed that it could be a good idea to do it with CPAN::Shell (core module).

This may be not good for File::Copy::Recursive which is used not in the testing infrastructure, but by build process, and distributions would want to have controlled build dpeendencies. For it we may want to call `cp -a` in the same way as macOS uses `ditto`. It's probably required to check that cp is from GNU coreutils first.

[Michael Catanzaro]

Comment on attachment 327850 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=327850&action=review

>> Source/cmake/WebKitCommon.cmake:25
>> +    find_package(PerlModules COMPONENTS version REQUIRED)
> 
> AFAIU, only File::Copy::Recursive is actually required when you are not using build-webkit and don't run testing scripts

CMake should only check for Perl modules that are required by the CMake build, yes. The rest probably don't need to be checked, because we can assume that if you're using build-webkit, you have run install-dependencies. But they could be checked at the build-webkit level, I suppose....

[Konstantin Tokarev]

Respective scripts will fail if used modules are not found, so the only specific handling can be autoinstalling

[Michael Catanzaro]

I assume you mean from build-webkit. Obviously CMake should not be autoinstalling anything.

[Konstantin Tokarev]

>Obviously CMake should not be autoinstalling anything.

Of course.

[Tomas Popela]

(In reply to Konstantin Tokarev from comment #4)
> Also, I'm not sure it's a good idea to re-format 3rd party code, it
> complicates importing new versions from upstreams

It was a really slight re-format (missing some spaces between cmake keywords and brackets)

[Tomas Popela]

(In reply to Konstantin Tokarev from comment #4)
> AFAIU, only File::Copy::Recursive is actually required when you are not
> using build-webkit and don't run testing scripts

JSON:PP is needed as well. (Initially I took the requirements from the Fedora package and from install-dependecies scripts).

[  1%] Generating ../../DerivedSources/ForwardingHeaders/JavaScriptCore/Scripts/lazywriter.py
cd /builddir/build/BUILD/webkitgtk-2.19.2/x86_64-redhat-linux-gnu/DerivedSources && /usr/bin/cmake -E copy_if_different /builddir/build/BUILD/webkitgtk-2.19.2/Source/JavaScriptCore/Scripts/lazywriter.py /builddir/build/BUILD/webkitgtk-2.19.2/x86_64-redhat-linux-gnu/DerivedSources/ForwardingHeaders/JavaScriptCore/Scripts/lazywriter.py
Can't locate JSON/PP.pm in @INC (you may need to install the JSON::PP module) (@INC contains: . /builddir/build/BUILD/webkitgtk-2.19.2/Source/WebCore/bindings/scripts /usr/local/lib64/perl5 /usr/local/share/perl5 /usr/lib64/perl5/vendor_perl /usr/share/perl5/vendor_perl /usr/lib64/perl5 /usr/share/perl5) at /builddir/build/BUILD/webkitgtk-2.19.2/Source/WebCore/bindings/scripts/generate-bindings.pl line 41.
BEGIN failed--compilation aborted at /builddir/build/BUILD/webkitgtk-2.19.2/Source/WebCore/bindings/scripts/generate-bindings.pl line 41.
Can't locate JSON/PP.pm in @INC (you may need to install the JSON::PP module) (@INC contains: . /builddir/build/BUILD/webkitgtk-2.19.2/Source/WebCore/bindings/scripts /usr/local/lib64/perl5 /usr/local/share/perl5 /usr/lib64/perl5/vendor_perl /usr/share/perl5/vendor_perl /usr/lib64/perl5 /usr/share/perl5) at /builddir/build/BUILD/webkitgtk-2.19.2/Source/WebCore/bindings/scripts/generate-bindings.pl line 41.
BEGIN failed--compilation aborted at /builddir/build/BUILD/webkitgtk-2.19.2/Source/WebCore/bindings/scripts/generate-bindings.pl line 41.
Can't locate JSON/PP.pm in @INC (you may need to install the JSON::PP module) (@INC contains: . /builddir/build/BUILD/webkitgtk-2.19.2/Source/WebCore/bindings/scripts /usr/local/lib64/perl5 /usr/local/share/perl5 /usr/lib64/perl5/vendor_perl /usr/share/perl5/vendor_perl /usr/lib64/perl5 /usr/share/perl5) at /builddir/build/BUILD/webkitgtk-2.19.2/Source/WebCore/bindings/scripts/generate-bindings.pl line 41.
BEGIN failed--compilation aborted at /builddir/build/BUILD/webkitgtk-2.19.2/Source/WebCore/bindings/scripts/generate-bindings.pl line 41.
[  1%] Generating ../../DerivedSources/ForwardingHeaders/JavaScriptCore/Scripts/make-js-file-arrays.py
Can't locate JSON/PP.pm in @INC (you may need to install the JSON::PP module) (@INC contains: . /builddir/build/BUILD/webkitgtk-2.19.2/Source/WebCore/bindings/scripts /usr/local/lib64/perl5 /usr/local/share/perl5 /usr/lib64/perl5/vendor_perl /usr/share/perl5/vendor_perl /usr/lib64/perl5 /usr/share/perl5) at /builddir/build/BUILD/webkitgtk-2.19.2/Source/WebCore/bindings/scripts/generate-bindings.pl line 41.
BEGIN failed--compilation aborted at /builddir/build/BUILD/webkitgtk-2.19.2/Source/WebCore/bindings/scripts/generate-bindings.pl line 41.
Can't locate JSON/PP.pm in @INC (you may need to install the JSON::PP module) (@INC contains: . /builddir/build/BUILD/webkitgtk-2.19.2/Source/WebCore/bindings/scripts /usr/local/lib64/perl5 /usr/local/share/perl5 /usr/lib64/perl5/vendor_perl /usr/share/perl5/vendor_perl /usr/lib64/perl5 /usr/share/perl5) at /builddir/build/BUILD/webkitgtk-2.19.2/Source/WebCore/bindings/scripts/generate-bindings.pl line 41.
BEGIN failed--compilation aborted at /builddir/build/BUILD/webkitgtk-2.19.2/Source/WebCore/bindings/scripts/generate-bindings.pl line 41.
cd /builddir/build/BUILD/webkitgtk-2.19.2/x86_64-redhat-linux-gnu/DerivedSources && /usr/bin/cmake -E copy_if_different /builddir/build/BUILD/webkitgtk-2.19.2/Source/JavaScriptCore/Scripts/make-js-file-arrays.py /builddir/build/BUILD/webkitgtk-2.19.2/x86_64-redhat-linux-gnu/DerivedSources/ForwardingHeaders/JavaScriptCore/Scripts/make-js-file-arrays.py
Can't locate JSON/PP.pm in @INC (you may need to install the JSON::PP module) (@INC contains: . /builddir/build/BUILD/webkitgtk-2.19.2/Source/WebCore/bindings/scripts /usr/local/lib64/perl5 /usr/local/share/perl5 /usr/lib64/perl5/vendor_perl /usr/share/perl5/vendor_perl /usr/lib64/perl5 /usr/share/perl5) at /builddir/build/BUILD/webkitgtk-2.19.2/Source/WebCore/bindings/scripts/generate-bindings.pl line 41.
BEGIN failed--compilation aborted at /builddir/build/BUILD/webkitgtk-2.19.2/Source/WebCore/bindings/scripts/generate-bindings.pl line 41.
make[2]: Leaving directory '/builddir/build/BUILD/webkitgtk-2.19.2/x86_64-redhat-linux-gnu'
make[2]: *** [Source/WebCore/CMakeFiles/WebCoreBindings.dir/build.make:61: Source/WebCore/CMakeFiles/WebCoreBindings] Error 1
make[1]: *** [CMakeFiles/Makefile2:930: Source/WebCore/CMakeFiles/WebCoreBindings.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....

[Konstantin Tokarev]

Indeed, I probably looked at wrong branch.

Slightly offtopic here, but I think it would be nice to use JSON::XS instead of JSON::PP if it's present

[Tomas Popela]

Created attachment 327974 [details]
Patch

[Tomas Popela]

Hmm I see that it's failing on win as the build bot is missing File::Copy::Recursive, but I think it's actually not needed there. I will move the File::Copy::Recursive to OptionsGTK.cmake and OptionsWPE.cmake. But what about other cmake ports? Leaving it in WebKitCommon.cmake, but add there some condition for win port? Any idea for some elegant solution to avoid duplicity?

[Konstantin Tokarev]

>but I think it's actually not needed there

I see that vcxproj files are calling copy-user-interface-resources.pl, so I guess error is present but not fatal

[Konstantin Tokarev]

BTW there is another approach: use find_package() without REQUIRED (in dubious cases, not when e.g. Perl itself is missing), and then mark them as required in port-specific files via set_package_properties() of FeatureSummary module.

It may be a good idea to use FeatureSummary more extesively and maybe even ditch our custom configuration summary printing. I remember we discussed with Michael already.

[Michael Catanzaro]

> It may be a good idea to use FeatureSummary more extesively and maybe even
> ditch our custom configuration summary printing. I remember we discussed
> with Michael already.

I'd probably be fine with that, if you file a bug with an example of what the output would look like.

[Michael Catanzaro]

Comment on attachment 327974 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=327974&action=review

> Source/cmake/WebKitCommon.cmake:21
> +    find_package(PerlModules COMPONENTS File::Copy::Recursive REQUIRED)

But see bug #180479

> Source/cmake/WebKitCommon.cmake:22
> +    find_package(PerlModules COMPONENTS JSON::PP REQUIRED)

This is apparently a core Perl module, so I'm not sure if we need to test for it? Does anyone know?

[Konstantin Tokarev]

>This is apparently a core Perl module

Not it isn't

[Michael Catanzaro]

Comment on attachment 327974 [details]
Patch

Emmanuele says:

"Yes, you can always assume they are there if Perl is there

If people go out of their way to install Perl and remove the core modules, aside from breaking everything they can also add them back

It's not going to be any more complicated than installing a non-core module"

So sorry to trash your work Tom, but we don't need to test for JSON::PP, and File::Copy::Recursive is a no-go. I think we should just never make the CMake build depend on any non-core Perl modules.

[Michael Catanzaro]

(In reply to Konstantin Tokarev from comment #20)
> >This is apparently a core Perl module
> 
> Not it isn't

It's not? Um

[Michael Catanzaro]

OK then.

[Michael Catanzaro]

Comment on attachment 327974 [details]
Patch

r+ -> r? -> r+ -> r- -> r+ again, have a nice day. I guess.

[Konstantin Tokarev]

All core modules are documented at http://perldoc.perl.org/

[Michael Catanzaro]

We're confused because http://search.cpan.org/~ishigaki/JSON-PP-2.97000/lib/JSON/PP.pm says "JSON::PP has been in the Perl core since Perl 5.14, mainly for CPAN toolchain modules to parse META.json."

[Konstantin Tokarev]

It seems that you are right, vanilla Perl 5.26.1 freshly installed with plenv provides usable JSON::PP out of the box. There is no File::Copy::Recursive though.

Still, two considerations:

* Debian splits Perl into packages "perl-base" and "perl-modules", the latter is what provides JSON::PP. So it's still possible to have a system with working perl but no JSON:PP
* We (still) require only Perl 5.10, while JSON::PP document talks about 5.14

[Tomas Popela]

(In reply to Konstantin Tokarev from comment #27)
> It seems that you are right, vanilla Perl 5.26.1 freshly installed with
> plenv provides usable JSON::PP out of the box. There is no
> File::Copy::Recursive though.

F27+ is using 5.26.1 and the build is failing unless I specify to install JSON::PP into it. So I'm confused. So maybe there is something wrong with Fedora packaging.

[Konstantin Tokarev]

Fedora makes a separate rpm from each of core modules: https://src.fedoraproject.org/cgit/rpms/perl.git/tree/perl.spec#n3096

[Tomas Popela]

(In reply to Konstantin Tokarev from comment #29)
> Fedora makes a separate rpm from each of core modules:
> https://src.fedoraproject.org/cgit/rpms/perl.git/tree/perl.spec#n3096

I know, but it really doesn't matter - what matters is that if it's supposed to be in core modules - then if should be installed if perl is installed.

[Tomas Popela]

(In reply to Tomas Popela from comment #30)
> (In reply to Konstantin Tokarev from comment #29)
> > Fedora makes a separate rpm from each of core modules:
> > https://src.fedoraproject.org/cgit/rpms/perl.git/tree/perl.spec#n3096
> 
> I know, but it really doesn't matter - what matters is that if it's supposed
> to be in core modules - then if should be installed if perl is installed.

Now I probably see that problem as we are not installing perl, but only a subpackage - that's the reason why it's not pulling it in build root. OK, then this patch is not required at all I think..

[Konstantin Tokarev]

This means Fedora, like Debian, doesn't install all core modules by default. Separate question is if this needs any handling, or we should support only full perl

[Tomas Popela]

(In reply to Konstantin Tokarev from comment #32)
> This means Fedora, like Debian, doesn't install all core modules by default.
> Separate question is if this needs any handling, or we should support only
> full perl

Yes, that's the question. But if Debian as well as Fedora are behaving like that then we probably could check for it in the end.

[Michael Catanzaro]

Yes, let's check for JSON::PP.

[Tomas Popela]

Created attachment 328694 [details]
Patch

[Konstantin Tokarev]

Comment on attachment 328694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328694&action=review

> Source/cmake/WebKitCommon.cmake:21
> +    find_package(PerlModules COMPONENTS JSON::PP REQUIRED)

Where is File::Copy::Recursive?

[Tomas Popela]

(In reply to Konstantin Tokarev from comment #36)
> Comment on attachment 328694 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328694&action=review
> 
> > Source/cmake/WebKitCommon.cmake:21
> > +    find_package(PerlModules COMPONENTS JSON::PP REQUIRED)
> 
> Where is File::Copy::Recursive?

Removed - look at bug 180199.

[Konstantin Tokarev]

I think it should be required in OptionsGTK.cmake

[Konstantin Tokarev]

Or it can be found in WebKitCommon. but marked as required in GTK and WPE

[Konstantin Tokarev]

Oops I didn't intend to close it

[Michael Catanzaro]

I assume JSON::PP is required by all ports... if not, then WebKitCommon.cmake is indeed not the right place for the check.

[Konstantin Tokarev]

I think it's OK to check for presence of core module even if it's not required. This is just a workaround for limitation/feature of Linux distros which make possible to have incomplete perl installation. On Windows and Mac it should always be present

[Tomas Popela]

(In reply to Michael Catanzaro from comment #41)
> I assume JSON::PP is required by all ports... if not, then
> WebKitCommon.cmake is indeed not the right place for the check.

Looks like it is.. At least it is installed on all bots (the EWS is green - previously was red on win if File::Copy::Recursive was required and missing)..

[Konstantin Tokarev]

LGTM

[Tomas Popela]

Comment on attachment 328694 [details]
Patch

Clearing flags on attachment: 328694

Committed r225901: <https://trac.webkit.org/changeset/225901>

[Tomas Popela]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36046689>