Bug 212420

Summary: Make generate-unified-sources.sh not depend on features being listed in FEATURE_DEFINES environment variable
Product: WebKit Reporter: Darin Adler <darin>
Component: Tools / TestsAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, annulen, ap, cdumez, ddkilzer, don.olmstead, ews-watchlist, glenn, gyuyoung.kim, hi, jbedard, keith_miller, krollin, mark.lam, msaboff, ryanhaddad, ryuan.choi, saam, sam, sergio, thorton, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=212500
Bug Depends on: 212451, 212664, 213935, 213936, 213937, 213939, 213940, 213941, 213943, 213944, 213945    
Bug Blocks: 212418, 212542    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch don.olmstead: review+

Description Darin Adler 2020-05-27 11:09:52 PDT
Right now generate-unified-sources.sh depends on the FEATURE_DEFINES environment variable, and this is the only part of our build system that does. Instead it should preprocess Platform.h and get them that way.
Comment 1 Darin Adler 2020-05-27 11:10:31 PDT
Unfortunately I don’t know much about Ruby so this may turn out to be a difficult task for me.
Comment 2 Darin Adler 2020-05-27 11:13:05 PDT
I think we can switch to an approach where we generate #if checks inside the unified .cpp files rather than omitting the source files entirely. Does that sound OK?
Comment 3 Darin Adler 2020-05-27 11:14:10 PDT
Or move the #if inside the .cpp files and not have any conditionals in the Sources.txt files at all.
Comment 4 Darin Adler 2020-05-27 11:25:49 PDT
Any objections to my removing all the #if from the Sources.txt files? There are 16 of them. Would this harm build times?
Comment 5 Keith Miller 2020-05-27 11:37:23 PDT
(In reply to Darin Adler from comment #4)
> Any objections to my removing all the #if from the Sources.txt files? There
> are 16 of them. Would this harm build times?

I don't think it would be problematic and I doubt it would matter for build times. The other plus side of this is that we have less combinations of unified sources to worry about.
Comment 6 David Kilzer (:ddkilzer) 2020-05-27 18:52:04 PDT
FYI, Devin Rousso recently added some C pre-processing in r262203 for Bug 210014 in Source/JavaScriptCore/DerivedSources.make, which caused Bug 212436.

Very different medium to run the C pre-processor, but likely a similar set of issues to solve.
Comment 7 Darin Adler 2020-05-28 10:54:11 PDT
Created attachment 400479 [details]
Patch
Comment 8 Darin Adler 2020-05-28 11:07:14 PDT
Created attachment 400483 [details]
Patch
Comment 9 Darin Adler 2020-05-28 12:07:25 PDT
This isn’t working because there are simply too many edge cases, things like Apple’s WebKitAdditions.

I think instead I will focus on getting the correct list of features passed in to generate-unified-sources.sh, which means invoking it from DerivedSources.make.
Comment 10 Darin Adler 2020-05-28 12:53:38 PDT
Created attachment 400496 [details]
Patch
Comment 11 Keith Rollin 2020-05-28 15:28:43 PDT
I think you should include a comment on the problem you're trying to solve. *Why* are you trying to make generate-unified-sources.sh not depend on FEATURE_DEFINES? How do your changes achieve that goal? I see you passing in --feature-flags to generate-unified-sources.sh -- couldn't that have been done without all these other changes?

It looks like you're simplifying the generate-unified-sources process by incorporating it in the DerivedSources step. If possible, it would be great to be consistent about that approach and apply it to the other projects that have a DerivedSources step. And even if a particular project doesn't have one, perhaps add one for this purpose. Having a consistent approach would be good.

There is also a build step ("Check .xcfilelists") that checks that .xcfilelist files are up-to-date. I would have expected to see changes in that area now that WebCore's Generate Unified Sources step has been removed, along with its associated .xcfilelist. Is the build not checking that (non-existent) .xcfilelist is up-to-date and trying to re-generate it?
Comment 12 Darin Adler 2020-05-28 16:30:04 PDT
(In reply to Keith Rollin from comment #11)
> I think you should include a comment on the problem you're trying to solve.

Will do. I wrote a paragraph for the change log:

        Before this, any #if in the Sources.txt and SourcesCocoa.txt files can
        check only features defined in the FeatureDefines.xcconfig file,
        which sets up the FEATURE_DEFINES environment variable. Instead,
        we'd like to pass in all the things defined in the Platform.h headers
        as well. We accomplish that using the FEATURE_AND_PLATFORM_DEFINES
        variable from the DerivedSources.make file. This script was the
        last place using FEATURE_DEFINES directly, so it frees us up to reduce
        FeatureDefines.xcconfig and move feature definitions to
        PlatformEnableCocoa.h instead, which will be less repetitive.

> It looks like you're simplifying the generate-unified-sources process by
> incorporating it in the DerivedSources step. If possible, it would be great
> to be consistent about that approach and apply it to the other projects that
> have a DerivedSources step. And even if a particular project doesn't have
> one, perhaps add one for this purpose. Having a consistent approach would be
> good.

I am moving it into DerivedSources.make so I can reuse FEATURE_AND_PLATFORM_DEFINES. Otherwise I don’t particularly think this is an improvement and I would not have made that change if I didn’t have that reason.

I understand your consistency argument, but this unwanted complexity of conditional source files based on features is currently specific to WebCore and I don’t want to spread it further. A better long term direction is to remove these conditionals, but it was too hard for me to do that (I tried!).

> There is also a build step ("Check .xcfilelists") that checks that
> .xcfilelist files are up-to-date. I would have expected to see changes in
> that area now that WebCore's Generate Unified Sources step has been removed,
> along with its associated .xcfilelist. Is the build not checking that
> (non-existent) .xcfilelist is up-to-date and trying to re-generate it?

I’ll check on that. But I don’t think there’s anything to do here. I didn’t see any explicit reference to the UnifiedSources-output.xcfilelist file, nor was the file recreated. The script did update the DerivedSources-input.xcfilelist and DerivedSources-output.xcfilelist files, and those changes are included in the patch.
Comment 13 Keith Rollin 2020-05-28 16:51:34 PDT
(In reply to Darin Adler from comment #12)
> I’ll check on that. But I don’t think there’s anything to do here. I didn’t
> see any explicit reference to the UnifiedSources-output.xcfilelist file, nor
> was the file recreated. The script did update the
> DerivedSources-input.xcfilelist and DerivedSources-output.xcfilelist files,
> and those changes are included in the patch.

The file OpenSource/Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py contains the following:

class WebCoreGenerator(BaseGenerator):
    ...
    @util.LogEntryExit
    def _get_generate_unified_sources_script(self):
        return os.path.join(self._get_project_dir(), "Scripts", "generate-unified-sources.sh")

That _get_generate_unified_sources_script function could be removed. I'm surprised it's not generating at least an empty file, but maybe I made allowances for empty files that I forgot about.

From what you say, removing that code doesn't seem necessary, but maybe it should just as part of cleanup.
Comment 14 Darin Adler 2020-05-28 16:56:47 PDT
(In reply to Keith Rollin from comment #13)
> The file
> OpenSource/Tools/Scripts/webkitpy/generate_xcfilelists_lib/generators.py
> contains the following:
> 
> class WebCoreGenerator(BaseGenerator):
>     ...
>     @util.LogEntryExit
>     def _get_generate_unified_sources_script(self):
>         return os.path.join(self._get_project_dir(), "Scripts",
> "generate-unified-sources.sh")
> 
> That _get_generate_unified_sources_script function could be removed.

Thanks for the pointer. I removed it.
Comment 15 Darin Adler 2020-05-28 17:06:29 PDT
Created attachment 400525 [details]
Patch
Comment 16 Darin Adler 2020-05-28 17:26:26 PDT
(In reply to Keith Rollin from comment #13)
> I'm surprised it's not generating at least an empty file

I was wrong. It was regenerating the file, and that change seems to have fixed it.
Comment 17 Darin Adler 2020-05-29 08:49:12 PDT
This is ready to go, and seems to work very well now. Just looking for a reviewer for this final version.
Comment 18 Andy Estes 2020-05-29 11:22:54 PDT
Comment on attachment 400525 [details]
Patch

r=me, but cq- since bug #212451 was reopened.
Comment 19 Darin Adler 2020-05-29 12:02:22 PDT
OK, now bug 212451 is re-fixed so this can go in.
Comment 20 Darin Adler 2020-05-29 12:04:31 PDT
Committed r262310: <https://trac.webkit.org/changeset/262310>
Comment 21 Radar WebKit Bug Importer 2020-05-29 12:05:18 PDT
<rdar://problem/63768339>
Comment 22 Alexey Proskuryakov 2020-06-03 16:00:06 PDT
Comment on attachment 400525 [details]
Patch

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

> Source/WebCore/DerivedSources.make:1228
> +UnifiedSourceBundlesTimeStamp.txt : $(WebCore)/Scripts/generate-unified-sources.sh Sources.txt SourcesCocoa.txt $(FEATURE_AND_PLATFORM_DEFINE_DEPENDENCIES)

I would expect this to depend on the set of source files being unified, not just on the scripts.
Comment 23 Darin Adler 2020-06-03 16:21:22 PDT
Comment on attachment 400525 [details]
Patch

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

>> Source/WebCore/DerivedSources.make:1228
>> +UnifiedSourceBundlesTimeStamp.txt : $(WebCore)/Scripts/generate-unified-sources.sh Sources.txt SourcesCocoa.txt $(FEATURE_AND_PLATFORM_DEFINE_DEPENDENCIES)
> 
> I would expect this to depend on the set of source files being unified, not just on the scripts.

Yes, it does. The set of source files being unified is a list  in Sources.txt and SourcesCocoa.txt. And those files are dependencies.
Comment 24 Andy Estes 2020-06-04 12:10:45 PDT
Reopening because r262310 was reverted in r262462: <https://trac.webkit.org/changeset/262462>.
Comment 25 Darin Adler 2020-06-14 14:21:34 PDT
So when doing this again, I think I will do this instead of what I did last time:

1) Remove most of the conditionals from Sources.txt and SourcesCocoa.txt; long term it would be best to not use conditionals there at all.

2) Modify generate-unified-sources.sh to do the same compiling steps that DerivedSources.make does to expand the entire set of feature defines from the Platform.h headers.

3) I won’t change it so that generate-unified-sources.sh is invoked by the makefile. Even though I find the idea appealing and it’s more efficient to not compile an extra time, it’s nice to sidestep all the tricky issues with .xcfilelist by not changing that.

4) Remove the feature defines from .xcconfig (as I did before but we had to revert). Many steps, some of them already tracked by other bugs.

5) Maybe long-term as a clean-up eliminate all the rest of the conditionals entirely from Sources.txt files and remove all attempts to pass feature defines in.

