Bug 212420 – Make generate-unified-sources.sh not depend on features being listed in FEATURE_DEFINES environment variable

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Darin Adler]

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.

[Darin Adler]

Unfortunately I don’t know much about Ruby so this may turn out to be a difficult task for me.

[Darin Adler]

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?

[Darin Adler]

Or move the #if inside the .cpp files and not have any conditionals in the Sources.txt files at all.

[Darin Adler]

Any objections to my removing all the #if from the Sources.txt files? There are 16 of them. Would this harm build times?

[Keith Miller]

(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.

[David Kilzer (:ddkilzer)]

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.

[Darin Adler]

Created attachment 400479 [details]
Patch

[Darin Adler]

Created attachment 400483 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

Created attachment 400496 [details]
Patch

[Keith Rollin]

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?

[Darin Adler]

(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.

[Keith Rollin]

(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.

[Darin Adler]

(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.

[Darin Adler]

Created attachment 400525 [details]
Patch

[Darin Adler]

(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.

[Darin Adler]

This is ready to go, and seems to work very well now. Just looking for a reviewer for this final version.

[Andy Estes]

Comment on attachment 400525 [details]
Patch

r=me, but cq- since bug #212451 was reopened.

[Darin Adler]

OK, now bug 212451 is re-fixed so this can go in.

[Darin Adler]

Committed r262310: <https://trac.webkit.org/changeset/262310>

[Radar WebKit Bug Importer]

<rdar://problem/63768339>

[Alexey Proskuryakov]

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.

[Darin Adler]

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.

[Andy Estes]

Reopening because r262310 was reverted in r262462: <https://trac.webkit.org/changeset/262462>.

[Darin Adler]

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?

[Keith Rollin]

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.)

[Darin Adler]

(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.

[Keith Rollin]

(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.

[Darin Adler]

(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.

[Darin Adler]

Created attachment 403506 [details]
Patch

[Darin Adler]

Turns out I was able to remove all the conditionals, and now resolving this is easier than I thought it would be. Hooray!

[Darin Adler]

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!

[Darin Adler]

Committed r263935: <https://trac.webkit.org/changeset/263935>

[Darin Adler]

Oops, missed the Tools directory somehow.

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