Skip to content
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

utils: remove swiftinterfaces from the build image #79840

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

compnerd
Copy link
Member

@compnerd compnerd commented Mar 7, 2025

These cannot be reserialized due to a bug in the serialization logic where we do not inject the VFS Ovelay for the system modules.

Bug: #79839

@compnerd
Copy link
Member Author

compnerd commented Mar 7, 2025

Please test with following PRs:
swiftlang/swift-installer-scripts#400

@swift-ci please smoke test

@compnerd
Copy link
Member Author

compnerd commented Mar 7, 2025

Please test with following PRs:
swiftlang/swift-installer-scripts#400

@swift-ci please build toolchain Windows platform

@compnerd
Copy link
Member Author

compnerd commented Mar 7, 2025

CC: @xymus

Copy link
Member

@weliveindetail weliveindetail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, that this workaround is the best way forward right now. Test on Windows failed with property 'SDKRoot' cannot be found.

These cannot be re-serialized due to a bug in the serialization logic
where we do not inject the VFS overlay for the system modules.

Bug: swiftlang#79839
@compnerd
Copy link
Member Author

Please test with following PRs:
swiftlang/swift-installer-scripts#400

@swift-ci please build toolchain Windows platform

@compnerd
Copy link
Member Author

Please test with following PRs:
swiftlang/swift-installer-scripts#400

@swift-ci please smoke test

1 similar comment
@compnerd
Copy link
Member Author

Please test with following PRs:
swiftlang/swift-installer-scripts#400

@swift-ci please smoke test

Copy link
Member

@weliveindetail weliveindetail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this in #79185 and this specific aspect is working in CI with the tiny fix below.

@compnerd
Copy link
Member Author

@swift-ci please smoke test macOS platform

@compnerd
Copy link
Member Author

Please test with following PRs:
swiftlang/swift-installer-scripts#400

@swift-ci please test Windows platform

@compnerd
Copy link
Member Author

Please test with following PRs:
swiftlang/swift-installer-scripts#400

@swift-ci please build toolchain Windows platform

@compnerd
Copy link
Member Author

CC: @xymus

@xymus
Copy link
Contributor

xymus commented Mar 11, 2025

It looks like this will correctly work around the change from #79588. Relying only on binary swiftmodules for a local build should be fine, but I would caution against it for distributed SDKs and toolchains.

For more context, we're very slowly moving towards using more and more textual swiftinterfaces for distributed modules. Binary swiftmodules are not designed to support changes in dependencies. Notably, a change to imported headers can force the compiler to drop entire decls, that's in the degraded mode as it crashes otherwise. So when building the modules on one machine and deploying on a different one the binary swiftmodules may be unusable, whereas swiftinterfaces are designed to be rebuilt on each machine against the local dependencies.

@compnerd
Copy link
Member Author

@xymus hmm, the problem is that the re-serialization will fail with module not found errors for underlying clang modules. I suspect that this is due to VFS overlays not being applied when re-serialising the modules.

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