Keith, any feedback on that?
Comment 26 Keith Rollin 2020-06-15 00:06:25 PDT
I hadn't really gone over the problem you were trying to solve before, nor your solution to it. So this time I did, reading all the comments and understanding the problem. I then asked myself how I would solve the problem. After that, I read your proposed solution. I was happy to see that I pretty much arrived at the same approach as you.

The only difference I came up with concerns checking for flags from Platform.h. DerivedSources.make preprocesses that file several times (at least six), pulling out the value of one flag each time. I think it would be good to see if we could replace that process with a single script that's more efficient. This script would preprocess Platform.h if needed, writing the output to a file in DerivedSources. This file could then be grepped for the desired value, skipping the compilation exception on full builds or if one of the Platform*.h files changed. The script would be called six times (or so) from DerivedSources.make, but it would be much more efficient than what we have now.

get-platform-feature-flag:
--------------------------
make Platform.make
grep .../DerivedSources/Platform.preprocessed.h "$1"

Platform.make
-------------
.../DerivedSources/Platform.preprocessed.h : Platform.make Platform.h ...
    $(CC) -std=gnu++1z -x c++ -E -P -dM $(SDK_FLAGS) \
        $(TARGET_TRIPLE_FLAGS) $(FRAMEWORK_FLAGS) $(HEADER_FLAGS) \
        -include "wtf/Platform.h" /dev/null

