Bug 229080 – Build ANGLE as intermediate static library

• Component: ANGLE
• Status: ASSIGNED

[Kimmo Kinnunen]

Build ANGLE as intermediate static library

This is needed to compile ANGLE unit tests and other executables that use ANGLE internal APIs.

[Kimmo Kinnunen]

Support for this was removed in bug 225162

[Kimmo Kinnunen]

Created attachment 435495 [details]
Patch

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

[Kenneth Russell]

Comment on attachment 435495 [details]
Patch

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

Knowing how much difficulty Dean had switching ANGLE over to the dynamic configuration I think it'd be better for him to review this patch, but it looks good to me overall assuming the build works - one question.

> Source/ThirdParty/ANGLE/Configurations/ANGLE-dynamic.xcconfig:7
> +ANGLE_OTHER_LDFLAGS = -allowable_client WebCore -allowable_client WebCoreTestSupport -framework QuartzCore -framework CoreGraphics -framework Foundation -framework IOSurface -framework Metal $(ANGLE_OTHER_LDFLAGS_$(WK_PLATFORM_NAME)) -Wl,-force_load,"$(BUILT_PRODUCTS_DIR)/libANGLE.a";

Just to confirm: this patch causes libANGLE.a to always be built, and changes the dylib so that it links against that to pick up all of its code?

[Kimmo Kinnunen]

(In reply to Kenneth Russell from comment #4)
> > Source/ThirdParty/ANGLE/Configurations/ANGLE-dynamic.xcconfig:7
> > +ANGLE_OTHER_LDFLAGS = -allowable_client WebCore -allowable_client WebCoreTestSupport -framework QuartzCore -framework CoreGraphics -framework Foundation -framework IOSurface -framework Metal $(ANGLE_OTHER_LDFLAGS_$(WK_PLATFORM_NAME)) -Wl,-force_load,"$(BUILT_PRODUCTS_DIR)/libANGLE.a";
> 
> Just to confirm: this patch causes libANGLE.a to always be built, and
> changes the dylib so that it links against that to pick up all of its code?

Yes, the project is set up so that "ANGLE (dynamic)" depends on "libANGLE" by the mechanism of "Frameworks and Libraries", which makes the dynamic library link the static library. The hunk related to "force_load" makes the linker not eliminate all code, since the dynamic library by itself doesn't actually refer to any symbols of the static library..

[Kenneth Russell]

Comment on attachment 435495 [details]
Patch

Thanks for the information. Let me know on Monday if you need my r+ or whether Dean can review this. If I remember correctly it was mainly Apple's internal builds of WebKit which broke with configuration changes like these, so this should be landed earlier in the week when people are on call to revert any breakage.

[Alex Christensen]

Comment on attachment 435495 [details]
Patch

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

This effectively undoes r276734 which was done for a good reason.  I've also been dealing with and fixing internal fallout from that change for several months since then.   I very very strongly oppose doing this right now, and only one very strongly oppose doing this in the future.  The stated reason has something to do with unit testing ANGLE.  Is there no other way to do that?  Can we not make the dylib able to be used in unit tests instead?  That would have the added benefit of testing a closer configuration to what customers see.

> Source/ThirdParty/ANGLE/ChangeLog:21
> +        Remove stale scheme that was left when this work was originally removed in
> +        bug 225162.

This sounds like a good thing to do.

[Alexey Proskuryakov]

I haven't had a chance to look at the patch yet, so just a question for now. Where does the static library go? Re-adding it to the SDK would be a problem, as that noticeably increases size of Xcode download, and removing it was a big deal.

[Kimmo Kinnunen]

Thanks for looking at this.

(In reply to Alex Christensen from comment #7)
> Comment on attachment 435495 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=435495&action=review
> 
> This effectively undoes r276734 which was done for a good reason.  

Well, it says the reason: "Now that everyone's linking against the dylib, we don't need to build the static library any more". Now there would be targets linking against the static lib too..


> The stated reason
> has something to do with unit testing ANGLE.  Is there no other way to do
> that?  Can we not make the dylib able to be used in unit tests instead? 

Unit tests don't really work that way. If dylib would be able to provide everything unit tests need, it would be the size of static lib, wouldn't it? The commit message explains that the tests use the internals of the library.


> I haven't had a chance to look at the patch yet, so just a question for now. Where does the static library go? Re-adding it to the SDK would be a problem, as that noticeably increases size of Xcode download, and removing it was a big deal.

Library goes to BUILT_PRODUCTS_DIR, but it's not used by any external target, so it doesn't need to be in the SDK. I don't know if by definition SDK == BUILT_PRODUCTS_DIR? I was under the impression that we had some scripting around this and tried to ensure the libANGLE.a wasn't part of this scripting.

So I think the rationale given was that:
1) WebCore direct dependencies are put to the SDK
2) libANGLE.a was a direct dependency of WebCore
3) Hence libANGLE.a increased the SDK size

I worked on the assumption that:
1) WebCore direct dependencies are put to the SDK
2) libANGLE.a is not a direct dependency of WebCore
3) Hence libANGLE.a does not need to increase the SDK size, if we just don't put it in there.

Now the question is if the "don't put it there" is possible. I hope this would be discussed.

[Kimmo Kinnunen]

>If dylib would be able to provide everything unit tests need, it would be the size of static lib, wouldn't it? 

Hmm, this analogy is probably very incorrect, so let's just pretend I didn't say it.

Anyway, I think the option is to rebuild everything for the test purposes. I'd still like to investigate the scenario where dylib and tests use the same compiled compilation units.

[Kimmo Kinnunen]

(In reply to Kimmo Kinnunen from comment #10)
> Anyway, I think the option is to rebuild everything for the test purposes.

In fact, in order to make forwards progress, I submitted this as bug 229130.
If you only have time, please take a look.

> I'd still like to investigate the scenario where dylib and tests use the
> same compiled compilation units.

This still holds: I'd like to use this bug to continue discussing the scenario where we would build libANGLE-shared.dylib with libANGLE.a. 

This would benefit:
1) Clearer match for testing what was compiled for production.
2) Better ergonomics, faster build and less error prone process for development edit-test cycle.
3) Possibly simpler to integrate to CI infrastructure?

[Alex Christensen]

If you really want to build a static library, then use buildit before and after r276734 to see where the static library was put, then use buildit before and after your change to make sure the static library is not put in the same place.  If buildit puts it in the same place, then it is in the SDK, which we don't want.

[Radar WebKit Bug Importer]

<rdar://problem/82176324>