Bug 229130 - Build ANGLE as an additional static library for tests and other executables
Summary: Build ANGLE as an additional static library for tests and other executables
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: ANGLE (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kimmo Kinnunen
URL:
Keywords: InRadar
Depends on:
Blocks: 229079 229080
  Show dependency treegraph
 
Reported: 2021-08-16 01:05 PDT by Kimmo Kinnunen
Modified: 2022-02-19 08:20 PST (History)
7 users (show)

See Also:


Attachments
Patch (494.27 KB, patch)
2021-08-16 01:12 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (493.86 KB, patch)
2021-08-18 04:03 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (497.46 KB, patch)
2021-08-23 02:29 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kimmo Kinnunen 2021-08-16 01:05:59 PDT
Build ANGLE as an additional static library for tests and other executables

Tests and other executables need to access ANGLE internal functions that should not and can not be exposed through the dynamic library.

This would result in two builds: one for actual use, other for testers.
Comment 1 Kimmo Kinnunen 2021-08-16 01:12:05 PDT
Created attachment 435578 [details]
Patch
Comment 2 EWS Watchlist 2021-08-16 01:13:17 PDT
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 3 Alex Christensen 2021-08-16 09:31:46 PDT
Comment on attachment 435578 [details]
Patch

If I understand this correctly, this makes it so we build ANGLE twice.  That would unnecessarily slow down the build.
Comment 4 Alex Christensen 2021-08-16 09:51:16 PDT
By the way, we already undid this in https://webkit.org/b/218469
Comment 5 Kimmo Kinnunen 2021-08-18 04:02:55 PDT
(In reply to Alex Christensen from comment #4)
> If I understand this correctly, this makes it so we build ANGLE twice.  That would unnecessarily slow down the build.

No, only for the case where the testers would be built.

> By the way, we already undid this in https://webkit.org/b/218469

No, it was different. In bug 218469, the static libANGLE.a would always be built.

Here, libANGLE.a would be built only when requested. None of the default used targets would request it to be built.

Taking the liberty of reopening to make progress.
Comment 6 Kimmo Kinnunen 2021-08-18 04:03:35 PDT
Created attachment 435760 [details]
Patch
Comment 7 Kenneth Russell 2021-08-18 12:39:23 PDT
Comment on attachment 435760 [details]
Patch

One way or another, it would be ideal for WebKit's clone of ANGLE to be able to build and run the integrated unit tests.

Please ping once this patch is green on the EWS bots.
Comment 8 Radar WebKit Bug Importer 2021-08-23 01:06:18 PDT
<rdar://problem/82233905>
Comment 9 Kimmo Kinnunen 2021-08-23 02:29:57 PDT
Created attachment 436170 [details]
Patch
Comment 10 Kenneth Russell 2021-08-23 17:56:21 PDT
Comment on attachment 436170 [details]
Patch

Hoping Alex finds this palatable so that Apple folks can build and run these important test suites out of WebKit's copy of the ANGLE repo.

As mentioned earlier, we're working on getting these tests passing with the direct-to-Metal backend in upstream ANGLE: https://bugs.chromium.org/p/angleproject/issues/detail?id=5505 .
Comment 11 Alex Christensen 2021-08-23 18:12:10 PDT
Do we have a plan to run the ANGLE unit tests regularly?
This basically checks in an extra, unused build system to be used with things outside of our repo.  Is there a reason that this needs to be in our repo?
Has anyone verified what this does to our internal build system?
Comment 12 Kimmo Kinnunen 2021-08-23 23:23:28 PDT
(In reply to Alex Christensen from comment #11)
> Do we have a plan to run the ANGLE unit tests regularly?

Yes, I think the current near to medium plan is that we commit changes to Source/ThirdParty/ANGLE, like we have been doing for quite some time. Currently, for quite some time, the changes have been committed without running tests or implementing additional tests that exercise the changes. This has the obvious drawbacks that testless changes typically have. Near to medium term, we should change this.

Even if we did not do any commits other than merges from upstream, it'd still be very good idea to test what we compile -- to understand whether our compilation matches the upstream compilation.


> This basically checks in an extra, unused build system

What do you mean extra, unused build system? There's no build system being added. 

The patch here adds normal build target (libANGLE.a) to the normal build system.

The target will be used in for the test executables and development executables. For example, see the blocking bug 229079.

> to be used with things outside of our repo.

What do you mean "to be used with things outside our repo"?
All the code being compiled and intended to be run by tests or development executables are in the repository (WebKit.git)

> Is there a reason that this needs to be in our repo?

Our development, such as debugging and testing, works based on the work in WebKit repository. As such, running test executables and preferably tests to exercise this code is needed.

> Has anyone verified what this does to our internal build system?

I have verified to the best of my knowledge that this change doesn't do anything to our internal build system, as none of the default build targets refer to the build targets added here.
Comment 13 Dean Jackson 2022-02-19 08:20:31 PST
Comment on attachment 436170 [details]
Patch

Clearing the review flag here since it has been untouched for a nearly 6 months.

It seems like a good idea to me. Kimmo do you want to re-upload?