[webkit-reviews] review denied: [Bug 35213] [chromium] hardcoded gcc path breaks solaris build : [Attachment 49416] seperate into new variable, reformatting again

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied electricmonopole at gmail.com's
request for review:
Bug 35213: [chromium] hardcoded gcc path breaks solaris build
https://bugs.webkit.org/show_bug.cgi?id=35213

Attachment 49416: seperate into new variable, reformatting again
https://bugs.webkit.org/attachment.cgi?id=49416&action=review

------- Additional Comments from David Levin <levin at chromium.org>
> Index: WebCore/bindings/scripts/CodeGeneratorObjC.pm
> ===================================================================
> --- WebCore/bindings/scripts/CodeGeneratorObjC.pm	(revision 55194)
> +++ WebCore/bindings/scripts/CodeGeneratorObjC.pm	(working copy)
> @@ -222,7 +222,13 @@ sub ReadPublicInterfaces
>      %publicInterfaces = ();
>  
>      my $fileName = "WebCore/bindings/objc/PublicDOMInterfaces.h";
> -    open FILE, "-|", "/usr/bin/gcc", "-E", "-P", "-x", "objective-c", 
> +    my $gcclocation

Should be gccLocation (since it is two separate words). This should be done
throughout the change.
Also add = ""; so it should look like this:

my $gccLocation = "";

> +    if (($Config::Config{'osname'})=~/solaris/i){

You need a space before the {
This should be done in several other places too.

> +	 $gcclocation = "/usr/sfw/bin/gcc";

4 space indent.

> +    } else {
> +	 $gcclocation = "/usr/bin/gcc";

4 space indent.


> ===================================================================
> --- WebCore/bindings/scripts/IDLParser.pm	(revision 55194)
> +++ WebCore/bindings/scripts/IDLParser.pm	(working copy)
> @@ -64,7 +64,15 @@ sub Parse
>      $parentsOnly = shift;
>  
>      if (!$preprocessor) {
> -	   $preprocessor = "/usr/bin/gcc -E -P -x c++";
> +	   # Detect OS. If Solaris, use /usr/sfw/bin/gcc.
> +	   require Config;
> +	   my $gccLocation = "";
> +	   if (($Config::Config{'osname'})=~/solaris/i){
> +	       $gcclocation = "/usr/sfw/bin/gcc";

$gccLocation (throughout)

> +	   } else {
> +	       $gcclocation = "/usr/bin/gcc";
> +	   }
> +	   $preprocessor = $gcclocation . " -E -P -x c++";
>      }
>  
>      if (!$defines) {
> Index: WebCore/css/make-css-file-arrays.pl
> ===================================================================
> --- WebCore/css/make-css-file-arrays.pl	(revision 55194)
> +++ WebCore/css/make-css-file-arrays.pl	(working copy)
> @@ -28,7 +28,15 @@ my $preprocessor;
>  GetOptions('preprocessor=s' => \$preprocessor);
>  
>  if (!$preprocessor) {
> -    $preprocessor = "/usr/bin/gcc -E -P -x c++";
> +    # Detect OS. If Solaris, use /usr/sfw/bin/gcc.
> +    require Config;
> +    my $gccLocation = "";
> +    if (($Config::Config{'osname'})=~/solaris/i){
> +	   $gcclocation = "/usr/sfw/bin/gcc";
> +    } else {
> +	   $gcclocation = "/usr/bin/gcc";
> +    }
> +    $preprocessor = $gcclocation . " -E -P -x c++";
>  }
>  
>  my $header = $ARGV[0];
> Index: WebCore/dom/make_names.pl
> ===================================================================
> --- WebCore/dom/make_names.pl (revision 55194)
> +++ WebCore/dom/make_names.pl (working copy)
> @@ -47,7 +47,16 @@ my %tags = ();
>  my %attrs = ();
>  my %parameters = ();
>  my $extraDefines = 0;
> -my $preprocessor = "/usr/bin/gcc -E -P -x c++";
> +my $preprocessor = "";

Get rid of this line and make the line where you initialize $preprocessor do
the "my".

> +# Detect OS. If Solaris, use /usr/sfw/bin/gcc.
> +require Config;
> +my $gccLocation = "";
> +if (($Config::Config{'osname'})=~/solaris/i){
> +    $gcclocation = "/usr/sfw/bin/gcc";
> +} else {
> +    $gcclocation = "/usr/bin/gcc";
> +}
> +$preprocessor = $gcclocation . " -E -P -x c++";
>  
>  GetOptions(
>      'tags=s' => \$tagsFile,