Bug 26537

Summary: Builds from command-line fail if custom build product directory is set and ~/Library/Preferences/xcodebuild.plist exists
Product: WebKit Reporter: Takeshi Yoshino <tyoshino>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: mrowe
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
Proposed fix for 26537
none
Proposed fix for 26537
mrowe: review-
Proposed fix for 26537 (rev 3)
mrowe: review-
Proposed fix for 26537 (rev 4)
none
Proposed fix for 26537 (rev 5) mrowe: review+

Description Takeshi Yoshino 2009-06-19 03:30:20 PDT
When I set the directory to place build products by Xcode like this,
Xcode -> Preferences -> Building tab -> Place Build Products in -> Customized location -> e.g. /tmp/ramdisk
I got the following error. This happened because build-webkit script didn't set BUILT_PRODUCTS_DIR variable correctly. Output files were generated into each project directories separately, not into the specified directory together.

% time build-webkit --debug
...
    setenv BUILT_PRODUCTS_DIR /Users/tyoshino/wk2_svn/JavaScriptCore/build/Debug
...
CompileC build/JavaScriptGlue.build/Debug/JavaScriptGlue.build/Objects-normal/i386/JSBase.o /Users/tyoshino/wk2_svn/JavaScriptGlue/JSBase.cpp normal i386 c++ com.apple.compilers.gcc.4_2
    cd /Users/tyoshino/wk2_svn/JavaScriptGlue
    /Developer/usr/bin/gcc-4.2 -x c++ -arch i386 -fmessage-length=0 -pipe -Wno-trigraphs -fno-exceptions -fno-rtti -fpascal-strings -fasm-blocks -O0 -Werror -Wnon-virtual-dtor -Wnewline-eof -DWEBKIT_VERSION_MIN_REQUIRED=WEBKIT_VERSION_LATEST -fstrict-aliasing -fvisibility-inlines-hidden -fno-threadsafe-statics -mmacosx-version-min=10.5 -gdwarf-2 -I/Users/tyoshino/wk2_svn/JavaScriptGlue/build/JavaScriptGlue.build/Debug/JavaScriptGlue.build/JavaScriptGlue.hmap -Wall -W -Wcast-align -Wchar-subscripts -Wformat-security -Wmissing-format-attribute -Wpointer-arith -Wwrite-strings -Wno-format-y2k -Wno-unused-parameter -Wundef -Wno-strict-aliasing -Wshorten-64-to-32 -F/Users/tyoshino/wk2_svn/JavaScriptGlue/build/Debug -I/Users/tyoshino/wk2_svn/JavaScriptGlue/build/Debug/include -IForwardingHeaders -I. -Iicu -I/Users/tyoshino/wk2_svn/JavaScriptGlue/build/JavaScriptGlue.build/Debug/JavaScriptGlue.build/DerivedSources -Wno-deprecated-declarations -c /Users/tyoshino/wk2_svn/JavaScriptGlue/JSBase.cpp -o /Users/tyoshino/wk2_svn/JavaScriptGlue/build/JavaScriptGlue.build/Debug/JavaScriptGlue.build/Objects-normal/i386/JSBase.o
In file included from /Users/tyoshino/wk2_svn/JavaScriptGlue/config.h:5,
                 from /Users/tyoshino/wk2_svn/JavaScriptGlue/JSBase.cpp:29:
ForwardingHeaders/wtf/Platform.h:1:37: error: JavaScriptCore/Platform.h: No such file or directory
Comment 1 Takeshi Yoshino 2009-06-19 03:33:29 PDT
Created attachment 31541 [details]
Proposed fix for 26537
Comment 2 Takeshi Yoshino 2009-06-19 03:35:17 PDT
Created attachment 31542 [details]
Proposed fix for 26537

Oops! I included unrelated changes by mistake.
Comment 3 Eric Seidel (no email) 2009-06-21 23:57:27 PDT
Comment on attachment 31542 [details]
Proposed fix for 26537

