Cleanup PackageModel Module #2980
Conversation
mattt
requested review from
abertelrud,
aciidgh,
friedbunny and
neonichu
as code owners
This comment has been minimized.
This comment has been minimized.
mattt
force-pushed
the
packagemodel-module-cleanup
branch
from
37a89bb to
28420cf
Compare
|
@swift-ci please smoke test |
|
@swift-ci please smoke test linux |
|
It looks like Linux smoke test is failing for unrelated reasons. Is this a known issue? Failing Log Lines/home/buildnode/jenkins/workspace/swift-package-manager-Linux-smoke-test/branch-main/sourcekit-lsp/Tests/LinuxMain.swift:13:1: error: expressions are not allowed at the top level 11:54:38 tests += LSPLoggingTests.__allTests() 11:54:38 ^ 11:54:38 /home/buildnode/jenkins/workspace/swift-package-manager-Linux-smoke-test/branch-main/sourcekit-lsp/Tests/LinuxMain.swift:14:1: error: expressions are not allowed at the top level 11:54:38 tests += LanguageServerProtocolJSONRPCTests.__allTests() 11:54:38 ^ 11:54:38 /home/buildnode/jenkins/workspace/swift-package-manager-Linux-smoke-test/branch-main/sourcekit-lsp/Tests/LinuxMain.swift:15:1: error: expressions are not allowed at the top level 11:54:38 tests += LanguageServerProtocolTests.__allTests() 11:54:38 ^ 11:54:38 /home/buildnode/jenkins/workspace/swift-package-manager-Linux-smoke-test/branch-main/sourcekit-lsp/Tests/LinuxMain.swift:16:1: error: expressions are not allowed at the top level 11:54:38 tests += SKCoreTests.__allTests() 11:54:38 ^ 11:54:38 /home/buildnode/jenkins/workspace/swift-package-manager-Linux-smoke-test/branch-main/sourcekit-lsp/Tests/LinuxMain.swift:17:1: error: expressions are not allowed at the top level 11:54:38 tests += SKSupportTests.__allTests() 11:54:38 ^ 11:54:38 /home/buildnode/jenkins/workspace/swift-package-manager-Linux-smoke-test/branch-main/sourcekit-lsp/Tests/LinuxMain.swift:18:1: error: expressions are not allowed at the top level 11:54:38 tests += SKSwiftPMWorkspaceTests.__allTests() 11:54:38 ^ 11:54:38 /home/buildnode/jenkins/workspace/swift-package-manager-Linux-smoke-test/branch-main/sourcekit-lsp/Tests/LinuxMain.swift:19:1: error: expressions are not allowed at the top level 11:54:38 tests += SourceKitDTests.__allTests() 11:54:38 ^ 11:54:38 /home/buildnode/jenkins/workspace/swift-package-manager-Linux-smoke-test/branch-main/sourcekit-lsp/Tests/LinuxMain.swift:20:1: error: expressions are not allowed at the top level 11:54:38 tests += SourceKitLSPTests.__allTests() 11:54:38 ^ 11:54:38 /home/buildnode/jenkins/workspace/swift-package-manager-Linux-smoke-test/branch-main/sourcekit-lsp/Tests/LinuxMain.swift:22:1: error: expressions are not allowed at the top level 11:54:38 XCTMain(tests) 11:54:38 ^ |
|
Not known to me, let's just try another re-run. |
|
@swift-ci please smoke test linux |
|
@swift-ci Please clean smoke test Linux platform |
|
@benlangmuir We're getting seemingly unrelated build failures with sourcekit-lsp on Linux smoke tests. [1]. I don't have much to go on, but do you think it has anything to do with swiftlang/swift-integration-tests#70? |
|
No, that integration test issue looks unrelated to me. I can't see any way that sourcekit-lsp would cause this failure - we haven't changed anything about LinuxMain.swift. Instead it looks like swiftpm is incorrectly building LinuxMain.swift as a regular swift file instead of as a main file like it used to. |
|
Looking at the history of https://ci.swift.org/job/swift-package-manager-Linux-smoke-test/ it seems like this failure is caused by this PR, since it's not happening on any other PR tests before or after. |
|
@benlangmuir Thanks for clarifying that — I appreciate your help with this. I'm still struggling to understand what about this PR might be affecting how SwiftPM builds LinuxMain, but that helps eliminate a few possibilities. |
|
@swift-ci Please smoke test Linux |
Move ProductType Codable conformance closer to declaration
…otocol declaration
Reimplement SwiftLanguageVersion and ToolsVersion comparison using public properties
Extract ResolvedPackage into separate file Extract ResolvedProduct into separate file Extract ResolvedTarget into separate file Reorganize protocol conformance
mattt
force-pushed
the
packagemodel-module-cleanup
branch
from
28420cf to
b58782f
Compare
|
@swift-ci please smoke test |
|
Thanks for this, @mattt. Using subdirectories for this makes sense to me; I think that initially they weren't used because the codebase was very small, and as it grew, things weren't moved into subdirectories when perhaps they should have. I haven't seen exactly what's going on here with LinuxMain, but will continue looking to see if I can spot the problem. One thing that comes to mind is if a |
|
@abertelrud Thanks for taking a look and offering some advice on resolving the failed build. I think you're onto something with that |
Use target_sources to add Manifest subdirectory
… public access level
mattt
force-pushed
the
packagemodel-module-cleanup
branch
from
29ead68 to
1231679
Compare
This reverts commit aa3f08b.
|
@swift-ci please smoke test |
|
@neonichu @abertelrud FYI: Build is passing now. Thanks for your help and patience reviewing this! |
|
Woohoo! Thanks for your work on this! The code base will be a lot more approachable after the restructuring. |
|
@mattt, it looks as if everything is ready for this one. Do you want me to go ahead and merge it, or do you have other things you want to add? |
|
@abertelrud Please go ahead and merge this. Thanks! |
Following up on #2970, this PR seeks to make code in the PackageModel module more understandable by extracting some standalone types into separate files and moving declarations around to be closer to related APIs.
Notably, this PR moves the
ResolvedPackage,ResolvedProduct, andResolvedTargettypes into the PackageGraph module. In the process of reading through the codebase, these types seemed to be more relevant to the concerns of PackageGraph than to PackageModel where they're currently declared. However, that could just be my lack of understanding how these types fit into the broader picture for SPM.The other change I wanted to call out was 64f6b5d, which attempts to make Manifest.swift more manageable by extracting the
____Descriptiontypes into individual files. Here, I created a subdirectory to group them together; without a common prefix, they'd be intermingled with other types. However, this convention isn't currently used in the codebase. Please let me know if you have a different, more preferable solution. (Alternatively, this grouping could be seen as a code smell, indicating that these types belong somewhere else... interested to hear any thoughts you have)