With get-platform-feature-flag in place, you could invoke it from Ruby to get the values you need for processing Sources*.txt.

It would be nice if these new files could be in WTF and exported like generate-unified-sources.sh so that other projects could use the same approach to fetching flags out of Platform*.h. (Checking, it looks like only one flag is fetched in WebKit/DerivedSources.make, so it doesn't look like the need for this is actually great.)
Comment 27 Darin Adler 2020-06-15 08:29:49 PDT
(In reply to Keith Rollin from comment #26)
> DerivedSources.make preprocesses that file several times (at
> least six), pulling out the value of one flag each time.

I believe that one of my patches fixes that so that each DerivedSources.make does it only once. Previously the WebCore one did it more than once. But if not, I should re-do that again; it was pretty easy to do.

But it's true that each project's DerivedSources.make does it, so maybe that's where the "at least 6" comes from?

I think it’s fine to optimize that by assuming that WTF is compiled with the identical flags to all the other projects. Especially if this has a measurable effect on build times. I think it might be fast enough, though, compared to the rest of the build even when building nothing, that this may not be an important optimization. We should look into that.

I guess the idea of doing this in WTF, though, could simplify the generate-unified-sources.sh work, since it would just be reading a file, not invoking the compiler.
Comment 28 Keith Rollin 2020-06-15 14:43:01 PDT
(In reply to Darin Adler from comment #27)
> (In reply to Keith Rollin from comment #26)
> > DerivedSources.make preprocesses that file several times (at
> > least six), pulling out the value of one flag each time.
> 
> I believe that one of my patches fixes that so that each DerivedSources.make
> does it only once. Previously the WebCore one did it more than once. But if
> not, I should re-do that again; it was pretty easy to do.
> 
> But it's true that each project's DerivedSources.make does it, so maybe
> that's where the "at least 6" comes from?

I was looking at a branch that was last updated Mar 22, and so missed seeing your update. Sorry about that.

> I guess the idea of doing this in WTF, though, could simplify the
> generate-unified-sources.sh work, since it would just be reading a file, not
> invoking the compiler.

I was thinking that the proposed script and Makefile would live in WTF, and be exported so that other projects could use it. I wasn't thinking that Platform.preprocessed.h would be created when building the WTF project. But your approach seems better if, as you say, the build environment is essentially the same between WTF and the other projects (and, really, it should be -- I don't think we should have flags receive different values in different projects).

But mostly I was thinking that the $CC command for generating the preprocessed file looks nasty, and that propagating that nastiness to the Ruby file was the wrong direction, and that the right direction would be to encapsulate it in one location.
Comment 29 Darin Adler 2020-07-03 14:49:21 PDT
(In reply to Darin Adler from comment #25)
> 1) Remove most of the conditionals from Sources.txt and SourcesCocoa.txt;
> long term it would be best to not use conditionals there at all.

I am working on this now.
Comment 30 Darin Adler 2020-07-03 21:23:25 PDT
Created attachment 403506 [details]
Patch
Comment 31 Darin Adler 2020-07-03 21:24:43 PDT
Turns out I was able to remove all the conditionals, and now resolving this is easier than I thought it would be. Hooray!
Comment 32 Darin Adler 2020-07-03 22:49:18 PDT
Looks like the tests are all passing. While the bot doesn’t check that this works on Apple internal SDK builds, seems highly likely that’s working as expected, too. Ready to go and looking for a reviewer!
Comment 33 Darin Adler 2020-07-04 09:41:51 PDT
Committed r263935: <https://trac.webkit.org/changeset/263935>
Comment 34 Darin Adler 2020-07-04 10:20:15 PDT
Oops, missed the Tools directory somehow.

Committed r263937: <https://trac.webkit.org/changeset/263937>