Bug 150849

Summary: Teach Makefile to build LayoutTestRelay when building for iOS Simulator
Product: WebKit Reporter: Daniel Bates <dbates>
Component: Tools / TestsAssignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, ddkilzer, jmarcell, lforschler
Priority: P2    
Version: WebKit Local Build   
Hardware: iPhone / iPad   
OS: Unspecified   
Bug Depends on: 150962    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
ap: review+
[Patch] Addendum - Build LayoutTestRelay when make invoked from Tools or top-level checkout ap: review+

Description Daniel Bates 2015-11-03 12:43:24 PST
For convenience, we should support building LayoutTestRelay by default when building WebKit using `make SDKROOT=iphonesimulator`.
Comment 1 Daniel Bates 2015-11-03 12:47:08 PST
Created attachment 264714 [details]
Patch
Comment 2 Alexey Proskuryakov 2015-11-03 13:32:55 PST
Comment on attachment 264714 [details]
Patch

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

> Tools/LayoutTestRelay/Makefile:14
> +ifneq (,$(SAVED_SDK_ROOT))
> +	override SDKROOT = $(SAVED_SDK_ROOT)
> +endif

Should we also unset SAVED_SDK_ROOT here?
Comment 3 Daniel Bates 2015-11-05 13:57:38 PST
(In reply to comment #2)
> Comment on attachment 264714 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264714&action=review
> 
> > Tools/LayoutTestRelay/Makefile:14
> > +ifneq (,$(SAVED_SDK_ROOT))
> > +	override SDKROOT = $(SAVED_SDK_ROOT)
> > +endif
> 
> Should we also unset SAVED_SDK_ROOT here?

Yes, will modify patch to undefine SAVED_SDK_ROOT inside the if block above before landing.
Comment 4 Daniel Bates 2015-11-05 14:01:11 PST
Committed r192073: <http://trac.webkit.org/changeset/192073>
Comment 5 WebKit Commit Bot 2015-11-05 15:14:47 PST
Re-opened since this is blocked by bug 150962
Comment 6 Daniel Bates 2015-11-05 17:38:43 PST
Created attachment 264909 [details]
Patch
Comment 7 Daniel Bates 2015-11-06 08:59:27 PST
Committed r192106: <http://trac.webkit.org/changeset/192106>
Comment 8 Jason Marcell 2015-11-06 14:43:45 PST
Comment on attachment 264909 [details]
Patch

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

> Tools/ChangeLog:11
> +        * LayoutTestRelay/Makefile: Define OVERWRITE_SDKROOT and OVERWRITE_ARCHS.

Nit: OVERWRITE_{ARCHS, SDKROOT} -> OVERRIDE_{ARCHS, SDKROOT}

> Makefile.shared:7
> +	ifneq (,$(OVERRIDE_SDKROOT))

Why is this nested inside the first if block? Don't you want to use the OVERRIDE_SDKROOT regardless of whether SDKROOT was set?

> Makefile.shared:9
> +			XCODE_OPTIONS := $(XCODE_OPTIONS) SDKROOT=$(OVERRIDE_SDKROOT)

Why did you use this syntax instead of the += syntax that was previously used?
Comment 9 Jason Marcell 2015-11-06 14:43:54 PST
Comment on attachment 264909 [details]
Patch

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

> Tools/ChangeLog:11
> +        * LayoutTestRelay/Makefile: Define OVERWRITE_SDKROOT and OVERWRITE_ARCHS.

Nit: OVERWRITE_{ARCHS, SDKROOT} -> OVERRIDE_{ARCHS, SDKROOT}

> Makefile.shared:7
> +	ifneq (,$(OVERRIDE_SDKROOT))

Why is this nested inside the first if block? Don't you want to use the OVERRIDE_SDKROOT regardless of whether SDKROOT was set?

> Makefile.shared:9
> +			XCODE_OPTIONS := $(XCODE_OPTIONS) SDKROOT=$(OVERRIDE_SDKROOT)

Why did you use this syntax instead of the += syntax that was previously used?
Comment 10 Daniel Bates 2015-11-10 20:40:45 PST
(In reply to comment #8)
> Comment on attachment 264909 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264909&action=review
> 
> > Tools/ChangeLog:11
> > +        * LayoutTestRelay/Makefile: Define OVERWRITE_SDKROOT and OVERWRITE_ARCHS.
> 
> Nit: OVERWRITE_{ARCHS, SDKROOT} -> OVERRIDE_{ARCHS, SDKROOT}
> 

Fixed in <http://trac.webkit.org/changeset/192289>.

> > Makefile.shared:7
> > +	ifneq (,$(OVERRIDE_SDKROOT))
> 
> Why is this nested inside the first if block? Don't you want to use the
> OVERRIDE_SDKROOT regardless of whether SDKROOT was set?
> 

Yes, we should respect OVERRIDE_SDKROOT regardless of whether SDKROOT is set. Filed bug #151127 to address this issue.

> > Makefile.shared:9
> > +			XCODE_OPTIONS := $(XCODE_OPTIONS) SDKROOT=$(OVERRIDE_SDKROOT)
> 
> Why did you use this syntax instead of the += syntax that was previously
> used?

Notice that XCODE_OPTIONS is a recursively expanded variable by <https://ftp.gnu.org/old-gnu/Manuals/make-3.79.1/html_chapter/make_6.html#SEC59>. The += operator preserves the flavor of the variable by <https://ftp.gnu.org/old-gnu/Manuals/make-3.79.1/html_chapter/make_6.html#SEC65>. Due to the way Makefiles are executed, we want to expand $OVERRIDE_SDKROOT in the definition of XCODE_OPTIONS immediately (i.e. make XCODE_OPTIONS a simply expanded variable) so that we can clear it and allow subsequent Makefiles to define it again. Therefore, I used the := operator to define XCODE_OPTIONS as a simply-expanded variable. For some reason, I was concerned about unconditionally defining XCODE_OPTIONS as a simply expanded variable and chose to only convert XCODE_OPTIONS to a simply-expanded variable when $(OVERRIDE_SDKROOT) is defined. We should look to unconditionally define XCODE_OPTIONS as a simply expanded variable and then we can use the += operator when appending the expression "SDKROOT=$(OVERRIDE_SDKROOT)".
Comment 11 Daniel Bates 2015-11-10 20:45:01 PST
Re-opening this bug since I inadvertently did not append LayoutTestRelay to the end of the list of modules to build in file Tools/Makefile. So, `make SDKROOT=iphonesimulator` only works when executed inside directory Tools/LayoutTestRelay. We should make `make SDKROOT=iphonesimulator` work when executed from directory Tools and the top-level WebKit checkout.
Comment 12 Daniel Bates 2015-11-10 20:55:24 PST
Created attachment 265266 [details]
[Patch] Addendum - Build LayoutTestRelay when make invoked from Tools or top-level checkout
Comment 13 Daniel Bates 2015-11-10 21:19:58 PST
Committed r192293: <http://trac.webkit.org/changeset/192293>