Bug 35213 – [chromium] hardcoded gcc path breaks solaris build

• Component: WebCore Misc.
• Status: RESOLVED FIXED

Created attachment 49152 [details]
patches to fix path on solaris

http://codereview.chromium.org/650016

There is no /usr/bin/gcc in Solaris and SXCE (Opensolaris has a symlink). Referring here.

These patches fix the issue, but a long-term solution is to fix the preprocessor FIXME, as commented in the code.

Created attachment 49158 [details]
revised patch

patch made with svn-create-patch script

Created attachment 49162 [details]
revised patch with reedited ChangeLog

fixed and edited flags

[Darin Adler]

Comment on attachment 49162 [details]
revised patch with reedited ChangeLog

Can't land this because there are tabs in the change log.

This bug title says "hardcoded gcc path breaks solaris build", but tis patch has nothing to do with that.

[Evan Martin]

Comment on attachment 49162 [details]
revised patch with reedited ChangeLog

r- based on Darin's comments.  It looks like patch #3 has different changes than patch #2.

-    open FILE, "-|", "/usr/bin/gcc", "-E", "-P", "-x", "objective-c", 
-        (map { "-D$_" } split(/ +/, $defines)), "-DOBJC_CODE_GENERATION", $fileName or die "Could not open $fileName";
+    if (($Config::Config{'osname'})=~/solaris/i){
+        open FILE, "-|", "/usr/sfw/bin/gcc", "-E", "-P", "-x", "objective-c", 
+            (map { "-D$_" } split(/ +/, $defines)), "-DOBJC_CODE_GENERATION", $fileName or die "Could not open $fileName";
+    } else {
+        open FILE, "-|", "/usr/bin/gcc", "-E", "-P", "-x", "objective-c", 
+            (map { "-D$_" } split(/ +/, $defines)), "-DOBJC_CODE_GENERATION", $fileName or die "Could not open $fileName";
+    }

Rather than duplicate code, why not factor out the gcc path into a variable?

Created attachment 49206 [details]
reuploading patch

For some reason the wrong patch was uploaded. Just great,

The previous patch was actually https://bugs.webkit.org/show_bug.cgi?id=35214 which made its way onto here for some reason. 

@Evan There was a FIXME describing that exact thing. Based on the fact that it's from 2007 and nobody has done it, I figured there was some complicated reason why, hence why I'm leaving it someone more familiar with WebKit, instead of risking something breaking. (I don't have access to a linux machine where I can build WebKit).

Created attachment 49208 [details]
fix formatting

fix formatting in ChangeLog

Created attachment 49209 [details]
formatting fix again

clicked the checkbox with "patch" written inside. Great.

Is there a command line alternative to the web interface? My browser is being exciting.

[Eric Seidel (no email)]

This patch fails to apply (as you can see by the purple bubbles below the patch).  Click on any one of the bubbles to see why.

Created attachment 49255 [details]
fix changelog formatting, again

Previous patch to changelog was against my local copy, which had other changes I've made but haven't been committed. Updated to newest svn, triple checked the patch content again. Hope it works this time...

anyone?

[David Levin]

Comment on attachment 49255 [details]
fix changelog formatting, again

> Index: ChangeLog
> [phone]  James Choi  <[e-mail]>
> +
> +        Change hardcoded gcc paths to be Solaris friendly
> +        https://bugs.webkit.org/show_bug.cgi?id=35214
> +
> +        * WebCore/bindings/scripts/CodeGeneratorObjC.pm, WebCore/bindings/scripts/IDLParser.pm, WebCore/css/make-css-file-arrays.pl, WebCore/dom/make_names.pl

Each file typically has its own line with a * before it. I imaging that is how prepare-ChangeLog generated it but this looks odd to me.


> Index: WebCore/bindings/scripts/CodeGeneratorObjC.pm

In each of these changes, the line containing gcc has been duplicated completely. It would be better if the change was only about the gcc location. For example,

    my $gccLocation = "";
    if (($Config::Config{'osname'}) =~ /solaris/i) {
        $gccLocation = "/usr/sfw/bin/gcc";
    } else {
        $gccLocation = "/usr/sfw/bin/gcc";
    }
    open FILE, "-|", $gccLocation, "-E", "-P", "-x", "objective-c", 
        (map { "-D$_" } split(/ +/, $defines)), "-DOBJC_CODE_GENERATION", $fileName or die "Could not open $fileName";


Also please note the spaces around the =~ and before the {.

[David Levin]

(In reply to comment #4)
> Rather than duplicate code, why not factor out the gcc path into a variable?

I should have just said (again) r- for what Mr. Martin said. :)

Created attachment 49416 [details]
seperate into new variable, reformatting again

[David Levin]

Comment on attachment 49416 [details]
seperate into new variable, reformatting again

> 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,

Created attachment 49488 [details]
even more formatting edits

More formatting things.

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

Could you explain? :)

[David Levin]

Comment on attachment 49488 [details]
even more formatting edits

I'll do these fixes and land it.


There should be spaces around the ~= as noted previously but I missed this in the last review :(.

> Index: WebCore/dom/make_names.pl

This could be slightly simpler my moving where preprocessor is declared.

> +require Config;
> +my $gccLocation = "";
> +if (($Config::Config{'osname'})=~/solaris/i) {
> +    $gccLocation = "/usr/sfw/bin/gcc";
> +} else {
> +    $gccLocation = "/usr/bin/gcc";
> +}
> +my $preprocessor = $gcclocation . " -E -P -x c++";

Something I didn't mention before: I think the comment "Detect OS. ..." is redundant with the code (-- It is a translation of the code into English so it doesn't seem useful), so it should be removed.

[WebKit Review Bot]

Attachment 49488 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/308746

[David Levin]

Two more things I noticed. 

1. gccLocation wasn't done consistently which resulted in the busted build noted above. (It is gcclocation in places still.)
2. The ChangeLog entry was in ./ChangeLog instead of WebCore/ChangeLog which is rather odd.  In the future use prepare-ChangeLog to create this and it will do the right thing (I just tried it to verify that).

I'll fix both of these while landing too.

All good points. Thank you for your help :)

[David Levin]

Committed as http://trac.webkit.org/changeset/55240