Bug 192131 – [Cocoa] Add some WKA extension points

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andy Estes]

[Cocoa] Add some WKA extension points

[Andy Estes]

Created attachment 355960 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/46330293>

[Andy Estes]

Created attachment 355963 [details]
Patch

[EWS Watchlist]

Attachment 355963 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/ApplePay/WebPaymentCoordinatorProxy.h:159:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebKit/UIProcess/ApplePay/WebPaymentCoordinatorProxy.h:162:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 355963 [details]
Patch

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

> Source/WebCore/DerivedSources.make:1152
> +-include WebCoreDerivedSourcesAdditions.make

Ugh, I don’t think that would be easy to make work with XCBuild. Maybe even impossible.

[Alexey Proskuryakov]

Please check with Keith before landing.

[Keith Rollin]

With XCBuild, we need to specify the set of input files that are consumed by a Run Script build phase and the set output files that are generated by the build phase. These lists of files are provided in .xcfilelist files.

When considering the input files, it's usually OK for the set to be over-specified. That is, it's OK if there are files in the list that aren't actually used/referenced in the build phase. This can happen, for example, if you're doing a Mac build and there are iOS-only files in the set of input files. If there are such files, and one or more of those files are touched, then this means that the build phase will be unnecessarily invoked. However, the run script build phases usually protect against that. In the case of Generate Derived Sources, the DerivedSources.make will be invoked, but it will find out that the none of the dependents of the targets are actually dirty, and no rules will be executed.

When considering the output files, it's also OK to be over-specified. What this would mean is that there might be some specified files that aren't actually created. These files will always be considered dirty -- even on subsequent incremental builds -- because they don't exist and so the build phase will always run. But, again, the run script build phases tend to protect against that.

In order to generate the .xcfilelist files, we have a script that determines their contents. In the case of Generate Derived Sources build phases, the .xcfilelist files are generated by making use of dependency information printed when invoking `make -f DerivedSources.make --debug`.

Given this, we consider what should happen with optionally included sub-make files. The best approach here is to make sure that they get included when the .xcfilelist files are generated. For people doing builds in an environment where WebCoreDerivedSourcesAdditions.make exists, this should be OK, and the maximally-specified .xcfilelist files should be generated as desired. For people who don't have WebCoreDerivedSourcesAdditions.make in their environment, they'll end up generating .xcfilelist files that don't reflect the input/output files referenced in WebCoreDerivedSourcesAdditions.make, and the .xcfilelist files will be under-specified.

I note that we have the same issue in WebKit:

WebKit/DerivedSources.make
205:-include WebKitDerivedSourcesAdditions.make

So this problem needs to be solved for both libraries. I don't know what the solution is off the top of my head. I'll think about it, but I'm also open to suggestions.

[Andy Estes]

