Skip to content

Verify emitted module interfaces by default (Take 2) #652

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 22, 2021

Conversation

xymus
Copy link
Contributor

@xymus xymus commented May 12, 2021

Verifying swiftinterface files after they are emitted will detect framework breaking issues much earlier. It can be turned off in offending projects with -no-verify-emitted-module-interface.

Only swiftinterfaces emitted from non-merge-module builds at verified by default. Merge-module builds are more likely to generate broken swiftinterfaces even when a whole-module or emit-module-separately build generates a valid one.

rdar://77534863

@xymus
Copy link
Contributor Author

xymus commented May 12, 2021

@swift-ci Please test

@xymus
Copy link
Contributor Author

xymus commented May 12, 2021

@swift-ci test Apple Silicon

@artemcm
Copy link
Contributor

artemcm commented May 12, 2021

@swift-ci test Apple Silicon

Sorry, I misunderstood. This does not work on this repo, it needs to be invoked as a cross-repository test with this PR from the swift repo.

xymus and others added 2 commits July 16, 2021 16:58
Merge-module builds do not respect the -preserve-types-as-written
frontend flag and may generate broken swiftinterface files. Only verify
swiftmodule in this case if explicitly asked to.
@xymus xymus force-pushed the revert-651-revert-629-verify-swiftinterfaces-by-default branch from e9fcd42 to fa379ca Compare July 17, 2021 00:01
@xymus
Copy link
Contributor Author

xymus commented Jul 17, 2021

@swift-ci Please test

@xymus
Copy link
Contributor Author

xymus commented Jul 19, 2021

The broken swiftinterface in the Swift repo has been fixed and the tests are looking good (except maybe for one that I'm testing again). swiftlang/swift#38454

@artemcm Can you give this another review?

@xymus xymus requested a review from artemcm July 19, 2021 16:37
@nkcsgexi
Copy link
Contributor

Related to the discussion in a Swift-side PR: swiftlang/swift#38465

Can we sanitize -disable-availability-checking when verifying module interfaces?

@xymus
Copy link
Contributor Author

xymus commented Jul 19, 2021

@nkcsgexi What do you mean by "sanitize"? I don't think -disable-availability-checking is printed in the swiftinterface at this time.

@nkcsgexi
Copy link
Contributor

ah, sorry for missing explanation. verifyModuleInterfaceJob calls addCommonFrontendOptions which adds all -Xfronend options. If the framework authors specified -Xfrontend -disable-availability-checking in the build setting, interface verification job will succeed even if there are availability violations in the interface, resulting in the escape of broken interfaces. By sanitizing, I meant after calling addCommonFrontendOptions, we specifically remove -disable-availability-checking to avoid situations like this.

@xymus
Copy link
Contributor Author

xymus commented Jul 19, 2021

Ah ok, good point. We could use that phase as an early sanity check for all kind of library issues.

@xymus
Copy link
Contributor Author

xymus commented Jul 22, 2021

I think we can merge this now. The remaining failure on Apple Silicon is unrelated to this change.

We should add extra verifications once this lands properly and we have a reliable verification in place.

@xymus xymus merged commit 641d954 into main Jul 22, 2021
@xymus xymus deleted the revert-651-revert-629-verify-swiftinterfaces-by-default branch July 22, 2021 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants