refactor serialization and state management #3696

tomerd · 2021-08-28T06:20:14Z

motivation: stronger seperation between state managment format and data model, allowing evolution of storeage format

changes:

refactor PinsStore, RepositoryManager, and WorkspaceState to model their state storage seperately from the data model
use Codable instead of JSONMappable/JSONSerializable/SimplePersistence
adjust call-sites and tests where necessary

Sources/Workspace/ManagedDependency.swift

tomerd · 2021-08-28T08:10:14Z

@swift-ci please smoke test

tomerd · 2021-08-28T18:43:37Z

@swift-ci please smoke test

tomerd · 2021-08-28T18:59:04Z

11:48:44 Test Suite 'Selected tests' started at 2021-08-28 13:46:58.332
11:48:44 Test Suite 'APIDiffTests' started at 2021-08-28 13:46:58.335
11:48:44 Test Case 'APIDiffTests.testCheckVendedModulesOnly' started at 2021-08-28 13:46:58.335
11:48:44 
/home/buildnode/jenkins/workspace/swift-package-manager-self-hosted-Linux-smoke-test/branch-main/swiftpm/Tests/CommandsTests/APIDiffTests.swift:144: error: APIDiffTests.testCheckVendedModulesOnly : XCTAssertTrue failed - 
11:48:44 Test Case 'APIDiffTests.testCheckVendedModulesOnly' failed (17.161 seconds)
11:48:44 Test Suite 'APIDiffTests' failed at 2021-08-28 13:47:15.495
11:48:44 	 Executed 1 test, with 1 failure (0 unexpected) in 17.161 (17.161) seconds
11:48:44 Test Suite 'Selected tests' failed at 2021-08-28 13:47:15.495
11:48:44 	 Executed 1 test, with 1 failure (0 unexpected) in 17.161 (17.161) seconds
11:48:44 
11:48:44 Test Suite 'Selected tests' started at 2021-08-28 13:46:58.324
11:48:44 Test Suite 'APIDiffTests' started at 2021-08-28 13:46:58.326
11:48:44 Test Case 'APIDiffTests.testFilters' started at 2021-08-28 13:46:58.328
11:48:44 /home/buildnode/jenkins/workspace/swift-package-manager-self-hosted-Linux-smoke-test/branch-main/swiftpm/Tests/CommandsTests/APIDiffTests.swift:182: error: APIDiffTests.testFilters : XCTAssertTrue failed - 
11:48:44 Test Case 'APIDiffTests.testFilters' failed (22.164 seconds)
11:48:44 Test Suite 'APIDiffTests' failed at 2021-08-28 13:47:20.492
11:48:44 	 Executed 1 test, with 1 failure (0 unexpected) in 22.164 (22.164) seconds
11:48:44 Test Suite 'Selected tests' failed at 2021-08-28 13:47:20.492
11:48:44 	 Executed 1 test, with 1 failure (0 unexpected) in 22.164 (22.164) seconds

APIDiffTests seem to be failing on Linux on this PR and others - maybe the underlying API diff tool on Linux is broken / changed? does it even make sense for SwiftPM tests to test the APIDiff functionality given its only an integration shim (which we need to move to plugin!). @neonichu @abertelrud @elsh?

Sources/Basics/JSON+Extensions.swift

Sources/PackageGraph/PinsStore.swift

motivation: stronger seperation between state managment format and data model, allowing evolution of storeage format changes: * refactor PinsStore, RepositoryManager, and WorkspaceState to model their state storage seperately from the data model * use Codable instead of JSONMappable/JSONSerializable/SimplePersistence * adjust call-sites and tests where necessary

tomerd · 2021-08-31T03:07:44Z

@swift-ci please smoke test

tomerd · 2021-08-31T07:20:54Z

@swift-ci please smoke test

abertelrud · 2021-08-31T16:54:46Z

Sources/PackageGraph/PinsStore.swift

+    private let encoder = JSONEncoder.makeWithDefaults()
+    private let decoder = JSONDecoder.makeWithDefaults()


Is there any particular advantage to keeping these around? (e.g. performance) Is there any concern about unintentional leaking of state from one encode/decode to the next?

memory and performance. I dont think they hold any state.

abertelrud · 2021-08-31T16:57:56Z

Sources/Workspace/WorkspaceState.swift

            // FIXME: We should emit a warning here using the diagnostic engine.
-            TSCBasic.stderrStream.write("\(error)\n")
+            TSCBasic.stderrStream.write("warning: unable to restore workspace state: \(error)")


