Summary: | Teach Makefile to build LayoutTestRelay when building for iOS Simulator | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||||
Component: | Tools / Tests | Assignee: | 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
Daniel Bates
2015-11-03 12:43:24 PST
Created attachment 264714 [details]
Patch
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? (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. Committed r192073: <http://trac.webkit.org/changeset/192073> Re-opened since this is blocked by bug 150962 Created attachment 264909 [details]
Patch
Committed r192106: <http://trac.webkit.org/changeset/192106> 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 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? (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)". 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. Created attachment 265266 [details]
[Patch] Addendum - Build LayoutTestRelay when make invoked from Tools or top-level checkout
Committed r192293: <http://trac.webkit.org/changeset/192293> |