Bug 180479 – Don't require perl(File::Copy::Recursive)

• Component: Web Inspector
• Status: RESOLVED FIXED

[Michael Catanzaro]

We can't use File::Copy::Recursive on GNOME Continuous or the GNOME SDK, without someone who really understands both Yocto and Perl adding dozens of Perl dependencies at the Yocto level. Let's remove this dependency because there's no hope of WebKit being updated anytime soon otherwise.

[Michael Catanzaro]

Perl is hard!

[Michael Catanzaro]

Created attachment 328621 [details]
Patch

[EWS Watchlist]

Comment on attachment 328621 [details]
Patch

Attachment 328621 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5522385

New failing tests:
js/dom/modules/import-execution-order.html

[EWS Watchlist]

Created attachment 328661 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Michael Catanzaro]

Ping reviewers.

[Konstantin Tokarev]

Comment on attachment 328621 [details]
Patch

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

> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:54
> +                system('cp', '-a', "${source}/$_", $destination);

use File::Copy

(Also, if system is not Windows it doesn't necessary mean that cp is available and supports -a argument, which is not defined in POSIX: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/cp.html)

[Michael Catanzaro]

(In reply to Konstantin Tokarev from comment #6)
> Comment on attachment 328621 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328621&action=review
> 
> > Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:54
> > +                system('cp', '-a', "${source}/$_", $destination);
> 
> use File::Copy

I can try it, but it's not clear from http://perldoc.perl.org/File/Copy.html whether that would be a recursive copy... I presume not, because otherwise the File::Copy::Recursive module would not exist, right?

> (Also, if system is not Windows it doesn't necessary mean that cp is
> available and supports -a argument, which is not defined in POSIX:
> http://pubs.opengroup.org/onlinepubs/9699919799/utilities/cp.html)

I could change it to cp -R instead, which I think should be fine.

[Konstantin Tokarev]

Indeed, it doesn't do recursive copy. File::Find could be used to obtain file list, but cp -R is probably fine

[Michael Catanzaro]

Created attachment 328843 [details]
Patch

[Michael Catanzaro]

Created attachment 328844 [details]
Patch

[Konstantin Tokarev]

Comment on attachment 328844 [details]
Patch

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

> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:54
> +                system('cp', '-R', "${source}/$_", $destination);

I think error handling should be added here: system(...) == 0 or die ...

Also maybe use explicitly named iterator instead of $_, but that's up to your taste

[Michael Catanzaro]

(In reply to Konstantin Tokarev from comment #11)
> Comment on attachment 328844 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328844&action=review
> 
> > Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:54
> > +                system('cp', '-R', "${source}/$_", $destination);
> 
> I think error handling should be added here: system(...) == 0 or die ...

OK.

> Also maybe use explicitly named iterator instead of $_, but that's up to
> your taste

No clue how to do that!

[Michael Catanzaro]

Created attachment 328850 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 328850 [details]
Patch

Clearing flags on attachment: 328850

Committed r225704: <https://trac.webkit.org/changeset/225704>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35944218>

[Michael Catanzaro]

It's broken... it needs to do 'mkdir -p'. Not sure why I failed to notice this, but it might be I was only testing debug builds, where COMBINE_INSPECTOR_RESOURCES is turned off. Release builds are a bit different.

Also, I didn't test the error handling that I added at the last minute... in comment #11 it should be "and die" since system() returns 0 on success, not "or die".

[Michael Catanzaro]

Created attachment 329142 [details]
Patch

[Konstantin Tokarev]

Note that I suggested system(...) == 0 or die, not system(...) or die

Sorry that I failed to catch it in the patch

[Michael Catanzaro]

(In reply to Konstantin Tokarev from comment #19)
> Note that I suggested system(...) == 0 or die, not system(...) or die

Ahh, I failed to read.

I remember considering that, too, but I did it wrong anyway.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #17)
> It's broken... it needs to do 'mkdir -p'. Not sure why I failed to notice
> this, but it might be I was only testing debug builds, where
> COMBINE_INSPECTOR_RESOURCES is turned off. Release builds are a bit
> different.

I think in debug builds it only copies directories, not individual files, which is why I failed to realize mkdir -p is needed.

[Michael Catanzaro]

(In reply to Konstantin Tokarev from comment #19)
> Note that I suggested system(...) == 0 or die, not system(...) or die
> 
> Sorry that I failed to catch it in the patch

r?

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #22)
> r?

Oh, I'll change it to "== 0 or die", that would be better.

[Konstantin Tokarev]

Yes please, system and die is too pessimistic

[Michael Catanzaro]

Created attachment 329147 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 329147 [details]
Patch

Clearing flags on attachment: 329147

Committed r225806: <https://trac.webkit.org/changeset/225806>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

Goodness gracious.

I failed to test an incremental build... make_path() throws a fatal error if the directory in question already exists.

[Michael Catanzaro]

Created attachment 329192 [details]
Patch

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #29)
> Created attachment 329192 [details]
> Patch

I suspect this might not be the idiomatic way to silence errors, but it works. I don't know Perl....

[Konstantin Tokarev]

1. Looking at code of make_path, it should never die, only print warnings in some cases, and specifying error argument should only suppress the warning. That's weird that it causes fatal error, I'll try to reproduce later

2. Even if this is the case, you simply check that $destination is already a directory

    make_path($destination) if (!-d $destination)

3. General way of handling die/croak in called code is via code like

    eval { make_path($destination) }; # Unsafe code block is wrapped in eval {}
    if ($@} { # handle exception, $@ is what was provided in die() arguments
    ....

There are CPAN modules which make it prettier, e.g. Try::Tiny which provides familiar try/catch/finally syntax

[Konstantin Tokarev]

Also note that

   eval { make_path($destination) }

does totally different thing then

    eval "make_path($destination)"

the former executes already compiled code block, while the latter compiles & runs code constructed at run time

[Konstantin Tokarev]

Also, I think rewriting CPAN dependencies is a wrong way to go, if they cause troubles it's better to implement autoinstall like we do for Python, then increase code complexity gratuitously

[Michael Catanzaro]

Autoinstall is not an option because there won't be network access during the build. We just need to stop requiring external dependencies for essential build scripts.

(In reply to Konstantin Tokarev from comment #31)
> 1. Looking at code of make_path, it should never die, only print warnings in
> some cases, and specifying error argument should only suppress the warning.
> That's weird that it causes fatal error, I'll try to reproduce later

The documentation says it sometimes does die (via croak()).

(In reply to Konstantin Tokarev from comment #31)
> 2. Even if this is the case, you simply check that $destination is already a
> directory
> 
>     make_path($destination) if (!-d $destination)

Let me try this.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #34)
> Let me try this.

It works... good suggestion.

[Michael Catanzaro]

Created attachment 329305 [details]
Patch

[Darin Adler]

Comment on attachment 329305 [details]
Patch

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

> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:52
> +        make_path($destination) if (!-d $destination);

This doesn’t seem to match the documentation of make_path. The documentation says it works like "mkdir -p", which does not complain if a directory already exists.

Perl coding style comments: The parentheses here are not needed. Might consider "unless" instead of "if !".

[Michael Catanzaro]

(In reply to Darin Adler from comment #37)
> This doesn’t seem to match the documentation of make_path. The documentation
> says it works like "mkdir -p", which does not complain if a directory
> already exists.

Yes... that's why I did not add the check originally.

Here's what I wrote two days ago:

(In reply to Michael Catanzaro from comment #28)
> Goodness gracious.
> 
> I failed to test an incremental build... make_path() throws a fatal error if
> the directory in question already exists.

But in testing now, the code works fine. make_path() does nothing and does not throw any error when the directory already exists. So I don't think my patch here is right. I'm going to close this until I hit the problem again.