I realize this is existing code, so doesn't need to be changed in this PR, but should a future PR change this to use a Diagnostics collector since I assume this could be emitted from an IDE as well (as it's part of libSwiftPM)?

the issue is that Workspace does not have a DiagnosticsEngine to use in this case. I was planning to address this in one of the next refactoring PRs, focused on making Diagnostics more globally available

Yah, we have at least one more such case that we can clean up once we make diagnostics more broadly available.

abertelrud

This is great cleanup!

finagolfin · 2021-09-16T13:44:38Z

Sources/Workspace/Destination.swift

+fileprivate struct DestinationInfo: Codable {
+    let target: String?
+    let sdk: AbsolutePath?
+    let binDir: AbsolutePath


I just noticed that this small change from this much larger pull changes the format for destination JSON files, rather than what's documented here and here.

@tomerd, should I update the docs and example for the new format or should we stick with the old format? I'll submit a pull for either way you want to go.

@buttaface great catch, would appreciate such help

actually, I think we can also consider the reverse it the old name is more correct - ie we could also rename / change the decoding to accept the old name, as the rename was more cleanup code wise

iow the goal here was not to change the format / names, but an internal cleanup of the decoding to use Codable, the name change was an oversight, so happy to change back to the name if preferable

def this is missing a test tho :D

OK, I will change it back and add a test.

Submitted two fixes in #3793, will submit the test later.

Motivation: The larger refactoring changed the destination JSON format and introduced a bug in passing flags. Changes: Use a CodingKeys enum to go back to the old format and correct the flag error.

tomerd requested review from abertelrud, friedbunny and neonichu as code owners August 28, 2021 06:20

WowbaggersLiquidLunch reviewed Aug 28, 2021

View reviewed changes

Sources/Workspace/ManagedDependency.swift Outdated Show resolved Hide resolved

tomerd force-pushed the refactor/serialization branch 2 times, most recently from 47fe878 to f284806 Compare August 28, 2021 08:10

tomerd force-pushed the refactor/serialization branch 3 times, most recently from 3ea2e7d to 3f0a2b0 Compare August 28, 2021 18:43

tomerd added the ready Author believes the PR is ready to be merged & any feedback has been addressed label Aug 28, 2021

abertelrud reviewed Aug 30, 2021

View reviewed changes

Sources/Basics/JSON+Extensions.swift Show resolved Hide resolved

abertelrud reviewed Aug 30, 2021

View reviewed changes

Sources/PackageGraph/PinsStore.swift Show resolved Hide resolved

abertelrud reviewed Aug 30, 2021

View reviewed changes

Sources/PackageGraph/PinsStore.swift Outdated Show resolved Hide resolved

abertelrud reviewed Aug 30, 2021

View reviewed changes

Sources/PackageGraph/PinsStore.swift Outdated Show resolved Hide resolved

tomerd self-assigned this Aug 30, 2021

tomerd mentioned this pull request Aug 31, 2021

refactor managed dependencies #3698

Merged

tomerd added 3 commits August 30, 2021 19:53

fixup

f36ac1a

fixup

c0b9d80

tomerd force-pushed the refactor/serialization branch from 3f0a2b0 to c0b9d80 Compare August 31, 2021 03:07

fixup

3454656

abertelrud reviewed Aug 31, 2021

View reviewed changes

neonichu approved these changes Aug 31, 2021

View reviewed changes

abertelrud reviewed Aug 31, 2021

View reviewed changes

abertelrud approved these changes Aug 31, 2021

View reviewed changes

tomerd merged commit 6f8a435 into swiftlang:main Aug 31, 2021

finagolfin reviewed Sep 16, 2021

View reviewed changes

tomerd pushed a commit that referenced this pull request Oct 6, 2021

Revert JSON format change and fix bug introduced by #3696 (#3793)

2f781b4

		private let encoder = JSONEncoder.makeWithDefaults()
		private let decoder = JSONDecoder.makeWithDefaults()

refactor serialization and state management #3696

refactor serialization and state management #3696

Uh oh!

Conversation

tomerd commented Aug 28, 2021

Uh oh!

Uh oh!

tomerd commented Aug 28, 2021

Uh oh!

tomerd commented Aug 28, 2021

Uh oh!

tomerd commented Aug 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tomerd commented Aug 31, 2021

Uh oh!

tomerd commented Aug 31, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abertelrud left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tomerd commented Aug 28, 2021 •

edited

Loading