SILGen tests: always build modules and API notes when importing the mock SDK

[gribozavr]

Previously, only test/SILGen/objc_nonnull_lie_hack.swift would build the API notes for the 'gizmo' module. Depending on the order in which tests ran, the module cache would either contain the 'gizmo' module with nullability annotations, or without. If we were unlucky and the module cache was created by a test that did not build the API notes, test/SILGen/objc_nonnull_lie_hack.swift failed.

This change also removes 46 instances of -enable-source-import (63 more left in the testsuite).

[gribozavr]

@jrose-apple Would you mind reviewing?

[gribozavr]

@swift-ci Please test

[swift-ci]

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 1ccb7f47759f13409021828efb988c3757fb5b4e
Test requested by - @gribozavr

[gribozavr]

@swift-ci Please test

[swift-ci]

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - ad1604484a1534f06271a1d2fe698c1b4d78c01a
Test requested by - @gribozavr

[gribozavr]

@swift-ci Please clean test

[swift-ci]

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - ad1604484a1534f06271a1d2fe698c1b4d78c01a
Test requested by - @gribozavr

[gribozavr]

@swift-ci Please test

[gribozavr]

@swift-ci Please smoke test

[jrose-apple]

It might be worth it at this point to fix the actual problem: we don't have a way to prepare the mock-SDK overlays beforehand. Building N copies of the overlays is silly and moderately expensive, and using a standard substitution is both including some unnecessary ones and (in some cases) leaving out some necessary ones as well. It doesn't help that the real overlays get used if the mock ones haven't been built.

[gribozavr]

It might be worth it at this point to fix the actual problem: we don't have a way to prepare the mock-SDK overlays beforehand.

While I agree that would be the ultimate fix, this PR is an easy/mechanical change and I have prepared the patch. Are you opposed to landing it as is?

Building N copies of the overlays is silly and moderately expensive

I measured, this change only added 1 second to total testing time on my machine. (Only measured the SilGen directory to cut down the noise.)

[jrose-apple]

You're right, getting -enable-source-import out of more tests is a good thing. How much do we care about SILGen tests accidentally using the real SDK?

[gribozavr]

How much do we care about SILGen tests accidentally using the real SDK?

These days nullability is not flipping often, so I don't care that much. What prompted me to create this PR is the difference between APINotes for the gizmo module. All tests importing it should see the apinotes, otherwise (as the PR description explains) depending on the run order, we get test failures.

[jrose-apple]

I agree about that, but that wasn't really the only change. The more I think about it the weirder it seems that %build-silgen-test-overlays would build gizmo's API notes but not objc_protocols_Bas or whatever. How is anyone supposed to know that?

The right answer to the API notes problem is to have API notes treated as a dependency for the module, and we should do that anyway. It's just not as straightforward as it should be (the bug has bounced between @DougGregor, @milseman, and me).

[gribozavr]

The more I think about it the weirder it seems that %build-silgen-test-overlays would build gizmo's API notes but not objc_protocols_Bas or whatever. How is anyone supposed to know that?

I can add objc_protocols_Bas to the substitution as well, but it was used in only one test so I decided it is not worth the compilation time.

If you are saying that %build-silgen-test-overlays is a bad name, then I agree that at this point it is a bad name, and it should be something like %build-silgen-test-fixture. What do you think?

[jrose-apple]

My problem isn't with the name, it's that I have no idea what it does. Before I would assume it built the overlays equivalent to the real ones, i.e. anything related to the public SDK. (In practice these substitutions build a few things up to Foundation; no AppKit or UIKit and no support modules like Dispatch.) I'm realizing now that that is a shaky assumption as well.

I don't want to get into this situation:

"My test isn't working."
"Do you have %build-silgen-test-overlays?"
"Oh, no. Do I need that?"
"Usually you need it if you use anything from SILGen's mock SDK."
"Okay, that makes sense, I guess. ...Nope, still doesn't work."
"Hm. Oh, it doesn't build that one."
"Huh. Why not?"
"Probably only one other test was using it."
"Well, should I add it to the substitution, or...?"

The answer might be "if it's used by more than one test", but that's not a very satisfying answer to me. The reason the substitutions were added were partially the repetition, but more because the dependencies of a module aren't obvious from the test. That doesn't apply to leaf modules imported by just the one test.

[gribozavr]

@jrose-apple Are you opposed to this PR landing as is? I'm still getting spurious failures in the testsuite locally because of the reasons I explained in the PR description. We even had this on the bots: https://ci.swift.org/job/swift-PR-osx/3518/console

[jrose-apple]

If we're just looking for a quick fix, we can use a separate module cache for that one test. That shouldn't force the decision here.

[gribozavr]

My problem isn't with the name, it's that I have no idea what it does.

It builds an environment for the SILGen tests. You said:

Rather than remember which of %empty-directory, %target-build-xyz-overlays, etc. needs to go at the top of each test, lit should just do it.

If %build-silgen-test-overlay was implicitly injected by lit, I don't think it addresses your concern that you don't understand what it does -- it might even be worse because you don't understand what it does, and you don't see it in your test file. So I'm confused by now.

"Well, should I add it to the substitution, or...?"
The answer might be "if it's used by more than one test",

So what would be the ideal solution? That the test source would be somehow scanned for dependencies and everything was automatically built? (But how do you determine build parameters then? Some tests require special parameters, for example, enabling resilience.)

[slavapestov]

@gribozavr @jrose-apple What's the latest on this? I would love to see this get merged in. We really need to get rid of -enable-source-import.

[jrose-apple]

This is really only coincidentally related to -enable-source-import. The argument was mostly over what needs to be spelled out explicitly and what should be folded into a substitution.

It's actually possible that with Doug fixing the issue of API notes not being included as module dependencies (apple/swift-clang@1bc5dd5) that we don't hit the original issue that prompted this whole thing, but that doesn't mean it's not worthwhile to get rid of -enable-source-import.

[slavapestov]

@jrose-apple Gentle poke about this PR

[jrose-apple]

Doesn't deserve to be gentle. :-) Thanks, Slava.

[CodaFi]

@jrose-apple What do you want to do about this pull request?

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 80c5cd1

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 80c5cd1

[jrose-apple]

I now/still believe #20957 is the right answer to this whole mess, but as neither me nor Dmitri are currently working on Swift…

I do think it always makes sense to be consistent about API notes for mock modules, but that doesn't need this whole patch.

[DougGregor]

Yeah, let's close this.