Skip to content

Conversation

@ktoso
Copy link
Contributor

@ktoso ktoso commented Mar 17, 2023

Summary: Bring back code which was assumed to be dead, but isn't in cross-module cases.
Description:
During #64237 I spotted that we had // TODO(distributed): make use of this after all, but FORCE it? above the deriveDistributedActor_id and a quick check uncovered that this code isn't hit in our tests and I removed it. The source compatibility suite uncovered that this caused a regression in the distributed cluster build.

This removal a day or two ago was incorrect and must be undone.

Turns out, this IS hit but during builds across modules only (!), and we should not remove it.

Risk: Low, this brings back code which was always here and was removed yesterday by a wrong assumption that it was not needed
Testing:

  • source compat suite caught regression in distributed cluster
  • manually verified on the distributed cluster
  • CI testing including the source compat suite confirm this is the fix.
  • I'm working on a way to test this within the swift build so we won't regress like this anymore

@ktoso ktoso requested a review from slavapestov as a code owner March 17, 2023 09:48
@ktoso ktoso force-pushed the wip-distributed-id-synthesis-back branch from 70b2d7b to 4ef926e Compare March 17, 2023 10:49
@ktoso
Copy link
Contributor Author

ktoso commented Mar 17, 2023

@swift-ci please smoke test
@swift-ci Please Test Source Compatibility Debug

@ktoso ktoso changed the title Bring back Derived id synthesis, as some cases still use it [Distributed] Bring back Derived id synthesis, as some cases still use it Mar 17, 2023
@ktoso ktoso added the distributed Feature → concurrency: distributed actor label Mar 17, 2023
@justice-adams-apple
Copy link
Contributor

We should test the release build of source compat as well, as that's where the build is failing

@justice-adams-apple
Copy link
Contributor

@swift-ci please test source compatibility

@ktoso
Copy link
Contributor Author

ktoso commented Mar 17, 2023

Got it, and it passed -- great!

@ktoso
Copy link
Contributor Author

ktoso commented Mar 17, 2023

@swift-ci please smoke test

@ktoso ktoso merged commit a28cba3 into swiftlang:main Mar 18, 2023
@ktoso ktoso deleted the wip-distributed-id-synthesis-back branch March 18, 2023 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

distributed Feature → concurrency: distributed actor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants