Bug 155124 – [GTK] Add a run-epiphany helper script.

• Component: Tools / Tests
• Status: NEW

[Carlos Alberto Lopez Perez]

The idea is to be able to easily execute the system epiphany browser with the locally built WebKit, supporting both --release and --debug command line switches.

[Carlos Alberto Lopez Perez]

Created attachment 273198 [details]
Patch

[Konstantin Tokarev]

Comment on attachment 273198 [details]
Patch

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

> Tools/Scripts/run-epiphany:1
> +#!/usr/bin/perl -w

You already have use warnings, no need for "-w"

> Tools/Scripts/run-epiphany:38
> +my $binPath;

Declare local variables where they are used

> Tools/Scripts/run-epiphany:57
> +    system(@jhbuildWrapper, "epiphany", @ARGV) or die;

die $!;

> Tools/Scripts/webkitdirs.pm:1110
> +sub setPortDefault($)

I think it would be better to add just one function

sub setPortName { $portName = shift; }

And use it like here:

setPortName(GTK);
prohibitUnknownPort();
# ...

[Carlos Garcia Campos]

Comment on attachment 273198 [details]
Patch

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

>> Tools/Scripts/run-epiphany:1
>> +#!/usr/bin/perl -w
> 
> You already have use warnings, no need for "-w"

I prefer to write new scripts in python instead of perl, unless we really need something from existing perl scripts.

> Tools/Scripts/run-epiphany:5
> +# Copyright (C) 2005, 2007, 2013 Apple Inc. All rights reserved.
> +# Copyright (C) 2007 Staikos Computing Services, Inc.  <[e-mail]>
> +# Copyright (C) 2016 Igalia S.L.

Where are these copyrights come from?

> Tools/Scripts/run-epiphany:45
> +if (isGtk()) {

If we are going to use perl, I would move this to webkitdirs.pm as a function, runEpiphany() like the existing runSafari or runMiniBrowser.

> Tools/Scripts/run-epiphany:55
> +    @jhbuildWrapper = wrapperPrefixIfNeeded();

I'm not sure about using the jhbuild for this. We don't have the required deps nor ephy in our jhbuild, so we will end up using the system installed ephy, for which we definitely don't want to use the (usually old) libs compiled in our jhbuild.

> Tools/Scripts/run-epiphany:60
> +} else {
> +    die "Unsupported platform."
> +}

Can this really happen? you are setting GTK as port in setPortDefault(GTK); so isGtk will always be true here, we don't even need to check it.

[Carlos Alberto Lopez Perez]

(In reply to comment #3)
> Comment on attachment 273198 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=273198&action=review
> 
> >> Tools/Scripts/run-epiphany:1
> >> +#!/usr/bin/perl -w
> > 
> > You already have use warnings, no need for "-w"
> 
> I prefer to write new scripts in python instead of perl, unless we really
> need something from existing perl scripts.
> 

I also prefer python, but I stick with was is already in place. Both run-minibrowser and run-safari are in Perl.

> > Tools/Scripts/run-epiphany:5
> > +# Copyright (C) 2005, 2007, 2013 Apple Inc. All rights reserved.
> > +# Copyright (C) 2007 Staikos Computing Services, Inc.  <[e-mail]>
> > +# Copyright (C) 2016 Igalia S.L.
> 
> Where are these copyrights come from?
> 

from Tools/Scripts/run-minibrowser

> > Tools/Scripts/run-epiphany:45
> > +if (isGtk()) {
> 
> If we are going to use perl, I would move this to webkitdirs.pm as a
> function, runEpiphany() like the existing runSafari or runMiniBrowser.
> 
> > Tools/Scripts/run-epiphany:55
> > +    @jhbuildWrapper = wrapperPrefixIfNeeded();
> 
> I'm not sure about using the jhbuild for this. We don't have the required
> deps nor ephy in our jhbuild, so we will end up using the system installed
> ephy, for which we definitely don't want to use the (usually old) libs
> compiled in our jhbuild.
> 

Good point.

The problem is that the system libraries are usually not enough to run webkitgtk+ when it was built using the jhbuild. For example things like cairo-gl or openwebrtc are not available in my Debian system.

Also since the main purpose is to test WebKitGTK+ rather than epiphany I prefer to use the libraries that were used to built WebKitGTK+

I guess that the best solution would be to run only the webkit process inside the jhbuild. Maybe something like WEB_PROCESS_CMD_PREFIX can help.

> > Tools/Scripts/run-epiphany:60
> > +} else {
> > +    die "Unsupported platform."
> > +}
> 
> Can this really happen? you are setting GTK as port in setPortDefault(GTK);
> so isGtk will always be true here, we don't even need to check it.

Can happen if the user pass --portname other than --gtk (example: --efl) in the command line.

[Michael Catanzaro]

I'm inclined to agree with Carlos, this script will probably be broken in general unless we start updating our jhbuild environment more regularly to keep up with stable Epiphany releases. For instance, your system should have Epiphany 3.18, but our jhbuild environment has GTK+ 3.16. If Epiphany 3.18 did not require GTK+ 3.18, that was just luck.

(In reply to comment #4)
> I guess that the best solution would be to run only the webkit process
> inside the jhbuild. Maybe something like WEB_PROCESS_CMD_PREFIX can help.

Interesting idea, that just might work.

[Michael Catanzaro]

(In reply to comment #5)
> I'm inclined to agree with Carlos

Gosh darn it, this must be the third time in the past month....

I'm inclined to agree with Carlos Garcia.

[Michael Catanzaro]

Comment on attachment 273198 [details]
Patch

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

I'm going to give this r- for the reasons discussed above. WebKit jhbuild is just too old to run system epiphany on recent systems.

>> Tools/Scripts/run-epiphany:38
>> +my $binPath;
> 
> Declare local variables where they are used

Is it normal in Perl to not leave a blank line after imports?

[Konstantin Tokarev]

Comment on attachment 273198 [details]
Patch

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

>>> Tools/Scripts/run-epiphany:38
>>> +my $binPath;
>> 
>> Declare local variables where they are used
> 
> Is it normal in Perl to not leave a blank line after imports?

People usually leave blank line, but perlcritic doesn't complain if it's missing