LGTM.
Comment 4 Mark Rowe (bdash) 2009-06-22 00:11:51 PDT
I don't understand why this is necessary.  I'm aware of a number of people that build with a global built products directory configured in Xcode.  I'd like to understand what is special about your setup that requires this change?
Comment 5 Takeshi Yoshino 2009-06-22 00:19:16 PDT
(In reply to comment #4)
> I don't understand why this is necessary.  I'm aware of a number of people that
> build with a global built products directory configured in Xcode.  I'd like to
> understand what is special about your setup that requires this change?
> 

Do they build using build-webkit script? I can build WebKit on Xcode IDE without this patch, but cannot with build-webkit script when I changed the global built products directory to somewhere else than <Checkout directory>/WebKitBuild.

I just followed the instruction on www.webkit.org. AFAIK, I didn't make any special changes on Xcode settings other than built product. One of my coworker also came across this failure when he tried to build.

Thanks
Comment 6 Mark Rowe (bdash) 2009-06-22 00:22:09 PDT
Comment on attachment 31542 [details]
Proposed fix for 26537

Explicitly setting SYMROOT and OBJROOT shouldn't be necessary.  Xcode derives these values automatically relative to configured build products directory.  While the patch apparently fixes your problem, I think we should understand why you're running in to this when no-one else is.  It makes me suspect that something is just a little off in your Xcode preferences.  Until we understand what, I don't think this should be landed.
Comment 7 Mark Rowe (bdash) 2009-06-22 00:24:20 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > I don't understand why this is necessary.  I'm aware of a number of people that
> > build with a global built products directory configured in Xcode.  I'd like to
> > understand what is special about your setup that requires this change?
> > 
> 
> Do they build using build-webkit script? I can build WebKit on Xcode IDE
> without this patch, but cannot with build-webkit script when I changed the
> global built products directory to somewhere else than <Checkout
> directory>/WebKitBuild.

Almost all of my coworkers at Apple have a custom built product location and build from the command-line (using "make", which gets the arguments to xcodebuild from webkitdirs.pm in a similar fashion to build-webkit).

Do you happen to have a ~/Library/Preferences/xcodebuild.plist?
Comment 8 Takeshi Yoshino 2009-06-22 00:29:08 PDT
(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > I don't understand why this is necessary.  I'm aware of a number of people that
> > > build with a global built products directory configured in Xcode.  I'd like to
> > > understand what is special about your setup that requires this change?
> > > 
> > 
> > Do they build using build-webkit script? I can build WebKit on Xcode IDE
> > without this patch, but cannot with build-webkit script when I changed the
> > global built products directory to somewhere else than <Checkout
> > directory>/WebKitBuild.
> 
> Almost all of my coworkers at Apple have a custom built product location and
> build from the command-line (using "make", which gets the arguments to
> xcodebuild from webkitdirs.pm in a similar fashion to build-webkit).

Hmm, I see.

> 
> Do you happen to have a ~/Library/Preferences/xcodebuild.plist?
> 

Yes

% od -c xcodebuild.plist                                                                                        [~/Library/Preferences]
0000000    b   p   l   i   s   t   0   0 321 001 002   _ 020 037   P   B
0000020    X   A   p   p   l   i   c   a   t   i   o   n   w   i   d   e
0000040    B   u   i   l   d   S   e   t   t   i   n   g   s 320  \b  \v
0000060    -  \0  \0  \0  \0  \0  \0 001 001  \0  \0  \0  \0  \0  \0  \0
0000100  003  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0000120    .                                                            
0000121
Comment 9 Takeshi Yoshino 2009-06-22 00:30:35 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > I don't understand why this is necessary.  I'm aware of a number of people that
> > > > build with a global built products directory configured in Xcode.  I'd like to
> > > > understand what is special about your setup that requires this change?
> > > > 
> > > 
> > > Do they build using build-webkit script? I can build WebKit on Xcode IDE
> > > without this patch, but cannot with build-webkit script when I changed the
> > > global built products directory to somewhere else than <Checkout
> > > directory>/WebKitBuild.
> > 
> > Almost all of my coworkers at Apple have a custom built product location and
> > build from the command-line (using "make", which gets the arguments to
> > xcodebuild from webkitdirs.pm in a similar fashion to build-webkit).
> 
> Hmm, I see.
> 
> > 
> > Do you happen to have a ~/Library/Preferences/xcodebuild.plist?
> > 
> 
> Yes
> 
> % od -c xcodebuild.plist                                                       
>                                 [~/Library/Preferences]
> 0000000    b   p   l   i   s   t   0   0 321 001 002   _ 020 037   P   B
> 0000020    X   A   p   p   l   i   c   a   t   i   o   n   w   i   d   e
> 0000040    B   u   i   l   d   S   e   t   t   i   n   g   s 320  \b  \v
> 0000060    -  \0  \0  \0  \0  \0  \0 001 001  \0  \0  \0  \0  \0  \0  \0
> 0000100  003  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
> 0000120    .                                                            
> 0000121
> 

And, FYI,

% defaults read com.apple.Xcode PBXApplicationwideBuildSettings                                                 [~/Library/Preferences]
{
    SYMROOT = "/tmp/ramdisk/WebKitBuild";
}
Comment 10 Takeshi Yoshino 2009-06-22 00:38:13 PDT
Deleting ~/Library/Preferences/xcodebuild.plist fixed this problem.
I see. This file gave bad parameter to xcodebuild.
Comment 11 Mark Rowe (bdash) 2009-06-22 00:40:48 PDT
> > Do you happen to have a ~/Library/Preferences/xcodebuild.plist?
> > 
> 
> Yes

Delete it!  The problem you're seeing is that xcodebuild is reading preferences from that file when it should be pulling them from com.apple.Xcode.plist, the standard Xcode preferences.  The situations under which xcodebuild.plist gets created are something of a mystery, but appears to be the result of a bug in xcodebuild itself (<rdar://problem/5585899>).

The existence of this bogus preference file is likely to cause other subtle issues beyond the obvious build directory problem, so I don't think your patch is the appropriate workaround.  A more direct approach may be to look for the existence of this preference file and warn the user, or perhaps even automatically remove it.
Comment 12 Takeshi Yoshino 2009-06-22 00:49:40 PDT
(In reply to comment #11)
> > > Do you happen to have a ~/Library/Preferences/xcodebuild.plist?
> > > 
> > 
> > Yes
> 
> Delete it!  The problem you're seeing is that xcodebuild is reading preferences
> from that file when it should be pulling them from com.apple.Xcode.plist, the
> standard Xcode preferences.  The situations under which xcodebuild.plist gets
> created are something of a mystery, but appears to be the result of a bug in
> xcodebuild itself (<rdar://problem/5585899>).
> 
> The existence of this bogus preference file is likely to cause other subtle
> issues beyond the obvious build directory problem, so I don't think your patch
> is the appropriate workaround.  A more direct approach may be to look for the

So, what's the best thing to do in this function? It's confusing that it has unnecessary code for setting baseProductionDirOption here.

> existence of this preference file and warn the user, or perhaps even
> automatically remove it.
> 

OK. I agree with you about this point. I'll send another patch.
Comment 13 Mark Rowe (bdash) 2009-06-22 01:00:56 PDT
(In reply to comment #12)
> (In reply to comment #11)
> > > > Do you happen to have a ~/Library/Preferences/xcodebuild.plist?
> > > > 
> > > 
> > > Yes
> > 
> > Delete it!  The problem you're seeing is that xcodebuild is reading preferences
> > from that file when it should be pulling them from com.apple.Xcode.plist, the
> > standard Xcode preferences.  The situations under which xcodebuild.plist gets
> > created are something of a mystery, but appears to be the result of a bug in
> > xcodebuild itself (<rdar://problem/5585899>).
> > 
> > The existence of this bogus preference file is likely to cause other subtle
> > issues beyond the obvious build directory problem, so I don't think your patch
> > is the appropriate workaround.  A more direct approach may be to look for the
> 
> So, what's the best thing to do in this function? It's confusing that it has
> unnecessary code for setting baseProductionDirOption here.

What unnecessary code are you referring to?  baseProductDirOption is set in two places: one place deals with the situation where Xcode has a global build product directory configured (no extra options necessary), and the other deals with the default behavior of placing the build product directory alongside the project (we override SYMROOT and OBJROOT to force things in to a single directory for all projects). 

It may be that this logic could be cleaner or more explicit, but that is something that would be best done in a separate patch.
Comment 14 Takeshi Yoshino 2009-06-22 01:15:52 PDT
> > So, what's the best thing to do in this function? It's confusing that it has
> > unnecessary code for setting baseProductionDirOption here.
> 
> What unnecessary code are you referring to?  baseProductDirOption is set in two
> places: one place deals with the situation where Xcode has a global build
> product directory configured (no extra options necessary), and the other deals
> with the default behavior of placing the build product directory alongside the
> project (we override SYMROOT and OBJROOT to force things in to a single
> directory for all projects). 
> 
Sorry, I was misunderstanding. I got it. The code is completely correct.

> It may be that this logic could be cleaner or more explicit, but that is
> something that would be best done in a separate patch.
> 

All right. Will do.

Thank you for review, Mark and Eric.
Comment 15 Takeshi Yoshino 2009-06-22 01:45:50 PDT
Created attachment 31638 [details]
Proposed fix for 26537 (rev 3)

A new approach to fix this issue following Mark's suggestion.
Comment 16 Darin Adler 2009-06-22 08:36:18 PDT
Comment on attachment 31638 [details]
Proposed fix for 26537 (rev 3)

I think we could go further and just remove that file!

r=me
Comment 17 Mark Rowe (bdash) 2009-06-22 08:37:32 PDT
Comment on attachment 31638 [details]
Proposed fix for 26537 (rev 3)

This logic should go in webkitdirs.pm, near where we determine whether we need to add any custom options to xcodebuild in relation to the build product directory.  That code is also used by the Makefiles, so it would be good to have the workaround affect both build-webkit and the Makefiles.  I also think that removing the file silently if it exists is a better option, as we're just asking the user to do something we can trivially do ourselves.  In addition, it'd be great if you could update the comment to refer to the Radar number I mentioned, so that in the future we can quickly determine why the workaround exists and if it is still needed.  Something like the following should suffice:

   # The presence of ~/Library/Preferences/xcodebuild.plist can prevent xcodebuild from respecting global settings such as a custom build products directory (<rdar://problem/5585899>).
Comment 18 Takeshi Yoshino 2009-06-22 20:58:32 PDT
Created attachment 31702 [details]
Proposed fix for 26537 (rev 4)
Comment 19 Takeshi Yoshino 2009-06-22 21:04:14 PDT
Created attachment 31703 [details]
Proposed fix for 26537 (rev 5)
Comment 20 Takeshi Yoshino 2009-06-22 21:07:09 PDT
(In reply to comment #16)
> (From update of attachment 31638 [details] [review])
> I think we could go further and just remove that file!
> 
> r=me
> 

OK. Done.

(In reply to comment #17)
> (From update of attachment 31638 [details] [review])
> This logic should go in webkitdirs.pm, near where we determine whether we need
> to add any custom options to xcodebuild in relation to the build product
> directory.  That code is also used by the Makefiles, so it would be good to
> have the workaround affect both build-webkit and the Makefiles.  I also think
> that removing the file silently if it exists is a better option, as we're just
> asking the user to do something we can trivially do ourselves.  In addition,
> it'd be great if you could update the comment to refer to the Radar number I
> mentioned, so that in the future we can quickly determine why the workaround
> exists and if it is still needed.  Something like the following should suffice:
> 
>    # The presence of ~/Library/Preferences/xcodebuild.plist can prevent
> xcodebuild from respecting global settings such as a custom build products
> directory (<rdar://problem/5585899>).
> 

Done. Please take a look again.

Thanks,
Comment 21 Mark Rowe (bdash) 2009-06-23 01:11:44 PDT
Comment on attachment 31703 [details]
Proposed fix for 26537 (rev 5)

This looks fine to me.  Lines in the ChangeLog and containing comments appear to be wrapped at a very narrow width, making them hard to read.  Whomever lands the patch may want to reflow them at a more reasonable length.
Comment 22 Eric Seidel (no email) 2009-06-23 18:52:40 PDT
Sigh.  Script failure r45032.
Comment 23 Takeshi Yoshino 2009-06-24 22:11:16 PDT
(In reply to comment #22)
> Sigh.  Script failure r45032.
> 

Thank you.