Bug 180479 - Don't require perl(File::Copy::Recursive)
Summary: Don't require perl(File::Copy::Recursive)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-06 09:40 PST by Michael Catanzaro
Modified: 2017-12-14 06:38 PST (History)
11 users (show)

See Also:


Attachments
Patch (2.04 KB, patch)
2017-12-06 13:56 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.18 MB, application/zip)
2017-12-06 17:27 PST, EWS Watchlist
no flags Details
Patch (2.07 KB, patch)
2017-12-08 11:37 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (2.07 KB, patch)
2017-12-08 11:38 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (2.07 KB, patch)
2017-12-08 12:18 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (2.01 KB, patch)
2017-12-12 13:06 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (2.02 KB, patch)
2017-12-12 13:38 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (1.69 KB, patch)
2017-12-12 18:43 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (1.67 KB, patch)
2017-12-13 18:32 PST, Michael Catanzaro
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2017-12-06 09:40:23 PST
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.
Comment 1 Michael Catanzaro 2017-12-06 13:54:01 PST
Perl is hard!
Comment 2 Michael Catanzaro 2017-12-06 13:56:57 PST
Created attachment 328621 [details]
Patch
Comment 3 EWS Watchlist 2017-12-06 17:27:21 PST
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
Comment 4 EWS Watchlist 2017-12-06 17:27:22 PST
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
Comment 5 Michael Catanzaro 2017-12-08 08:20:28 PST
Ping reviewers.
Comment 6 Konstantin Tokarev 2017-12-08 08:28:59 PST
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)
Comment 7 Michael Catanzaro 2017-12-08 10:39:16 PST
(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.
Comment 8 Konstantin Tokarev 2017-12-08 10:43:32 PST
Indeed, it doesn't do recursive copy. File::Find could be used to obtain file list, but cp -R is probably fine
Comment 9 Michael Catanzaro 2017-12-08 11:37:41 PST
Created attachment 328843 [details]
Patch
Comment 10 Michael Catanzaro 2017-12-08 11:38:25 PST
Created attachment 328844 [details]
Patch
Comment 11 Konstantin Tokarev 2017-12-08 11:46:10 PST
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
Comment 12 Michael Catanzaro 2017-12-08 12:11:27 PST
(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!
Comment 13 Michael Catanzaro 2017-12-08 12:18:58 PST
Created attachment 328850 [details]
Patch
Comment 14 WebKit Commit Bot 2017-12-08 14:44:51 PST
Comment on attachment 328850 [details]
Patch

Clearing flags on attachment: 328850

Committed r225704: <https://trac.webkit.org/changeset/225704>
Comment 15 WebKit Commit Bot 2017-12-08 14:44:52 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2017-12-08 14:45:42 PST
<rdar://problem/35944218>
Comment 17 Michael Catanzaro 2017-12-12 13:04:00 PST
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".
Comment 18 Michael Catanzaro 2017-12-12 13:06:24 PST
Created attachment 329142 [details]
Patch
Comment 19 Konstantin Tokarev 2017-12-12 13:19:37 PST
Note that I suggested system(...) == 0 or die, not system(...) or die

Sorry that I failed to catch it in the patch
Comment 20 Michael Catanzaro 2017-12-12 13:23:59 PST
(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.
Comment 21 Michael Catanzaro 2017-12-12 13:25:29 PST
(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.
Comment 22 Michael Catanzaro 2017-12-12 13:27:13 PST
(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?
Comment 23 Michael Catanzaro 2017-12-12 13:27:43 PST
(In reply to Michael Catanzaro from comment #22)
> r?

Oh, I'll change it to "== 0 or die", that would be better.
Comment 24 Konstantin Tokarev 2017-12-12 13:35:23 PST
Yes please, system and die is too pessimistic
Comment 25 Michael Catanzaro 2017-12-12 13:38:12 PST
Created attachment 329147 [details]
Patch
Comment 26 WebKit Commit Bot 2017-12-12 14:23:21 PST
Comment on attachment 329147 [details]
Patch

Clearing flags on attachment: 329147

Committed r225806: <https://trac.webkit.org/changeset/225806>
Comment 27 WebKit Commit Bot 2017-12-12 14:23:23 PST
All reviewed patches have been landed.  Closing bug.
Comment 28 Michael Catanzaro 2017-12-12 18:06:00 PST
Goodness gracious.

I failed to test an incremental build... make_path() throws a fatal error if the directory in question already exists.
Comment 29 Michael Catanzaro 2017-12-12 18:43:20 PST
Created attachment 329192 [details]
Patch
Comment 30 Michael Catanzaro 2017-12-12 18:43:50 PST
(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....
Comment 31 Konstantin Tokarev 2017-12-13 02:54:08 PST
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
Comment 32 Konstantin Tokarev 2017-12-13 02:56:57 PST
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
Comment 33 Konstantin Tokarev 2017-12-13 03:02:38 PST
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
Comment 34 Michael Catanzaro 2017-12-13 07:32:38 PST
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.
Comment 35 Michael Catanzaro 2017-12-13 18:32:24 PST
(In reply to Michael Catanzaro from comment #34)
> Let me try this.

It works... good suggestion.
Comment 36 Michael Catanzaro 2017-12-13 18:32:51 PST
Created attachment 329305 [details]
Patch
Comment 37 Darin Adler 2017-12-13 21:56:27 PST
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 !".
Comment 38 Michael Catanzaro 2017-12-14 06:38:29 PST
(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.