[webkit-reviews] review granted: [Bug 233304] Add missing dependencies for <wtf/Platform.h> when generating derived sources : [Attachment 444634] Patch v1

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted David Kilzer (:ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 233304: Add missing dependencies for <wtf/Platform.h> when generating
derived sources
https://bugs.webkit.org/show_bug.cgi?id=233304

Attachment 444634: Patch v1

https://bugs.webkit.org/attachment.cgi?id=444634&action=review




--- Comment #5 from Darin Adler <darin at apple.com> ---
Comment on attachment 444634
  --> https://bugs.webkit.org/attachment.cgi?id=444634
Patch v1

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

I’ll say r+ even though I have questions, assuming the answers are all good

> Source/JavaScriptCore/ChangeLog:3
> +	   Add missing dependencies for <wtf/Platform.h> when generating
derived sources

Hard coding Platorm.h doesn’t seem exactly right. But I suppose it’s better
than missing these dependencies. Maybe it’s really literally just Platform.h
though, in which case it’s fine.

> Source/WebCore/ChangeLog:21
> +	   - Add an input depdency on the script run from the build phase

This dependency only mentions the main script source files themselves. The
scripts don’t in turn depend on any additional files? Our perl and python
scripts typically do but maybe these shell scripts are simpler than that?

Also typo: “depdency”

Comments apply to the other similar case above too.

> Source/JavaScriptCore/DerivedSources-input.xcfilelist:2
>
+$(BUILT_PRODUCTS_DIR)/usr/local/include/WebKitAdditions/AdditionalFeatureDefin
es.h

How can this be right? These files are installed as part of JavaScriptCore
build, aren’t they? Later than this. So how can they be inputs? Or is this all
installed as part of building other projects earlier?

If you understand why this is right, that’s good. To me it looks obviously
wrong, but this could just be me getting educated.