Bug 218378 - Further lessen reliance on VPATH in WebCore/DerivedSources.make
Summary: Further lessen reliance on VPATH in WebCore/DerivedSources.make
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Rollin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-30 02:19 PDT by Keith Rollin
Modified: 2020-11-03 00:35 PST (History)
5 users (show)

See Also:


Attachments
Patch (13.25 KB, patch)
2020-10-30 02:22 PDT, Keith Rollin
no flags Details | Formatted Diff | Diff
Patch (13.18 KB, patch)
2020-10-30 12:36 PDT, Keith Rollin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Rollin 2020-10-30 02:19:47 PDT
Bug 217696 updated WebCore/DerivedSources.make to rely less on VPATH and make more use of explicit partial or full paths. The solution there, however, did not go far enough, and led to failures when building WebKit for Safari Tech Preview and using old SDKs that contains files that have since been "upstreamed" into WebKit. Address this problem by taking further control of how DerivedSources.make finds needed files instead of using the VPATH mechanism.
Comment 1 Keith Rollin 2020-10-30 02:19:57 PDT
<rdar://problem/70730895>
Comment 2 Keith Rollin 2020-10-30 02:22:08 PDT
Created attachment 412725 [details]
Patch
Comment 3 Alexey Proskuryakov 2020-10-30 10:13:54 PDT
Comment on attachment 412725 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        No new tests -- this is a build fix.

I think that this is the right approach to addressing the regression causing a build failure. However, it makes me concerned about general case. The old SDK can have old copies of other files, not just IDLs. For example, we cannot defend against the compiler finding headers in a wrong directory.

In a build system that modifies the SDK as it goes, the comprehensive solution would be to clean the SDK before building, and that's already done in some situations - I'm actually somewhat surprised that we ran into this where we did. But I'm not certain on how any potential solutions for local engineering builds could look like.
Comment 4 Darin Adler 2020-10-30 11:16:40 PDT
Alexey, your comments make sense. But why didn’t you also set review+?
Comment 5 Alexey Proskuryakov 2020-10-30 11:53:18 PDT
I just don't know makefile syntax enough to r+ anything bu most trivial patches. I have no objections.
Comment 6 Darin Adler 2020-10-30 12:26:13 PDT
Comment on attachment 412725 [details]
Patch

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

> Source/WebCore/DerivedSources.make:1315
> +    $(sort \
> +        $(foreach \

I think putting sort and foreach on separate lines is perhaps overdoing it a bit. I’m OK with this sort of vertical formatting if it makes things clearer but I’m not sure it’s having that effect here, and especially not for the sort line vs. the foreach lines.

It might be hard to see the structure in one super-long line, but we should explore other ways to do that. I think this one might be *easier* to read as one line.
Comment 7 Keith Rollin 2020-10-30 12:36:34 PDT
Created attachment 412788 [details]
Patch
Comment 8 Keith Rollin 2020-10-30 12:37:52 PDT
Made Darin's suggested change.
Comment 9 EWS 2020-10-30 13:34:34 PDT
Committed r269206: <https://trac.webkit.org/changeset/269206>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 412788 [details].