(In reply to Keith Rollin from comment #7)
> For people who don't have WebCoreDerivedSourcesAdditions.make in
> their environment, they'll end up generating .xcfilelist files that don't
> reflect the input/output files referenced in
> WebCoreDerivedSourcesAdditions.make, and the .xcfilelist files will be
> under-specified.

I think I was following you until this part. If WebCoreDerivedSourcesAdditions.make were missing, then how would the .xcfilelist be underspecified? A non-existent WebCoreDerivedSourcesAdditions.make would have no inputs or outputs.

[Keith Rollin]

(In reply to Andy Estes from comment #8)
> I think I was following you until this part. If
> WebCoreDerivedSourcesAdditions.make were missing, then how would the
> .xcfilelist be underspecified? A non-existent
> WebCoreDerivedSourcesAdditions.make would have no inputs or outputs.

One thing that I failed to mention was that the .xcfilelist files need to exist at the start of the build. As such, they can't be generated on-the-fly by a build step. This means that these files need to be checked into source-control. And this, in turn, means that if someone does something to affect their contents -- such as add a new file to DerivedSources.make -- they the .xcfilelist files need to be regenerate and checked into source control.

I'm thinking about the case where someone who doesn't have WebCoreDerivedSourcesAdditions.make is in a position where they need to regenerate the .xcfilelist files. They can run the script to do the regeneration, but since the WebCoreDerivedSourcesAdditions.make is missing, its files won't be included in the resulting .xcfilelist files. Those .xcfileliset files will be underspecified if used by someone who does have WebCoreDerivedSourcesAdditions.make.

There might be one thing that can help us here, but it's not pretty. As part of the work I'm doing to support XCBuild, I'm thinking of adding a build step that will validate the .xcfilelist files. I can run the script that generates the .xcfilelistfiles, but in a mode that will validate the existing files rather than generating new ones. If it determines that the existing .xcfilelist files are out-of-date, it stops the build and tells the developer to regenerate them and restart the build. With this build step in place, if a developer regenerates the .xcfilelist files without WebCoreDerivedSourcesAdditions.make and checks them in, and then a developer with WebCoreDerivedSourcesAdditions.make comes along and tries to do a build, they'll find that they need to regenerate the .xcfilelist files again. This time, they should be complete and generally usable.

[Andy Estes]

(In reply to Keith Rollin from comment #9)
> I'm thinking about the case where someone who doesn't have
> WebCoreDerivedSourcesAdditions.make is in a position where they need to
> regenerate the .xcfilelist files. They can run the script to do the
> regeneration, but since the WebCoreDerivedSourcesAdditions.make is missing,
> its files won't be included in the resulting .xcfilelist files. Those
> .xcfileliset files will be underspecified if used by someone who does have
> WebCoreDerivedSourcesAdditions.make.

Ok, so if the validation build phase you describe is in place then such a change would break Apple Internal SDK builds and someone at Apple would have to run the regeneration script again and check in the new .xcfilelist files to fix the build.

It's not great to have a system where open source contributors can break Apple builds in such a way that only someone at Apple can fix.

Can the inputs/outputs be specified in more than one .xcfilelist file? If so, could WebKitAdditions contribute its own .xcfilelist to describe its additional Makefiles' inputs and outputs?

> 
> There might be one thing that can help us here, but it's not pretty. As part
> of the work I'm doing to support XCBuild, I'm thinking of adding a build
> step that will validate the .xcfilelist files. I can run the script that
> generates the .xcfilelistfiles, but in a mode that will validate the
> existing files rather than generating new ones. If it determines that the
> existing .xcfilelist files are out-of-date, it stops the build and tells the
> developer to regenerate them and restart the build. With this build step in
> place, if a developer regenerates the .xcfilelist files without
> WebCoreDerivedSourcesAdditions.make and checks them in, and then a developer
> with WebCoreDerivedSourcesAdditions.make comes along and tries to do a
> build, they'll find that they need to regenerate the .xcfilelist files
> again. This time, they should be complete and generally usable.

This sounds like a good idea to me. Ensuring the .xcfilelist files stay up-to-date seems like a problem we need to solve even if we don't have optional includes to worry about.

[Keith Rollin]

(In reply to Andy Estes from comment #10)
> (In reply to Keith Rollin from comment #9)
> 
> Can the inputs/outputs be specified in more than one .xcfilelist file? If
> so, could WebKitAdditions contribute its own .xcfilelist to describe its
> additional Makefiles' inputs and outputs?

Xcode provides the ability to add more than one .xcfilelist to both the inputs and outputs sections, so that part seems possible. However, it looks like the specified .xcfilelists are not optional. If one is missing, Xcode will give an error. This means that the .xcfilelist files for the WebKitAdditions bits would have to be in the WebKit repo.

[Andy Estes]

(In reply to Keith Rollin from comment #11)
> (In reply to Andy Estes from comment #10)
> > (In reply to Keith Rollin from comment #9)
> > 
> > Can the inputs/outputs be specified in more than one .xcfilelist file? If
> > so, could WebKitAdditions contribute its own .xcfilelist to describe its
> > additional Makefiles' inputs and outputs?
> 
> Xcode provides the ability to add more than one .xcfilelist to both the
> inputs and outputs sections, so that part seems possible. However, it looks
> like the specified .xcfilelists are not optional. If one is missing, Xcode
> will give an error. This means that the .xcfilelist files for the
> WebKitAdditions bits would have to be in the WebKit repo.

It looks like .xcfilelist files can reference build settings, and we know how to optionally include .xcconfig files, so that should give us a patch forward here. We could have an .xcfilelist in open source that includes WebKitAdditions bits using a  build setting that evaluates to a non-empty value when WebKitAdditions is available.

Given that, as Keith and I discussed in person, we think this patch is ok to land as-is since XCBuild is still off by default.

[Andy Estes]

Created attachment 356070 [details]
Patch

[EWS Watchlist]

Attachment 356070 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/ApplePay/WebPaymentCoordinatorProxy.h:159:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebKit/UIProcess/ApplePay/WebPaymentCoordinatorProxy.h:162:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 356070 [details]
Patch

Clearing flags on attachment: 356070

Committed r238713: <https://trac.webkit.org/changeset/238713>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

Reverted r238713 for reason:

Breaks internal builds.

Committed r238722: <https://trac.webkit.org/changeset/238722>

[Ryan Haddad]

(In reply to Ryan Haddad from comment #17)
> Reverted r238713 for reason:
> 
> Breaks internal builds.
> 
> Committed r238722: <https://trac.webkit.org/changeset/238722>
See radar.

[Andy Estes]

Created attachment 356178 [details]
Patch

[EWS Watchlist]

Attachment 356178 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/ApplePay/WebPaymentCoordinatorProxy.h:159:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebKit/UIProcess/ApplePay/WebPaymentCoordinatorProxy.h:162:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 356178 [details]
Patch

Clearing flags on attachment: 356178

Committed r238739: <https://trac.webkit.org/changeset/238739>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.