From 8de92f7249c200e440c835ce40b639fb4bff2438 Mon Sep 17 00:00:00 2001 From: Bri Peticca Date: Tue, 11 Mar 2025 16:58:02 -0400 Subject: [PATCH 1/8] Initial creation of new TraitConfiguration enum To phase in the modification to TraitConfiguration, start with the new enum and match the inits configured for the legacy TraitConfiguration struct --- Sources/CoreCommands/Options.swift | 17 +++++++++++ Sources/PackageGraph/TraitConfiguration.swift | 30 +++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/Sources/CoreCommands/Options.swift b/Sources/CoreCommands/Options.swift index fbe63053edc..1bcc9a065a6 100644 --- a/Sources/CoreCommands/Options.swift +++ b/Sources/CoreCommands/Options.swift @@ -28,6 +28,7 @@ import enum PackageModel.Sanitizer @_spi(SwiftPMInternal) import struct PackageModel.SwiftSDK import struct PackageGraph.TraitConfiguration +import enum PackageGraph.TraitConfiguration2 import struct SPMBuildCore.BuildParameters import struct SPMBuildCore.BuildSystemProvider @@ -711,6 +712,22 @@ extension TraitConfiguration { } } +extension TraitConfiguration2 { + package init(traitOptions: TraitOptions) { + var enabledTraits = traitOptions.enabledTraits + if traitOptions.disableDefaultTraits { + // If there are no enabled traits specified we can disable the + // default trait by passing in an empty set. Otherwise the enabling specific traits + // requires the user to pass the default as well. + enabledTraits = enabledTraits ?? [] + } + self.init( + enabledTraits: enabledTraits, + enableAllTraits: traitOptions.enableAllTraits + ) + } +} + // MARK: - Extensions extension BuildConfiguration { diff --git a/Sources/PackageGraph/TraitConfiguration.swift b/Sources/PackageGraph/TraitConfiguration.swift index db68fcb6941..169a2dceb94 100644 --- a/Sources/PackageGraph/TraitConfiguration.swift +++ b/Sources/PackageGraph/TraitConfiguration.swift @@ -26,3 +26,33 @@ public struct TraitConfiguration: Codable, Hashable { self.enableAllTraits = enableAllTraits } } + +public enum TraitConfiguration2: Codable, Hashable { + case enableAllTraits + case noConfiguration + case noEnabledTraits + case enabledTraits(Set) + + public init( + enabledTraits: Set? = nil, + enableAllTraits: Bool = false + ) { + // If all traits are enabled, then no other checks are necessary. + guard !enableAllTraits else { + self = .enableAllTraits + return + } + + if let enabledTraits { + if enabledTraits.isEmpty { + self = .noEnabledTraits + } else { + self = .enabledTraits(enabledTraits) + } + } else { + // Since enableAllTraits isn't enabled and there isn't a set of traits set, + // there is no configuration passed by the user. + self = .noConfiguration + } + } +} From 544d11ddd31362df6724be7422955556dc61cfdd Mon Sep 17 00:00:00 2001 From: Bri Peticca Date: Thu, 13 Mar 2025 16:25:40 -0400 Subject: [PATCH 2/8] Replace TraitConfiguration struct with enum and modify enabledTraits method * enabledTraits will now instead default to using the new trait config * comment out enabledTraits legacy method in favour of enabledTraits2; begin using this method instead. To rename to enabledTraits. * remove optional TraitConfiguration parameters, since enum encapsulates a "no configuration" case. TODO: * Must still clean up commented out code * Ensure behaviour still operates as expected * Add tests for calculateAllEnabledTraits, as this is now public * Ensure checks on TraitConfiguration aren't redundant i.e. make appropriate use of the new structure * Pre-calculate enabled traits across the root with the TraitConfiguration and store where appropriate so we don't have to recalculate this constantly --- .../PackageCommands/EditCommands.swift | 4 +- .../Commands/PackageCommands/Resolve.swift | 2 +- Sources/CoreCommands/Options.swift | 35 +++--- Sources/CoreCommands/SwiftCommandState.swift | 12 +- .../PackageGraph/ModulesGraph+Loading.swift | 6 +- Sources/PackageGraph/ModulesGraph.swift | 2 +- Sources/PackageGraph/PackageContainer.swift | 4 +- Sources/PackageGraph/PackageGraphRoot.swift | 78 +++++++++++- .../Resolution/DependencyResolutionNode.swift | 11 +- .../PubGrub/PubGrubDependencyResolver.swift | 2 +- .../PubGrub/PubGrubPackageContainer.swift | 2 +- Sources/PackageGraph/TraitConfiguration.swift | 73 +++++++++--- Sources/PackageLoading/ManifestLoader.swift | 1 + Sources/PackageModel/Manifest/Manifest.swift | 111 +++++++++--------- .../SwiftBuildSupport/SwiftBuildSystem.swift | 2 +- .../FileSystemPackageContainer.swift | 4 +- .../RegistryPackageContainer.swift | 4 +- .../SourceControlPackageContainer.swift | 4 +- .../ResolverPrecomputationProvider.swift | 13 +- .../Workspace/Workspace+Configuration.swift | 6 +- .../Workspace/Workspace+Dependencies.swift | 2 +- Sources/Workspace/Workspace+Manifests.swift | 34 +++--- Sources/Workspace/Workspace+Registry.swift | 1 + Sources/Workspace/Workspace.swift | 4 +- .../ManifestExtensions.swift | 8 ++ .../MockPackageGraphs.swift | 10 +- .../_InternalTestSupport/MockWorkspace.swift | 5 +- Sources/_InternalTestSupport/misc.swift | 2 +- Tests/BuildTests/PluginInvocationTests.swift | 3 +- .../PackageGraphPerfTests.swift | 1 + .../PackageGraphTests/ModulesGraphTests.swift | 2 + Tests/PackageGraphTests/PubGrubTests.swift | 7 +- Tests/PackageModelTests/ManifestTests.swift | 8 +- .../RegistryClientTests.swift | 4 +- .../SourceKitLSPAPITests.swift | 11 +- .../ManifestSourceGenerationTests.swift | 1 + .../RegistryPackageContainerTests.swift | 1 + Tests/WorkspaceTests/WorkspaceTests.swift | 3 + .../XCBuildSupportTests/PIFBuilderTests.swift | 3 + 39 files changed, 314 insertions(+), 172 deletions(-) diff --git a/Sources/Commands/PackageCommands/EditCommands.swift b/Sources/Commands/PackageCommands/EditCommands.swift index 6e3bbdc4a0c..92a1dc7e72a 100644 --- a/Sources/Commands/PackageCommands/EditCommands.swift +++ b/Sources/Commands/PackageCommands/EditCommands.swift @@ -36,7 +36,7 @@ extension SwiftPackageCommand { var packageIdentity: String func run(_ swiftCommandState: SwiftCommandState) async throws { - try await swiftCommandState.resolve(nil) + try await swiftCommandState.resolve() let workspace = try swiftCommandState.getActiveWorkspace() // Put the dependency in edit mode. @@ -65,7 +65,7 @@ extension SwiftPackageCommand { var packageIdentity: String func run(_ swiftCommandState: SwiftCommandState) async throws { - try await swiftCommandState.resolve(nil) + try await swiftCommandState.resolve() let workspace = try swiftCommandState.getActiveWorkspace() try await workspace.unedit( diff --git a/Sources/Commands/PackageCommands/Resolve.swift b/Sources/Commands/PackageCommands/Resolve.swift index cae63da126b..07a639014c6 100644 --- a/Sources/Commands/PackageCommands/Resolve.swift +++ b/Sources/Commands/PackageCommands/Resolve.swift @@ -14,7 +14,7 @@ import ArgumentParser import CoreCommands import TSCUtility -import struct PackageGraph.TraitConfiguration +import enum PackageGraph.TraitConfiguration extension SwiftPackageCommand { struct ResolveOptions: ParsableArguments { diff --git a/Sources/CoreCommands/Options.swift b/Sources/CoreCommands/Options.swift index 1bcc9a065a6..5d14bfda2a2 100644 --- a/Sources/CoreCommands/Options.swift +++ b/Sources/CoreCommands/Options.swift @@ -27,8 +27,7 @@ import class PackageModel.Manifest import enum PackageModel.Sanitizer @_spi(SwiftPMInternal) import struct PackageModel.SwiftSDK -import struct PackageGraph.TraitConfiguration -import enum PackageGraph.TraitConfiguration2 +import enum PackageGraph.TraitConfiguration import struct SPMBuildCore.BuildParameters import struct SPMBuildCore.BuildSystemProvider @@ -696,23 +695,23 @@ package struct TraitOptions: ParsableArguments { public var disableDefaultTraits: Bool = false } -extension TraitConfiguration { - package init(traitOptions: TraitOptions) { - var enabledTraits = traitOptions.enabledTraits - if traitOptions.disableDefaultTraits { - // If there are no enabled traits specified we can disable the - // default trait by passing in an empty set. Otherwise the enabling specific traits - // requires the user to pass the default as well. - enabledTraits = enabledTraits ?? [] - } - self.init( - enabledTraits: enabledTraits, - enableAllTraits: traitOptions.enableAllTraits - ) - } -} +//extension TraitConfiguration { +// package init(traitOptions: TraitOptions) { +// var enabledTraits = traitOptions.enabledTraits +// if traitOptions.disableDefaultTraits { +// // If there are no enabled traits specified we can disable the +// // default trait by passing in an empty set. Otherwise the enabling specific traits +// // requires the user to pass the default as well. +// enabledTraits = enabledTraits ?? [] +// } +// self.init( +// enabledTraits: enabledTraits, +// enableAllTraits: traitOptions.enableAllTraits +// ) +// } +//} -extension TraitConfiguration2 { +extension TraitConfiguration { package init(traitOptions: TraitOptions) { var enabledTraits = traitOptions.enabledTraits if traitOptions.disableDefaultTraits { diff --git a/Sources/CoreCommands/SwiftCommandState.swift b/Sources/CoreCommands/SwiftCommandState.swift index 1b21565ea22..3de23b415b0 100644 --- a/Sources/CoreCommands/SwiftCommandState.swift +++ b/Sources/CoreCommands/SwiftCommandState.swift @@ -203,7 +203,7 @@ public final class SwiftCommandState { } /// Get the current workspace root object. - public func getWorkspaceRoot(traitConfiguration: TraitConfiguration? = nil) throws -> PackageGraphRootInput { + public func getWorkspaceRoot(traitConfiguration: TraitConfiguration = .default) throws -> PackageGraphRootInput { let packages: [AbsolutePath] if let workspace = options.locations.multirootPackageDataFile { @@ -437,7 +437,7 @@ public final class SwiftCommandState { } /// Returns the currently active workspace. - public func getActiveWorkspace(emitDeprecatedConfigurationWarning: Bool = false, traitConfiguration: TraitConfiguration? = nil) throws -> Workspace { + public func getActiveWorkspace(emitDeprecatedConfigurationWarning: Bool = false, traitConfiguration: TraitConfiguration = .default) throws -> Workspace { if let workspace = _workspace { return workspace } @@ -501,7 +501,7 @@ public final class SwiftCommandState { return workspace } - public func getRootPackageInformation(traitConfiguration: TraitConfiguration? = nil) async throws -> (dependencies: [PackageIdentity: [PackageIdentity]], targets: [PackageIdentity: [String]]) { + public func getRootPackageInformation(traitConfiguration: TraitConfiguration = .default) async throws -> (dependencies: [PackageIdentity: [PackageIdentity]], targets: [PackageIdentity: [String]]) { let workspace = try self.getActiveWorkspace(traitConfiguration: traitConfiguration) let root = try self.getWorkspaceRoot(traitConfiguration: traitConfiguration) let rootManifests = try await workspace.loadRootManifests( @@ -606,7 +606,7 @@ public final class SwiftCommandState { } /// Resolve the dependencies. - public func resolve(_ traitConfiguration: TraitConfiguration?) async throws { + public func resolve(_ traitConfiguration: TraitConfiguration = .default) async throws { let workspace = try getActiveWorkspace(traitConfiguration: traitConfiguration) let root = try getWorkspaceRoot(traitConfiguration: traitConfiguration) @@ -636,7 +636,7 @@ public final class SwiftCommandState { ) async throws -> ModulesGraph { try await self.loadPackageGraph( explicitProduct: explicitProduct, - traitConfiguration: nil, + traitConfiguration: .default, testEntryPointPath: testEntryPointPath ) } @@ -649,7 +649,7 @@ public final class SwiftCommandState { @discardableResult package func loadPackageGraph( explicitProduct: String? = nil, - traitConfiguration: TraitConfiguration? = nil, + traitConfiguration: TraitConfiguration = .default, testEntryPointPath: AbsolutePath? = nil ) async throws -> ModulesGraph { do { diff --git a/Sources/PackageGraph/ModulesGraph+Loading.swift b/Sources/PackageGraph/ModulesGraph+Loading.swift index bf70dec4106..073be85f1b8 100644 --- a/Sources/PackageGraph/ModulesGraph+Loading.swift +++ b/Sources/PackageGraph/ModulesGraph+Loading.swift @@ -904,8 +904,10 @@ private func calculateEnabledTraits( throw ModuleError.invalidTrait(package: identity, trait: trait) } } - - if let parentPackage, !(explictlyEnabledTraits == nil || areDefaultsEnabled) && manifest.traits.isEmpty { + + // explicitlyEnabledTraits != nil && !areDefaultsEnabled + + if let parentPackage, !(explictlyEnabledTraits == nil || areDefaultsEnabled) && !manifest.supportsTraits { // We throw an error when default traits are disabled for a package without any traits // This allows packages to initially move new API behind traits once. throw ModuleError.disablingDefaultTraitsOnEmptyTraits( diff --git a/Sources/PackageGraph/ModulesGraph.swift b/Sources/PackageGraph/ModulesGraph.swift index 41d007d9a38..a17872bbbf4 100644 --- a/Sources/PackageGraph/ModulesGraph.swift +++ b/Sources/PackageGraph/ModulesGraph.swift @@ -426,7 +426,7 @@ public func loadModulesGraph( useXCBuildFileRules: Bool = false, customXCTestMinimumDeploymentTargets: [PackageModel.Platform: PlatformVersion]? = .none, observabilityScope: ObservabilityScope, - traitConfiguration: TraitConfiguration? = nil + traitConfiguration: TraitConfiguration = .default ) throws -> ModulesGraph { let rootManifests = manifests.filter(\.packageKind.isRoot).spm_createDictionary { ($0.path, $0) } let externalManifests = try manifests.filter { !$0.packageKind.isRoot } diff --git a/Sources/PackageGraph/PackageContainer.swift b/Sources/PackageGraph/PackageContainer.swift index d7e9e4a34b5..58c751f0c8e 100644 --- a/Sources/PackageGraph/PackageContainer.swift +++ b/Sources/PackageGraph/PackageContainer.swift @@ -102,7 +102,7 @@ public protocol PackageContainer { /// Fetch the enabled traits of a package container. /// /// NOTE: This method should only be called on root packages. - func getEnabledTraits(traitConfiguration: TraitConfiguration?, version: Version?) async throws -> Set + func getEnabledTraits(traitConfiguration: TraitConfiguration, version: Version?) async throws -> Set } extension PackageContainer { @@ -118,7 +118,7 @@ extension PackageContainer { return true } - public func getEnabledTraits(traitConfiguration: TraitConfiguration?, version: Version? = nil) async throws -> Set { + public func getEnabledTraits(traitConfiguration: TraitConfiguration, version: Version? = nil) async throws -> Set { return [] } } diff --git a/Sources/PackageGraph/PackageGraphRoot.swift b/Sources/PackageGraph/PackageGraphRoot.swift index 68fca677e18..3565636376b 100644 --- a/Sources/PackageGraph/PackageGraphRoot.swift +++ b/Sources/PackageGraph/PackageGraphRoot.swift @@ -24,13 +24,13 @@ public struct PackageGraphRootInput { public let dependencies: [PackageDependency] /// The trait configuration for the root packages. - public let traitConfiguration: TraitConfiguration? + public let traitConfiguration: TraitConfiguration /// Create a package graph root. public init( packages: [AbsolutePath], dependencies: [PackageDependency] = [], - traitConfiguration: TraitConfiguration? = nil + traitConfiguration: TraitConfiguration = .default ) { self.packages = packages self.dependencies = dependencies @@ -103,16 +103,21 @@ public struct PackageGraphRoot { } }) + var enableTraitsMap: [PackageIdentity: Set] = [:] do { // Calculate the enabled traits for root. - self.enabledTraits = try packages.reduce(into: [PackageIdentity: Set]()) { traitsMap, package in + enableTraitsMap = try packages.reduce(into: [PackageIdentity: Set]()) { traitsMap, package in let manifest = package.value.manifest let traitConfiguration = input.traitConfiguration - let enabledTraits = try manifest.enabledTraits(using: traitConfiguration?.enabledTraits, enableAllTraits: traitConfiguration?.enableAllTraits ?? false) + // Should only ever have to use trait configuration here. +// let enabledTraits = try manifest.enabledTraits(using: traitConfiguration.enabledTraits, enableAllTraits: traitConfiguration.enableAllTraits) + let enabledTraits = try manifest.enabledTraits2(using: traitConfiguration) traitsMap[package.key] = enabledTraits } + + self.enabledTraits = enableTraitsMap } catch { self.enabledTraits = [:] } @@ -131,7 +136,11 @@ public struct PackageGraphRoot { // is enabled. return manifests.values.reduce(false) { guard $1.pruneDependencies else { return $0 || true } - if let isUsed = try? $1.isPackageDependencyUsed(dep, enabledTraits: input.traitConfiguration?.enabledTraits, enableAllTraits: input.traitConfiguration?.enableAllTraits ?? false) { +// if let isUsed = try? $1.isPackageDependencyUsed(dep, enabledTraits: input.traitConfiguration?.enabledTraits, enableAllTraits: input.traitConfiguration?.enableAllTraits ?? false) { +// return $0 || isUsed +// } + let enabledTraits: Set? = enableTraitsMap[$1.packageIdentity] + if let isUsed = try? $1.isPackageDependencyUsed(dep, enabledTraits: enabledTraits) { return $0 || isUsed } return true @@ -140,7 +149,14 @@ public struct PackageGraphRoot { if let explicitProduct { // FIXME: `dependenciesRequired` modifies manifests and prevents conversion of `Manifest` to a value type - let deps = try? manifests.values.lazy.map({ try $0.dependenciesRequired(for: .everything, input.traitConfiguration?.enabledTraits, enableAllTraits: input.traitConfiguration?.enableAllTraits ?? false) }).flatMap({ $0 }) +// let deps = try? manifests.values.lazy.map({ try $0.dependenciesRequired(for: .everything, input.traitConfiguration.enabledTraits, enableAllTraits: input.traitConfiguration.enableAllTraits) }).flatMap({ $0 }) + let deps = try? manifests.values.lazy + .map({ manifest -> [PackageDependency] in + let enabledTraits: Set? = enableTraitsMap[manifest.packageIdentity] + return try manifest.dependenciesRequired(for: .everything, enabledTraits) + }) + .flatMap({ $0 }) + for dependency in deps ?? [] { adjustedDependencies.append(dependency.filtered(by: .specific([explicitProduct]))) } @@ -224,3 +240,53 @@ extension PackageDependency.Registry.Requirement { } } } + +// TODO: bp to move to Manifest+Traits.swift file +extension Manifest { + public func enabledTraits2(using traitConfiguration: TraitConfiguration) throws -> Set? { + guard supportsTraits else { +// if var explicitTraits { +// explicitTraits.remove("default") +// if !explicitTraits.isEmpty { +// throw TraitError.traitsNotSupported( +// package: displayName, +// explicitlyEnabledTraits: traits.map(\.name) +// ) +// } +// } + // if no defaults enabled AND enabled traits is not empty (minus default traits) + if var explicitTraits = traitConfiguration.enabledTraits { + explicitTraits.remove("default") + if !explicitTraits.isEmpty { + throw TraitError.traitsNotSupported( + package: displayName, + explicitlyEnabledTraits: traits.map(\.name) + ) + } + } + + return nil + } + + var enabledTraits: Set = [] + + switch traitConfiguration { + case .enableAllTraits: + enabledTraits = Set(traits.map(\.name)) + case .noConfiguration: + if let defaultTraits = defaultTraits?.map(\.name) { + enabledTraits = Set(defaultTraits) + } + case .noEnabledTraits: + return [] + case .enabledTraits(let explicitlyEnabledTraits): + enabledTraits = explicitlyEnabledTraits + } + + if let allEnabledTraits = try? calculateAllEnabledTraits(explictlyEnabledTraits: enabledTraits) { + enabledTraits = allEnabledTraits + } + + return enabledTraits + } +} diff --git a/Sources/PackageGraph/Resolution/DependencyResolutionNode.swift b/Sources/PackageGraph/Resolution/DependencyResolutionNode.swift index d7f343f0bf7..c9e9b53d35f 100644 --- a/Sources/PackageGraph/Resolution/DependencyResolutionNode.swift +++ b/Sources/PackageGraph/Resolution/DependencyResolutionNode.swift @@ -58,7 +58,7 @@ public enum DependencyResolutionNode { /// It is a warning condition, and builds do not actually need these dependencies. /// However, forcing the graph to resolve and fetch them anyway allows the diagnostics passes access /// to the information needed in order to provide actionable suggestions to help the user stitch up the dependency declarations properly. - case root(package: PackageReference, traitConfiguration: TraitConfiguration? = nil) + case root(package: PackageReference, traitConfiguration: TraitConfiguration = .default) /// The package. public var package: PackageReference { @@ -94,7 +94,14 @@ public enum DependencyResolutionNode { public var traits: Set? { switch self { case .root(_, let config): - return config?.enabledTraits + switch config { + case .enabledTraits(let traits): + return traits + case .noEnabledTraits: + return [] + case .noConfiguration, .enableAllTraits: // TODO: bp fix + return nil + } case .product(_, _, let enabledTraits): return enabledTraits default: diff --git a/Sources/PackageGraph/Resolution/PubGrub/PubGrubDependencyResolver.swift b/Sources/PackageGraph/Resolution/PubGrub/PubGrubDependencyResolver.swift index a41560d5f76..71e39debc1e 100644 --- a/Sources/PackageGraph/Resolution/PubGrub/PubGrubDependencyResolver.swift +++ b/Sources/PackageGraph/Resolution/PubGrub/PubGrubDependencyResolver.swift @@ -165,7 +165,7 @@ public struct PubGrubDependencyResolver { } /// Execute the resolution algorithm to find a valid assignment of versions. - public func solve(constraints: [Constraint], traitConfiguration: TraitConfiguration? = nil) async -> Result<[DependencyResolverBinding], Error> { + public func solve(constraints: [Constraint], traitConfiguration: TraitConfiguration = .default) async -> Result<[DependencyResolverBinding], Error> { // the graph resolution root let root: DependencyResolutionNode if constraints.count == 1, let constraint = constraints.first, constraint.package.kind.isRoot { diff --git a/Sources/PackageGraph/Resolution/PubGrub/PubGrubPackageContainer.swift b/Sources/PackageGraph/Resolution/PubGrub/PubGrubPackageContainer.swift index 04603d23a68..7974e0dbb03 100644 --- a/Sources/PackageGraph/Resolution/PubGrub/PubGrubPackageContainer.swift +++ b/Sources/PackageGraph/Resolution/PubGrub/PubGrubPackageContainer.swift @@ -155,7 +155,7 @@ final class PubGrubPackageContainer { node: DependencyResolutionNode, overriddenPackages: [PackageReference: (version: BoundVersion, products: ProductFilter)], root: DependencyResolutionNode, - traitConfiguration: TraitConfiguration? = nil + traitConfiguration: TraitConfiguration = .default ) async throws -> [Incompatibility] { // FIXME: It would be nice to compute bounds for this as well. if await !self.underlying.isToolsVersionCompatible(at: version) { diff --git a/Sources/PackageGraph/TraitConfiguration.swift b/Sources/PackageGraph/TraitConfiguration.swift index 169a2dceb94..9bef4f1001c 100644 --- a/Sources/PackageGraph/TraitConfiguration.swift +++ b/Sources/PackageGraph/TraitConfiguration.swift @@ -11,23 +11,23 @@ //===----------------------------------------------------------------------===// /// The trait configuration. -public struct TraitConfiguration: Codable, Hashable { - /// The traits to enable for the package. - package var enabledTraits: Set? - - /// Enables all traits of the package. - package var enableAllTraits: Bool - - public init( - enabledTraits: Set? = nil, - enableAllTraits: Bool = false - ) { - self.enabledTraits = enabledTraits - self.enableAllTraits = enableAllTraits - } -} +//public struct TraitConfiguration: Codable, Hashable { +// /// The traits to enable for the package. +// package var enabledTraits: Set? +// +// /// Enables all traits of the package. +// package var enableAllTraits: Bool +// +// public init( +// enabledTraits: Set? = nil, +// enableAllTraits: Bool = false +// ) { +// self.enabledTraits = enabledTraits +// self.enableAllTraits = enableAllTraits +// } +//} -public enum TraitConfiguration2: Codable, Hashable { +public enum TraitConfiguration: Codable, Hashable { case enableAllTraits case noConfiguration case noEnabledTraits @@ -55,4 +55,45 @@ public enum TraitConfiguration2: Codable, Hashable { self = .noConfiguration } } + + /// Default instance of `TraitConfiguration`. + public static var `default`: TraitConfiguration { + .init(enabledTraits: nil) + } + + public var enabledTraits: Set? { + switch self { + case .enabledTraits(let traits): + return traits + case .noConfiguration, .enableAllTraits: + return nil + case .noEnabledTraits: + return [] + } + } + + public var enableAllTraits: Bool { + if case .enableAllTraits = self { + return true + } + + return false + } + +// public var enablesDefaultTraits: Bool { +// switch self { +// case .enableAllTraits, .noConfiguration: +// return true +// case .enabledTraits(let traits): +// return traits.contains("default") +// case .noEnabledTraits: +// return false +// } +// } + +// public var enabledTraitsWithoutDefault: Set? { +// switch self { +// case .enabledTraits(let traits): +// } +// } } diff --git a/Sources/PackageLoading/ManifestLoader.swift b/Sources/PackageLoading/ManifestLoader.swift index bc77765c0cf..9fbb9bbe74b 100644 --- a/Sources/PackageLoading/ManifestLoader.swift +++ b/Sources/PackageLoading/ManifestLoader.swift @@ -440,6 +440,7 @@ public final class ManifestLoader: ManifestLoaderProtocol { let manifest = Manifest( displayName: parsedManifest.name, + packageIdentity: packageIdentity, path: manifestPath, packageKind: packageKind, packageLocation: packageLocation, diff --git a/Sources/PackageModel/Manifest/Manifest.swift b/Sources/PackageModel/Manifest/Manifest.swift index 118997b95b1..579ef7ab4b0 100644 --- a/Sources/PackageModel/Manifest/Manifest.swift +++ b/Sources/PackageModel/Manifest/Manifest.swift @@ -30,6 +30,9 @@ public final class Manifest: Sendable { /// FIXME: deprecate this, there is no value in this once we have real package identifiers public let displayName: String + /// The package identity. + public let packageIdentity: PackageIdentity + // FIXME: deprecate this, this is not part of the manifest information, we just use it as a container for this data // FIXME: This doesn't belong here, we want the Manifest to be purely tied // to the repository state, it shouldn't matter where it is. @@ -108,6 +111,7 @@ public final class Manifest: Sendable { public init( displayName: String, + packageIdentity: PackageIdentity, path: AbsolutePath, packageKind: PackageReference.Kind, packageLocation: String, @@ -128,6 +132,7 @@ public final class Manifest: Sendable { pruneDependencies: Bool = false ) { self.displayName = displayName + self.packageIdentity = packageIdentity self.path = path self.packageKind = packageKind self.packageLocation = packageLocation @@ -182,18 +187,18 @@ public final class Manifest: Sendable { } /// Returns a list of dependencies that are being guarded by traits. - public func dependenciesGuarded(by enabledTraits: Set?, enableAllTraits: Bool = false) -> [PackageDependency] { + public func dependenciesGuarded(by enabledTraits: Set?) -> [PackageDependency] { guard supportsTraits else { return [] } let traitGuardedDeps = self.traitGuardedDependencies(lowercasedKeys: true) - let explicitlyEnabledTraits = try? self.enabledTraits(using: enabledTraits, enableAllTraits: enableAllTraits) +// let explicitlyEnabledTraits = try? self.enabledTraits(using: enabledTraits, enableAllTraits: enableAllTraits) guard self.toolsVersion >= .v5_2 && !self.packageKind.isRoot else { let deps = self.dependencies.filter({ - if let guardTraits = traitGuardedDeps[$0.identity.description]?.values.flatMap({ $0 }), !guardTraits.isEmpty, let explicitlyEnabledTraits + if let guardTraits = traitGuardedDeps[$0.identity.description]?.values.flatMap({ $0 }), !guardTraits.isEmpty, let enabledTraits { - return !guardTraits.allSatisfy({ explicitlyEnabledTraits.contains($0) }) + return !guardTraits.allSatisfy({ enabledTraits.contains($0) }) } return false @@ -203,9 +208,9 @@ public final class Manifest: Sendable { if let dependencies = self._requiredDependencies[.nothing] { let deps = dependencies.filter({ - if let guardTraits = traitGuardedDeps[$0.identity.description]?.values.flatMap({ $0 }), let explicitlyEnabledTraits + if let guardTraits = traitGuardedDeps[$0.identity.description]?.values.flatMap({ $0 }), let enabledTraits { - return !guardTraits.allSatisfy({ explicitlyEnabledTraits.contains($0) }) + return !guardTraits.allSatisfy({ enabledTraits.contains($0) }) } return false @@ -221,7 +226,7 @@ public final class Manifest: Sendable { continue } - if let explicitlyEnabledTraits, guardingTraits.intersection(explicitlyEnabledTraits) != guardingTraits { + if let enabledTraits, guardingTraits.intersection(enabledTraits) != guardingTraits { guardedDependencies.insert(dependency.identity) } } @@ -232,7 +237,7 @@ public final class Manifest: Sendable { else { return } - if let explicitlyEnabledTraits, guardingTraits.intersection(explicitlyEnabledTraits) != guardingTraits { + if let enabledTraits, guardingTraits.intersection(enabledTraits) != guardingTraits { guardedDependencies.insert(dependency.identity) } } @@ -244,7 +249,7 @@ public final class Manifest: Sendable { } /// Returns the package dependencies required for a particular products filter and trait configuration. - public func dependenciesRequired(for productFilter: ProductFilter, _ enabledTraits: Set?, enableAllTraits: Bool = false) throws -> [PackageDependency] { + public func dependenciesRequired(for productFilter: ProductFilter, _ enabledTraits: Set?) throws -> [PackageDependency] { #if ENABLE_TARGET_BASED_DEPENDENCY_RESOLUTION // If we have already calculated it, returned the cached value. if let dependencies = self._requiredDependencies[productFilter] { @@ -256,13 +261,13 @@ public final class Manifest: Sendable { return dependencies } #else - let explicitlyEnabledTraits: Set? = try self.enabledTraits(using: enabledTraits, enableAllTraits: enableAllTraits) +// let explicitlyEnabledTraits: Set? = try self.enabledTraits(using: enabledTraits, enableAllTraits: enableAllTraits) guard self.toolsVersion >= .v5_2 && !self.packageKind.isRoot else { var dependencies = self.dependencies if pruneDependencies { dependencies = try dependencies.filter({ - return try self.isPackageDependencyUsed($0, enabledTraits: explicitlyEnabledTraits) + return try self.isPackageDependencyUsed($0, enabledTraits: enabledTraits) }) } return dependencies @@ -274,7 +279,7 @@ public final class Manifest: Sendable { if var dependencies = self._requiredDependencies[.nothing] { if pruneDependencies { dependencies = try dependencies.filter({ - return try self.isPackageDependencyUsed($0, enabledTraits: explicitlyEnabledTraits) + return try self.isPackageDependencyUsed($0, enabledTraits: enabledTraits) }) } return dependencies @@ -282,7 +287,7 @@ public final class Manifest: Sendable { var requiredDependencies: Set = [] for target in self.targetsRequired(for: self.products) { for targetDependency in target.dependencies { - guard try self.isTargetDependencyEnabled(target: target.name, targetDependency, enabledTraits: explicitlyEnabledTraits) else { continue } + guard try self.isTargetDependencyEnabled(target: target.name, targetDependency, enabledTraits: enabledTraits) else { continue } if let dependency = self.packageDependency(referencedBy: targetDependency) { requiredDependencies.insert(dependency.identity) } @@ -668,40 +673,40 @@ extension Manifest { /// Calculates the set of all transitive traits that are enabled for this manifest using the passed set of explicitly enabled traits and a flag that /// determines whether all traits are enabled. - public func enabledTraits(using explicitTraits: Set?, enableAllTraits: Bool = false) throws -> Set? { - guard supportsTraits else { - if var explicitTraits { - explicitTraits.remove("default") - if !explicitTraits.isEmpty { - throw TraitError.traitsNotSupported( - package: displayName, - explicitlyEnabledTraits: traits.map(\.name) - ) - } - } - - return nil - } - - var enabledTraits = explicitTraits - - if enableAllTraits { - enabledTraits = (enabledTraits ?? []).union(Set(traits.map(\.name))) - } - - if let allEnabledTraits = try? calculateAllEnabledTraits(explictlyEnabledTraits: enabledTraits) { - enabledTraits = allEnabledTraits - } - - return enabledTraits - } +// public func enabledTraits(using explicitTraits: Set?, enableAllTraits: Bool = false) throws -> Set? { +// guard supportsTraits else { +// if var explicitTraits { +// explicitTraits.remove("default") +// if !explicitTraits.isEmpty { +// throw TraitError.traitsNotSupported( +// package: displayName, +// explicitlyEnabledTraits: traits.map(\.name) +// ) +// } +// } +// +// return nil +// } +// +// var enabledTraits = explicitTraits +// +// if enableAllTraits { +// enabledTraits = (enabledTraits ?? []).union(Set(traits.map(\.name))) +// } +// +// if let allEnabledTraits = try? calculateAllEnabledTraits(explictlyEnabledTraits: enabledTraits) { +// enabledTraits = allEnabledTraits +// } +// +// return enabledTraits +// } /// Given a trait, determine if the trait is enabled given the current set of enabled traits. - public func isTraitEnabled(_ trait: TraitDescription, _ explicitTraits: Set?, _ enableAllTraits: Bool = false) throws -> Bool { + public func isTraitEnabled(_ trait: TraitDescription, _ enabledTraits: Set?) throws -> Bool { guard supportsTraits else { - if var explicitTraits { - explicitTraits.remove("default") - if !explicitTraits.isEmpty { + if var enabledTraits { + enabledTraits.remove("default") + if !enabledTraits.isEmpty { throw TraitError.invalidTrait( package: displayName, trait: trait.name, @@ -723,14 +728,14 @@ extension Manifest { ) } - let allEnabledTraits = try enabledTraits(using: explicitTraits, enableAllTraits: enableAllTraits) ?? [] + let allEnabledTraits = try calculateAllEnabledTraits(explictlyEnabledTraits: enabledTraits) return allEnabledTraits.contains(trait.name) } /// Calculates and returns a set of all enabled traits, beginning with a set of explicitly enabled traits (either defined by default traits of /// this manifest, or by a user-generated traits configuration) and determines which traits are transitively enabled. - private func calculateAllEnabledTraits(explictlyEnabledTraits: Set?) throws -> Set { + public func calculateAllEnabledTraits(explictlyEnabledTraits: Set?) throws -> Set { // This the point where we flatten the enabled traits and resolve the recursive traits var enabledTraits = explictlyEnabledTraits ?? [] let areDefaultsEnabled = enabledTraits.remove("default") != nil @@ -775,7 +780,7 @@ extension Manifest { } /// Computes the dependencies that are in use per target in this manifest. - public func usedTargetDependencies(withTraits enabledTraits: Set?, enableAllTraits: Bool = false) throws -> [String: Set] { + public func usedTargetDependencies(withTraits enabledTraits: Set?) throws -> [String: Set] { return try self.targets.reduce(into: [String: Set]()) { depMap, target in let nonTraitDeps = target.dependencies.filter { $0.condition?.traits?.isEmpty ?? true @@ -786,7 +791,7 @@ extension Manifest { // For each trait that is a condition on this target dependency, assure that // each one is enabled in the manifest. - return try traits.allSatisfy({ try isTraitEnabled(.init(stringLiteral: $0), enabledTraits, enableAllTraits) }) + return try traits.allSatisfy({ try isTraitEnabled(.init(stringLiteral: $0), enabledTraits) }) } let deps = nonTraitDeps + traitGuardedDeps @@ -796,9 +801,7 @@ extension Manifest { /// Computes the set of package dependencies that are used by targets of this manifest. public func usedDependencies(withTraits enabledTraits: Set?, enableAllTraits: Bool = false) throws -> (knownPackage: Set, unknownPackage: Set) { - let deps = try self.usedTargetDependencies( - withTraits: enabledTraits, - enableAllTraits: enableAllTraits) + let deps = try self.usedTargetDependencies(withTraits: enabledTraits) .values .flatMap({ $0 }) .compactMap(\.package) @@ -851,14 +854,14 @@ extension Manifest { } let traitsThatEnableDependency = traitGuardedDependencies()[package]?[target.name] ?? [] - let isEnabled = try traitsThatEnableDependency.allSatisfy({ try isTraitEnabled(.init(stringLiteral: $0), enabledTraits, enableAllTraits) }) + let isEnabled = try traitsThatEnableDependency.allSatisfy({ try isTraitEnabled(.init(stringLiteral: $0), enabledTraits) }) return traitsThatEnableDependency.isEmpty || isEnabled } /// Determines whether a given package dependency is used by this manifest given a set of enabled traits. - public func isPackageDependencyUsed(_ dependency: PackageDependency, enabledTraits: Set?, enableAllTraits: Bool = false) throws -> Bool { - let usedDependencies = try self.usedDependencies(withTraits: enabledTraits, enableAllTraits: enableAllTraits) + public func isPackageDependencyUsed(_ dependency: PackageDependency, enabledTraits: Set?) throws -> Bool { + let usedDependencies = try self.usedDependencies(withTraits: enabledTraits) let foundKnownPackage = usedDependencies.knownPackage.contains(where: { $0.caseInsensitiveCompare(dependency.identity.description) == .orderedSame }) diff --git a/Sources/SwiftBuildSupport/SwiftBuildSystem.swift b/Sources/SwiftBuildSupport/SwiftBuildSystem.swift index 4613a77b728..9c1885dfe0f 100644 --- a/Sources/SwiftBuildSupport/SwiftBuildSystem.swift +++ b/Sources/SwiftBuildSupport/SwiftBuildSystem.swift @@ -52,7 +52,7 @@ func withSession( _ diagnostics: [SwiftBuild.SwiftBuildMessage.DiagnosticInfo] ) async throws -> Void ) async throws { - switch await service.createSession(name: name, resourceSearchPaths: packageManagerResourcesDirectory.map { [$0.pathString] } ?? [], cachePath: nil, inferiorProductsPath: nil, environment: nil) { + switch await service.createSession(name: name, cachePath: nil, inferiorProductsPath: nil, environment: nil) { case (.success(let session), let diagnostics): do { try await body(session, diagnostics) diff --git a/Sources/Workspace/PackageContainer/FileSystemPackageContainer.swift b/Sources/Workspace/PackageContainer/FileSystemPackageContainer.swift index d3d9e5c7003..052c4e9027a 100644 --- a/Sources/Workspace/PackageContainer/FileSystemPackageContainer.swift +++ b/Sources/Workspace/PackageContainer/FileSystemPackageContainer.swift @@ -136,7 +136,7 @@ public struct FileSystemPackageContainer: PackageContainer { fatalError("This should never be called") } - public func getEnabledTraits(traitConfiguration: TraitConfiguration?, at version: Version? = nil) async throws -> Set { + public func getEnabledTraits(traitConfiguration: TraitConfiguration, at version: Version? = nil) async throws -> Set { guard version == nil else { throw InternalError("File system package container does not support versioning.") } @@ -144,7 +144,7 @@ public struct FileSystemPackageContainer: PackageContainer { guard manifest.packageKind.isRoot else { return [] } - let enabledTraits = try manifest.enabledTraits(using: traitConfiguration?.enabledTraits, enableAllTraits: traitConfiguration?.enableAllTraits ?? false) + let enabledTraits = try manifest.enabledTraits2(using: traitConfiguration) return enabledTraits ?? [] } } diff --git a/Sources/Workspace/PackageContainer/RegistryPackageContainer.swift b/Sources/Workspace/PackageContainer/RegistryPackageContainer.swift index 1854c2e0ac9..1a56c93b0cd 100644 --- a/Sources/Workspace/PackageContainer/RegistryPackageContainer.swift +++ b/Sources/Workspace/PackageContainer/RegistryPackageContainer.swift @@ -254,7 +254,7 @@ public class RegistryPackageContainer: PackageContainer { } } - public func getEnabledTraits(traitConfiguration: TraitConfiguration?, at version: Version?) async throws -> Set { + public func getEnabledTraits(traitConfiguration: TraitConfiguration, at version: Version?) async throws -> Set { guard let version else { throw InternalError("Version needed to compute enabled traits for registry package \(self.package.identity.description)") } @@ -262,7 +262,7 @@ public class RegistryPackageContainer: PackageContainer { guard manifest.packageKind.isRoot else { return [] } - let enabledTraits = try manifest.enabledTraits(using: traitConfiguration?.enabledTraits, enableAllTraits: traitConfiguration?.enableAllTraits ?? false) + let enabledTraits = try manifest.enabledTraits2(using: traitConfiguration) return enabledTraits ?? [] } } diff --git a/Sources/Workspace/PackageContainer/SourceControlPackageContainer.swift b/Sources/Workspace/PackageContainer/SourceControlPackageContainer.swift index 4bc7be1a26c..4b40888eacf 100644 --- a/Sources/Workspace/PackageContainer/SourceControlPackageContainer.swift +++ b/Sources/Workspace/PackageContainer/SourceControlPackageContainer.swift @@ -419,12 +419,12 @@ internal final class SourceControlPackageContainer: PackageContainer, CustomStri } } - public func getEnabledTraits(traitConfiguration: TraitConfiguration?, at revision: String?, version: Version?) async throws -> Set { + public func getEnabledTraits(traitConfiguration: TraitConfiguration, at revision: String?, version: Version?) async throws -> Set { guard let version, let tag = getTag(for: version) else { return [] } let manifest = try await self.loadManifest(tag: tag, version: version) - return try manifest.enabledTraits(using: traitConfiguration?.enabledTraits, enableAllTraits: traitConfiguration?.enableAllTraits ?? false) ?? [] + return try manifest.enabledTraits2(using: traitConfiguration) ?? [] } public var isRemoteContainer: Bool? { diff --git a/Sources/Workspace/ResolverPrecomputationProvider.swift b/Sources/Workspace/ResolverPrecomputationProvider.swift index 1c3ea8ade29..c98f3f3bc86 100644 --- a/Sources/Workspace/ResolverPrecomputationProvider.swift +++ b/Sources/Workspace/ResolverPrecomputationProvider.swift @@ -177,25 +177,26 @@ private struct LocalPackageContainer: PackageContainer { } } - func getEnabledTraits(traitConfiguration: TraitConfiguration?, at version: Version? = nil) async throws -> Set { + func getEnabledTraits(traitConfiguration: TraitConfiguration, at version: Version? = nil) async throws -> Set { guard manifest.packageKind.isRoot else { return [] } - let configurationEnabledTraits = traitConfiguration?.enabledTraits - let enableAllTraits = traitConfiguration?.enableAllTraits ?? false +// let configurationEnabledTraits = traitConfiguration?.enabledTraits +// let enableAllTraits = traitConfiguration?.enableAllTraits ?? false if let version { switch dependency?.state { case .sourceControlCheckout(.version(version, revision: _)): - return try manifest.enabledTraits(using: configurationEnabledTraits, enableAllTraits: enableAllTraits) ?? [] +// return try manifest.enabledTraits2(using: configurationEnabledTraits, enableAllTraits: enableAllTraits) ?? [] + return try manifest.enabledTraits2(using: traitConfiguration) ?? [] case .registryDownload(version: version): - return try manifest.enabledTraits(using: configurationEnabledTraits, enableAllTraits: enableAllTraits) ?? [] + return try manifest.enabledTraits2(using: traitConfiguration) ?? [] default: throw InternalError("expected version based state, but state was \(String(describing: dependency?.state))") } } else { - return try manifest.enabledTraits(using: configurationEnabledTraits, enableAllTraits: enableAllTraits) ?? [] + return try manifest.enabledTraits2(using: traitConfiguration) ?? [] } } } diff --git a/Sources/Workspace/Workspace+Configuration.swift b/Sources/Workspace/Workspace+Configuration.swift index 1bfc7d62636..671ffba09f4 100644 --- a/Sources/Workspace/Workspace+Configuration.swift +++ b/Sources/Workspace/Workspace+Configuration.swift @@ -796,7 +796,7 @@ public struct WorkspaceConfiguration { public var pruneDependencies: Bool /// The trait configuration for the root. - public var traitConfiguration: TraitConfiguration? + public var traitConfiguration: TraitConfiguration public init( skipDependenciesUpdates: Bool, @@ -813,7 +813,7 @@ public struct WorkspaceConfiguration { manifestImportRestrictions: (startingToolsVersion: ToolsVersion, allowedImports: [String])?, usePrebuilts: Bool, pruneDependencies: Bool, - traitConfiguration: TraitConfiguration? + traitConfiguration: TraitConfiguration ) { self.skipDependenciesUpdates = skipDependenciesUpdates self.prefetchBasedOnResolvedFile = prefetchBasedOnResolvedFile @@ -849,7 +849,7 @@ public struct WorkspaceConfiguration { manifestImportRestrictions: .none, usePrebuilts: false, pruneDependencies: false, - traitConfiguration: nil + traitConfiguration: .default ) } diff --git a/Sources/Workspace/Workspace+Dependencies.swift b/Sources/Workspace/Workspace+Dependencies.swift index f6af11802d9..e1b97b4312f 100644 --- a/Sources/Workspace/Workspace+Dependencies.swift +++ b/Sources/Workspace/Workspace+Dependencies.swift @@ -47,7 +47,7 @@ import struct SourceControl.Revision import struct TSCUtility.Version import struct PackageModel.TargetDescription import struct PackageModel.TraitDescription -import struct PackageGraph.TraitConfiguration +import enum PackageGraph.TraitConfiguration import class PackageModel.Manifest extension Workspace { diff --git a/Sources/Workspace/Workspace+Manifests.swift b/Sources/Workspace/Workspace+Manifests.swift index 6d10022e28a..e92bef552c8 100644 --- a/Sources/Workspace/Workspace+Manifests.swift +++ b/Sources/Workspace/Workspace+Manifests.swift @@ -27,7 +27,7 @@ import protocol PackageGraph.CustomPackageContainer import struct PackageGraph.GraphLoadingNode import struct PackageGraph.PackageContainerConstraint import struct PackageGraph.PackageGraphRoot -import struct PackageGraph.TraitConfiguration +import enum PackageGraph.TraitConfiguration import class PackageLoading.ManifestLoader import struct PackageLoading.ManifestValidator import struct PackageLoading.ToolsVersionParser @@ -219,7 +219,7 @@ extension Workspace { inputIdentities.append(package.reference) var traits: Set? = [] if let enabledTraits = rootEnabledTraitsMap[package.reference.identity] { - traits = try package.manifest.enabledTraits(using: enabledTraits) + traits = enabledTraits//try package.manifest.enabledTraits(using: enabledTraits) } let node = try GraphLoadingNode( identity: identity, @@ -236,7 +236,7 @@ extension Workspace { if let enabledTraits = rootDependenciesEnabledTraitsMap[dependency.identity] { // Recursively calculate the enabled traits of this package. - traits = try manifest.enabledTraits(using: enabledTraits) + traits = enabledTraits//try manifest.enabledTraits(using: enabledTraits) } return try GraphLoadingNode( identity: dependency.identity, @@ -329,9 +329,10 @@ extension Workspace { // Calculate all transitively enabled traits for this manifest. var allEnabledTraits: Set = [] - if let explicitlyEnabledTraits, - let calculatedTraits = try manifest.enabledTraits(using: Set(explicitlyEnabledTraits)) { - allEnabledTraits = calculatedTraits + if let explicitlyEnabledTraits +// let calculatedTraits = try manifest.enabledTraits(using: Set(explicitlyEnabledTraits)) + { + allEnabledTraits = Set(explicitlyEnabledTraits) } return try GraphLoadingNode( @@ -410,7 +411,7 @@ extension Workspace { guard let conditionTraits = $0.condition?.traits else { return true } - return !conditionTraits.intersection(workspace.configuration.traitConfiguration?.enabledTraits ?? []).isEmpty + return !conditionTraits.intersection(workspace.configuration.traitConfiguration.enabledTraits ?? []).isEmpty }.map(\.name) ?? [] traitMap[dependency.identity] = Set(explicitlyEnabledTraits) @@ -576,13 +577,15 @@ extension Workspace { let rootManifests = try root.manifests.mapValues({ manifest in let deps = try manifest.dependencies.filter({ dep in guard configuration.pruneDependencies else { return true } - let explicitlyEnabledTraits = try manifest.enabledTraits(using: configuration.traitConfiguration?.enabledTraits, enableAllTraits: configuration.traitConfiguration?.enableAllTraits ?? false) +// let explicitlyEnabledTraits = try manifest.enabledTraits(using: configuration.traitConfiguration?.enabledTraits, enableAllTraits: configuration.traitConfiguration?.enableAllTraits ?? false) + let explicitlyEnabledTraits = try manifest.enabledTraits2(using: configuration.traitConfiguration) let isDepUsed = try manifest.isPackageDependencyUsed(dep, enabledTraits: explicitlyEnabledTraits) return isDepUsed }) return Manifest( displayName: manifest.displayName, + packageIdentity: manifest.packageIdentity, path: manifest.path, packageKind: manifest.packageKind, packageLocation: manifest.packageLocation, @@ -614,7 +617,8 @@ extension Workspace { guard configuration.pruneDependencies else { return true } var config = configuration.traitConfiguration if manifest.packageKind.isRoot { - let manifestEnabledTraits = try manifest.enabledTraits(using: configuration.traitConfiguration?.enabledTraits, enableAllTraits: configuration.traitConfiguration?.enableAllTraits ?? false) +// let manifestEnabledTraits = try manifest.enabledTraits(using: configuration.traitConfiguration?.enabledTraits, enableAllTraits: configuration.traitConfiguration?.enableAllTraits ?? false) + let manifestEnabledTraits = try manifest.enabledTraits2(using: configuration.traitConfiguration) config = .init(enabledTraits: manifestEnabledTraits) } else { let rootManifests = root.manifests.values.filter({ $0.packageKind.isRoot }) @@ -637,10 +641,10 @@ extension Workspace { } } - manifestEnabledTraits = try manifest.enabledTraits(using: manifestEnabledTraits) +// manifestEnabledTraits = try manifest.enabledTraits(using: manifestEnabledTraits) config = .init(enabledTraits: manifestEnabledTraits) } - let isDepUsed = try manifest.isPackageDependencyUsed(dep, enabledTraits: config?.enabledTraits) + let isDepUsed = try manifest.isPackageDependencyUsed(dep, enabledTraits: config.enabledTraits) return isDepUsed }).map(\.packageRef) }).flatMap(\.self) @@ -679,8 +683,10 @@ extension Workspace { }.map(\.name) var allEnabledTraits: Set = [] - if let explicitlyEnabledTraits, let calculatedTraits = try $0.enabledTraits(using: Set(explicitlyEnabledTraits)) { - allEnabledTraits = calculatedTraits + if let explicitlyEnabledTraits +// let calculatedTraits = try $0.enabledTraits(using: Set(explicitlyEnabledTraits)) + { + allEnabledTraits = Set(explicitlyEnabledTraits) } return $0.canonicalPackageLocation == dependency.packageRef.canonicalLocation ? @@ -704,7 +710,7 @@ extension Workspace { let manifestGraphRoots = try topLevelManifests.map { identity, manifest in // Since these represent the root manifests, can pass in the enabled traits from the trait configuration here, as that is what it represents. let isRoot = manifest.packageKind.isRoot - let enabledTraits = isRoot ? try manifest.enabledTraits(using: configuration.traitConfiguration?.enabledTraits, enableAllTraits: configuration.traitConfiguration?.enableAllTraits ?? false) : [] + let enabledTraits = isRoot ? try manifest.enabledTraits2(using: configuration.traitConfiguration) : [] return try KeyedPair( GraphLoadingNode( identity: identity, diff --git a/Sources/Workspace/Workspace+Registry.swift b/Sources/Workspace/Workspace+Registry.swift index a1d1f2d9190..ab1196be50e 100644 --- a/Sources/Workspace/Workspace+Registry.swift +++ b/Sources/Workspace/Workspace+Registry.swift @@ -315,6 +315,7 @@ extension Workspace { let modifiedManifest = Manifest( displayName: manifest.displayName, + packageIdentity: manifest.packageIdentity, path: manifest.path, packageKind: manifest.packageKind, packageLocation: manifest.packageLocation, diff --git a/Sources/Workspace/Workspace.swift b/Sources/Workspace/Workspace.swift index 7b3488650cf..6f49cda78df 100644 --- a/Sources/Workspace/Workspace.swift +++ b/Sources/Workspace/Workspace.swift @@ -985,7 +985,7 @@ extension Workspace { try await self.loadPackageGraph( rootPath: rootPath, explicitProduct: explicitProduct, - traitConfiguration: nil, + traitConfiguration: .default, observabilityScope: observabilityScope ) } @@ -994,7 +994,7 @@ extension Workspace { package func loadPackageGraph( rootPath: AbsolutePath, explicitProduct: String? = nil, - traitConfiguration: TraitConfiguration? = nil, + traitConfiguration: TraitConfiguration = .default, observabilityScope: ObservabilityScope ) async throws -> ModulesGraph { try await self.loadPackageGraph( diff --git a/Sources/_InternalTestSupport/ManifestExtensions.swift b/Sources/_InternalTestSupport/ManifestExtensions.swift index 90034880eaa..7a4cf3b9b10 100644 --- a/Sources/_InternalTestSupport/ManifestExtensions.swift +++ b/Sources/_InternalTestSupport/ManifestExtensions.swift @@ -39,6 +39,7 @@ extension Manifest { displayName: displayName, path: path, packageKind: .root(path), + packageIdentity: .plain(displayName), packageLocation: path.pathString, defaultLocalization: defaultLocalization, platforms: platforms, @@ -79,6 +80,7 @@ extension Manifest { displayName: displayName, path: path, packageKind: .fileSystem(path), + packageIdentity: .plain(displayName), packageLocation: path.pathString, defaultLocalization: defaultLocalization, platforms: platforms, @@ -118,6 +120,7 @@ extension Manifest { displayName: displayName, path: path, packageKind: .localSourceControl(path), + packageIdentity: .plain(displayName), packageLocation: path.pathString, defaultLocalization: defaultLocalization, platforms: platforms, @@ -158,6 +161,7 @@ extension Manifest { displayName: displayName, path: path, packageKind: .remoteSourceControl(url), + packageIdentity: .plain(displayName), packageLocation: url.absoluteString, defaultLocalization: defaultLocalization, platforms: platforms, @@ -197,6 +201,7 @@ extension Manifest { displayName: displayName, path: path, packageKind: .registry(identity), + packageIdentity: .plain(displayName), packageLocation: identity.description, defaultLocalization: defaultLocalization, platforms: platforms, @@ -218,6 +223,7 @@ extension Manifest { displayName: String, path: AbsolutePath = .root, packageKind: PackageReference.Kind, + packageIdentity: PackageIdentity, packageLocation: String? = nil, defaultLocalization: String? = nil, platforms: [PlatformDescription] = [], @@ -236,6 +242,7 @@ extension Manifest { ) -> Manifest { return Manifest( displayName: displayName, + packageIdentity: packageIdentity, path: path.basename == Manifest.filename ? path : path.appending(component: Manifest.filename), packageKind: packageKind, packageLocation: packageLocation ?? path.pathString, @@ -260,6 +267,7 @@ extension Manifest { public func with(location: String) -> Manifest { Manifest( displayName: self.displayName, + packageIdentity: self.packageIdentity, path: self.path, packageKind: self.packageKind, packageLocation: location, diff --git a/Sources/_InternalTestSupport/MockPackageGraphs.swift b/Sources/_InternalTestSupport/MockPackageGraphs.swift index 259e7ef2231..7e60fa7c41d 100644 --- a/Sources/_InternalTestSupport/MockPackageGraphs.swift +++ b/Sources/_InternalTestSupport/MockPackageGraphs.swift @@ -126,7 +126,7 @@ package func macrosPackageGraph() throws -> MockPackageGraph { ), ], observabilityScope: observability.topScope, - traitConfiguration: nil + traitConfiguration: .default ) XCTAssertNoDiagnostics(observability.diagnostics) @@ -284,7 +284,7 @@ package func macrosTestsPackageGraph() throws -> MockPackageGraph { ), ], observabilityScope: observability.topScope, - traitConfiguration: nil + traitConfiguration: .default ) XCTAssertNoDiagnostics(observability.diagnostics) @@ -317,7 +317,7 @@ package func trivialPackageGraph() throws -> MockPackageGraph { ), ], observabilityScope: observability.topScope, - traitConfiguration: nil + traitConfiguration: .default ) XCTAssertNoDiagnostics(observability.diagnostics) @@ -361,7 +361,7 @@ package func embeddedCxxInteropPackageGraph() throws -> MockPackageGraph { ), ], observabilityScope: observability.topScope, - traitConfiguration: nil + traitConfiguration: .default ) XCTAssertNoDiagnostics(observability.diagnostics) @@ -426,7 +426,7 @@ package func toolsExplicitLibrariesGraph(linkage: ProductType.LibraryType) throw ), ], observabilityScope: observability.topScope, - traitConfiguration: nil + traitConfiguration: .default ) XCTAssertNoDiagnostics(observability.diagnostics) diff --git a/Sources/_InternalTestSupport/MockWorkspace.swift b/Sources/_InternalTestSupport/MockWorkspace.swift index 46bc374776e..a04d830af16 100644 --- a/Sources/_InternalTestSupport/MockWorkspace.swift +++ b/Sources/_InternalTestSupport/MockWorkspace.swift @@ -90,7 +90,7 @@ public final class MockWorkspace { let skipDependenciesUpdates: Bool public var sourceControlToRegistryDependencyTransformation: WorkspaceConfiguration.SourceControlToRegistryDependencyTransformation var defaultRegistry: Registry? - public let traitConfiguration: TraitConfiguration? + public let traitConfiguration: TraitConfiguration public init( sandbox: AbsolutePath, @@ -110,7 +110,7 @@ public final class MockWorkspace { sourceControlToRegistryDependencyTransformation: WorkspaceConfiguration.SourceControlToRegistryDependencyTransformation = .disabled, defaultRegistry: Registry? = .none, customHostTriple: Triple = hostTriple, - traitConfiguration: TraitConfiguration? = nil + traitConfiguration: TraitConfiguration = .default ) async throws { try fileSystem.createMockToolchain() @@ -288,6 +288,7 @@ public final class MockWorkspace { displayName: package.name, path: packagePath, packageKind: packageKind, + packageIdentity: .plain(package.name), packageLocation: packageLocation, platforms: package.platforms, version: v, diff --git a/Sources/_InternalTestSupport/misc.swift b/Sources/_InternalTestSupport/misc.swift index 9febddfa75a..fe4ac206dcd 100644 --- a/Sources/_InternalTestSupport/misc.swift +++ b/Sources/_InternalTestSupport/misc.swift @@ -402,7 +402,7 @@ public func loadPackageGraph( useXCBuildFileRules: Bool = false, customXCTestMinimumDeploymentTargets: [PackageModel.Platform: PlatformVersion]? = .none, observabilityScope: ObservabilityScope, - traitConfiguration: TraitConfiguration? + traitConfiguration: TraitConfiguration = .default ) throws -> ModulesGraph { try loadModulesGraph( identityResolver: identityResolver, diff --git a/Tests/BuildTests/PluginInvocationTests.swift b/Tests/BuildTests/PluginInvocationTests.swift index 5a49d0a3c4b..2601571748e 100644 --- a/Tests/BuildTests/PluginInvocationTests.swift +++ b/Tests/BuildTests/PluginInvocationTests.swift @@ -81,8 +81,7 @@ final class PluginInvocationTests: XCTestCase { ] ) ], - observabilityScope: observability.topScope, - traitConfiguration: nil + observabilityScope: observability.topScope ) // Check the basic integrity before running plugins. diff --git a/Tests/PackageGraphPerformanceTests/PackageGraphPerfTests.swift b/Tests/PackageGraphPerformanceTests/PackageGraphPerfTests.swift index 92b7788ef0e..5ffb2235d8e 100644 --- a/Tests/PackageGraphPerformanceTests/PackageGraphPerfTests.swift +++ b/Tests/PackageGraphPerformanceTests/PackageGraphPerfTests.swift @@ -66,6 +66,7 @@ final class PackageGraphPerfTests: XCTestCasePerf { displayName: name, path: try AbsolutePath(validating: location).appending(component: Manifest.filename), packageKind: isRoot ? .root(try .init(validating: location)) : .localSourceControl(try .init(validating: location)), + packageIdentity: .plain(name), packageLocation: location, platforms: [], version: "1.0.0", diff --git a/Tests/PackageGraphTests/ModulesGraphTests.swift b/Tests/PackageGraphTests/ModulesGraphTests.swift index 6b793af0d58..9c77082567b 100644 --- a/Tests/PackageGraphTests/ModulesGraphTests.swift +++ b/Tests/PackageGraphTests/ModulesGraphTests.swift @@ -3903,6 +3903,7 @@ extension Manifest { displayName: self.displayName, path: self.path.parentDirectory, packageKind: self.packageKind, + packageIdentity: self.packageIdentity, packageLocation: self.packageLocation, toolsVersion: self.toolsVersion, dependencies: self.dependencies, @@ -3915,6 +3916,7 @@ extension Manifest { displayName: self.displayName, path: self.path.parentDirectory, packageKind: self.packageKind, + packageIdentity: self.packageIdentity, packageLocation: self.packageLocation, toolsVersion: self.toolsVersion, dependencies: dependencies, diff --git a/Tests/PackageGraphTests/PubGrubTests.swift b/Tests/PackageGraphTests/PubGrubTests.swift index eea4c038ee3..974db2432ac 100644 --- a/Tests/PackageGraphTests/PubGrubTests.swift +++ b/Tests/PackageGraphTests/PubGrubTests.swift @@ -316,7 +316,7 @@ final class PubGrubTests: XCTestCase { let deps = try builder.create(dependencies: [ "foo": (.versionSet(v1Range), .specific(["foo"])) ]) - let result = await resolver.solve(constraints: deps, traitConfiguration: nil) + let result = await resolver.solve(constraints: deps) switch result { case .failure(let error): @@ -1615,7 +1615,7 @@ final class PubGrubTests: XCTestCase { "foo": (.versionSet(v1Range), .specific(["foo"])), "bar": (.versionSet(v2Range), .specific(["bar"])), ]) - let result = await resolver.solve(constraints: dependencies, traitConfiguration: nil) + let result = await resolver.solve(constraints: dependencies) AssertResult(result, [ ("foo", .version("1.1.0")), @@ -1660,8 +1660,7 @@ final class PubGrubTests: XCTestCase { package: other ), overriddenPackages: [:], - root: .root(package: root), - traitConfiguration: nil + root: .root(package: root) ) XCTAssertEqual( result, diff --git a/Tests/PackageModelTests/ManifestTests.swift b/Tests/PackageModelTests/ManifestTests.swift index 2e36e15a58d..e54adafce56 100644 --- a/Tests/PackageModelTests/ManifestTests.swift +++ b/Tests/PackageModelTests/ManifestTests.swift @@ -326,7 +326,7 @@ Trait '"\(trait.name)"' is not declared by package 'Foo'. There are no available // Calculate the enabled traits without an explicitly declared set of enabled traits. // This should default to fetching the default traits, if they exist (which in this test case // they do), and then will calculate the transitive set of traits that are enabled. - let allEnabledTraits = try manifest.enabledTraits(using: nil)?.sorted() + let allEnabledTraits = try manifest.enabledTraits2(using: .default)?.sorted() XCTAssertEqual(allEnabledTraits, ["Trait1", "Trait2"]) } } @@ -360,13 +360,13 @@ Trait '"\(trait.name)"' is not declared by package 'Foo'. There are no available // Calculate the enabled traits with an explicitly declared set of enabled traits. // This should override the default traits (since it isn't explicitly passed in here). - let allEnabledTraitsWithoutDefaults = try manifest.enabledTraits(using: ["Trait3"])?.sorted() + let allEnabledTraitsWithoutDefaults = try manifest.enabledTraits2(using: .enabledTraits(["Trait3"]))?.sorted() XCTAssertEqual(allEnabledTraitsWithoutDefaults, ["Trait3"]) // Calculate the enabled traits with an explicitly declared set of enabled traits, // including the default traits. Since default traits are explicitly enabled in the // passed set of traits, this will be factored into the calculation. - let allEnabledTraitsWithDefaults = try manifest.enabledTraits(using: ["default", "Trait3"])?.sorted() + let allEnabledTraitsWithDefaults = try manifest.enabledTraits2(using: .enabledTraits(["default", "Trait3"]))?.sorted() XCTAssertEqual(allEnabledTraitsWithDefaults, ["Trait1", "Trait2", "Trait3"]) } @@ -400,7 +400,7 @@ Trait '"\(trait.name)"' is not declared by package 'Foo'. There are no available ) // Calculate the enabled traits with all traits enabled flag. - let allEnabledTraits = try manifest.enabledTraits(using: [], enableAllTraits: true)?.sorted() + let allEnabledTraits = try manifest.enabledTraits2(using: .enableAllTraits)?.sorted() XCTAssertEqual(allEnabledTraits, ["Trait1", "Trait2", "Trait3"]) } } diff --git a/Tests/PackageRegistryTests/RegistryClientTests.swift b/Tests/PackageRegistryTests/RegistryClientTests.swift index e9c824e812c..dc59f4daf12 100644 --- a/Tests/PackageRegistryTests/RegistryClientTests.swift +++ b/Tests/PackageRegistryTests/RegistryClientTests.swift @@ -1360,7 +1360,7 @@ final class RegistryClientTests: XCTestCase { version: version, customToolsVersion: nil, observabilityScope: ObservabilitySystem.NOOP, - callbackQueue: .sharedConcurrent, + callbackQueue: .sharedConcurrent ) { continuation.resume(with: $0) } } let parsedToolsVersion = try ToolsVersionParser.parse(utf8String: manifestSync) @@ -3419,7 +3419,7 @@ final class RegistryClientTests: XCTestCase { signatureFormat: .none, fileSystem: localFileSystem, observabilityScope: ObservabilitySystem.NOOP, - callbackQueue: .sharedConcurrent, + callbackQueue: .sharedConcurrent ) { result in continuation.resume(with: result) } } diff --git a/Tests/SourceKitLSPAPITests/SourceKitLSPAPITests.swift b/Tests/SourceKitLSPAPITests/SourceKitLSPAPITests.swift index fd4ee893e92..8231e03238b 100644 --- a/Tests/SourceKitLSPAPITests/SourceKitLSPAPITests.swift +++ b/Tests/SourceKitLSPAPITests/SourceKitLSPAPITests.swift @@ -59,8 +59,7 @@ final class SourceKitLSPAPITests: XCTestCase { TargetDescription(name: "plugin", type: .plugin, pluginCapability: .buildTool) ]), ], - observabilityScope: observability.topScope, - traitConfiguration: nil + observabilityScope: observability.topScope ) XCTAssertNoDiagnostics(observability.diagnostics) @@ -147,8 +146,7 @@ final class SourceKitLSPAPITests: XCTestCase { ] ), ], - observabilityScope: observability.topScope, - traitConfiguration: nil + observabilityScope: observability.topScope ) XCTAssertNoDiagnostics(observability.diagnostics) @@ -219,8 +217,7 @@ final class SourceKitLSPAPITests: XCTestCase { ] ), ], - observabilityScope: observability.topScope, - traitConfiguration: nil + observabilityScope: observability.topScope ) XCTAssertNoDiagnostics(observability.diagnostics) @@ -326,7 +323,7 @@ final class SourceKitLSPAPITests: XCTestCase { func testClangOutputPaths() async throws { let fs = InMemoryFileSystem(emptyFiles: "/Pkg/Sources/lib/include/lib.h", - "/Pkg/Sources/lib/lib.cpp", + "/Pkg/Sources/lib/lib.cpp" ) let observability = ObservabilitySystem.makeForTesting() diff --git a/Tests/WorkspaceTests/ManifestSourceGenerationTests.swift b/Tests/WorkspaceTests/ManifestSourceGenerationTests.swift index 9d150cf8736..ca7240d75a5 100644 --- a/Tests/WorkspaceTests/ManifestSourceGenerationTests.swift +++ b/Tests/WorkspaceTests/ManifestSourceGenerationTests.swift @@ -517,6 +517,7 @@ final class ManifestSourceGenerationTests: XCTestCase { displayName: "MyLibrary", path: packageDir.appending("Package.swift"), packageKind: .root("/tmp/MyLibrary"), + packageIdentity: .plain("MyLibrary"), packageLocation: packageDir.pathString, platforms: [], toolsVersion: .v5_5, diff --git a/Tests/WorkspaceTests/RegistryPackageContainerTests.swift b/Tests/WorkspaceTests/RegistryPackageContainerTests.swift index 0f777655947..92aa55dd45d 100644 --- a/Tests/WorkspaceTests/RegistryPackageContainerTests.swift +++ b/Tests/WorkspaceTests/RegistryPackageContainerTests.swift @@ -275,6 +275,7 @@ final class RegistryPackageContainerTests: XCTestCase { displayName: packageIdentity.description, path: manifestPath, packageKind: packageKind, + packageIdentity: packageIdentity, packageLocation: packageLocation, platforms: [], toolsVersion: manifestToolsVersion diff --git a/Tests/WorkspaceTests/WorkspaceTests.swift b/Tests/WorkspaceTests/WorkspaceTests.swift index 495c60828cd..a1e1c23fb74 100644 --- a/Tests/WorkspaceTests/WorkspaceTests.swift +++ b/Tests/WorkspaceTests/WorkspaceTests.swift @@ -2917,6 +2917,7 @@ final class WorkspaceTests: XCTestCase { displayName: manifest.displayName, path: manifest.path, packageKind: manifest.packageKind, + packageIdentity: manifest.packageIdentity, packageLocation: manifest.packageLocation, platforms: [], version: manifest.version, @@ -12342,6 +12343,7 @@ final class WorkspaceTests: XCTestCase { displayName: packageIdentity.description, path: manifestPath, packageKind: packageKind, + packageIdentity: packageIdentity, packageLocation: packageLocation, platforms: [], toolsVersion: manifestToolsVersion @@ -15038,6 +15040,7 @@ final class WorkspaceTests: XCTestCase { displayName: "Foo", path: packagePath.appending(component: Manifest.filename), packageKind: .registry("org.foo"), + packageIdentity: .plain("Foo"), packageLocation: "org.foo", toolsVersion: .current, products: [ diff --git a/Tests/XCBuildSupportTests/PIFBuilderTests.swift b/Tests/XCBuildSupportTests/PIFBuilderTests.swift index 1aebcbd52a3..8afe9a29197 100644 --- a/Tests/XCBuildSupportTests/PIFBuilderTests.swift +++ b/Tests/XCBuildSupportTests/PIFBuilderTests.swift @@ -133,6 +133,7 @@ final class PIFBuilderTests: XCTestCase { displayName: "Foo", path: "/Foo", packageKind: .root("/Foo"), + packageIdentity: .plain("Foo"), defaultLocalization: "fr", toolsVersion: .v5_2, dependencies: [ @@ -1926,6 +1927,7 @@ final class PIFBuilderTests: XCTestCase { displayName: "Bar", path: "/Bar", packageKind: .root("/Bar"), + packageIdentity: .plain("Bar"), toolsVersion: .v4_2, cLanguageStandard: "c11", swiftLanguageVersions: [.v4_2], @@ -2688,6 +2690,7 @@ final class PIFBuilderTests: XCTestCase { displayName: "Foo", path: "/Foo", packageKind: .root("/Foo"), + packageIdentity: .plain("Foo"), toolsVersion: .v5_3, targets: [ .init(name: "foo", dependencies: [ From 8e347599102d565ea9df2ecc3e5867f7e9e92cc9 Mon Sep 17 00:00:00 2001 From: Bri Peticca Date: Tue, 18 Mar 2025 17:00:49 -0400 Subject: [PATCH 3/8] Allow PackageGraphRoot init to throw + cleanup - Since we will calculate all enabled traits for root manifests in the PackageGraphRoot init (which can throw depending on whether there is a non-allowable trait configuration passed), allow the init to throw this error as well. - Clean up some commented code - Replace redundant calculations of enabled traits since we now store the calculated traits in the PackageGraphRoot TODO: - Address disallow disabling traits test not catching error - More cleanup of commented out code --- Sources/CoreCommands/Options.swift | 16 ---- .../PackageGraph/ModulesGraph+Loading.swift | 1 - Sources/PackageGraph/ModulesGraph.swift | 2 +- Sources/PackageGraph/PackageGraphRoot.swift | 74 +++++++-------- Sources/PackageGraph/TraitConfiguration.swift | 39 ++++---- Sources/PackageModel/Manifest/Manifest.swift | 21 +++-- .../FileSystemPackageContainer.swift | 1 + .../ResolverPrecomputationProvider.swift | 4 - .../Workspace/Workspace+Dependencies.swift | 6 +- Sources/Workspace/Workspace+Manifests.swift | 92 +++++++++---------- .../_InternalTestSupport/MockWorkspace.swift | 4 +- Sources/swift-bootstrap/main.swift | 2 +- Tests/FunctionalTests/TraitTests.swift | 9 +- Tests/PackageModelTests/ManifestTests.swift | 4 +- 14 files changed, 128 insertions(+), 147 deletions(-) diff --git a/Sources/CoreCommands/Options.swift b/Sources/CoreCommands/Options.swift index 2309c66059b..86afd00fd9f 100644 --- a/Sources/CoreCommands/Options.swift +++ b/Sources/CoreCommands/Options.swift @@ -689,22 +689,6 @@ package struct TraitOptions: ParsableArguments { public var disableDefaultTraits: Bool = false } -//extension TraitConfiguration { -// package init(traitOptions: TraitOptions) { -// var enabledTraits = traitOptions.enabledTraits -// if traitOptions.disableDefaultTraits { -// // If there are no enabled traits specified we can disable the -// // default trait by passing in an empty set. Otherwise the enabling specific traits -// // requires the user to pass the default as well. -// enabledTraits = enabledTraits ?? [] -// } -// self.init( -// enabledTraits: enabledTraits, -// enableAllTraits: traitOptions.enableAllTraits -// ) -// } -//} - extension TraitConfiguration { package init(traitOptions: TraitOptions) { var enabledTraits = traitOptions.enabledTraits diff --git a/Sources/PackageGraph/ModulesGraph+Loading.swift b/Sources/PackageGraph/ModulesGraph+Loading.swift index f1379a7963e..939246126ef 100644 --- a/Sources/PackageGraph/ModulesGraph+Loading.swift +++ b/Sources/PackageGraph/ModulesGraph+Loading.swift @@ -967,7 +967,6 @@ private func calculateEnabledTraits( } // explicitlyEnabledTraits != nil && !areDefaultsEnabled - if let parentPackage, !(explictlyEnabledTraits == nil || areDefaultsEnabled) && !manifest.supportsTraits { // We throw an error when default traits are disabled for a package without any traits // This allows packages to initially move new API behind traits once. diff --git a/Sources/PackageGraph/ModulesGraph.swift b/Sources/PackageGraph/ModulesGraph.swift index a17872bbbf4..99f42480447 100644 --- a/Sources/PackageGraph/ModulesGraph.swift +++ b/Sources/PackageGraph/ModulesGraph.swift @@ -439,7 +439,7 @@ public func loadModulesGraph( let packages = Array(rootManifests.keys) let input = PackageGraphRootInput(packages: packages, traitConfiguration: traitConfiguration) - let graphRoot = PackageGraphRoot( + let graphRoot = try PackageGraphRoot( input: input, manifests: rootManifests, explicitProduct: explicitProduct, diff --git a/Sources/PackageGraph/PackageGraphRoot.swift b/Sources/PackageGraph/PackageGraphRoot.swift index 3565636376b..8e5c1876c3f 100644 --- a/Sources/PackageGraph/PackageGraphRoot.swift +++ b/Sources/PackageGraph/PackageGraphRoot.swift @@ -49,6 +49,7 @@ public struct PackageGraphRoot { return self.packages.compactMapValues { $0.manifest } } + /// The root manifest(s)'s enabled traits (and their transitively enabled traits). public var enabledTraits: [PackageIdentity: Set] /// The root package references. @@ -94,7 +95,7 @@ public struct PackageGraphRoot { explicitProduct: String? = nil, dependencyMapper: DependencyMapper? = nil, observabilityScope: ObservabilityScope - ) { + ) throws { self.packages = input.packages.reduce(into: .init(), { partial, inputPath in if let manifest = manifests[inputPath] { let packagePath = manifest.path.parentDirectory @@ -103,25 +104,19 @@ public struct PackageGraphRoot { } }) + // Calculate the enabled traits for root. var enableTraitsMap: [PackageIdentity: Set] = [:] - do { - // Calculate the enabled traits for root. - enableTraitsMap = try packages.reduce(into: [PackageIdentity: Set]()) { traitsMap, package in - let manifest = package.value.manifest - let traitConfiguration = input.traitConfiguration - - // Should only ever have to use trait configuration here. -// let enabledTraits = try manifest.enabledTraits(using: traitConfiguration.enabledTraits, enableAllTraits: traitConfiguration.enableAllTraits) - let enabledTraits = try manifest.enabledTraits2(using: traitConfiguration) - - traitsMap[package.key] = enabledTraits - } + enableTraitsMap = try packages.reduce(into: [PackageIdentity: Set]()) { traitsMap, package in + let manifest = package.value.manifest + let traitConfiguration = input.traitConfiguration - self.enabledTraits = enableTraitsMap - } catch { - self.enabledTraits = [:] + // Should only ever have to use trait configuration here for roots. + let enabledTraits = try manifest.enabledTraits2(using: traitConfiguration) + traitsMap[package.key] = enabledTraits } + self.enabledTraits = enableTraitsMap + // FIXME: Deprecate special casing once the manifest supports declaring used executable products. // Special casing explicit products like this is necessary to pass the test suite and satisfy backwards compatibility. // However, changing the dependencies based on the command line arguments may force `Package.resolved` to temporarily change, @@ -134,22 +129,19 @@ public struct PackageGraphRoot { // Check that the dependency is used in at least one of the manifests. // If not, then we can omit this dependency if pruning unused dependencies // is enabled. - return manifests.values.reduce(false) { - guard $1.pruneDependencies else { return $0 || true } -// if let isUsed = try? $1.isPackageDependencyUsed(dep, enabledTraits: input.traitConfiguration?.enabledTraits, enableAllTraits: input.traitConfiguration?.enableAllTraits ?? false) { -// return $0 || isUsed -// } - let enabledTraits: Set? = enableTraitsMap[$1.packageIdentity] - if let isUsed = try? $1.isPackageDependencyUsed(dep, enabledTraits: enabledTraits) { - return $0 || isUsed + return manifests.values.reduce(false) { result, manifest in + guard manifest.pruneDependencies else { return true } + let enabledTraits: Set? = enableTraitsMap[manifest.packageIdentity] + if let isUsed = try? manifest.isPackageDependencyUsed(dep, enabledTraits: enabledTraits) { + return result || isUsed } + return true } }) if let explicitProduct { // FIXME: `dependenciesRequired` modifies manifests and prevents conversion of `Manifest` to a value type -// let deps = try? manifests.values.lazy.map({ try $0.dependenciesRequired(for: .everything, input.traitConfiguration.enabledTraits, enableAllTraits: input.traitConfiguration.enableAllTraits) }).flatMap({ $0 }) let deps = try? manifests.values.lazy .map({ manifest -> [PackageDependency] in let enabledTraits: Set? = enableTraitsMap[manifest.packageIdentity] @@ -243,26 +235,22 @@ extension PackageDependency.Registry.Requirement { // TODO: bp to move to Manifest+Traits.swift file extension Manifest { + /// Calculates the set of all transitive traits that are enabled for this manifest using the passed set of + /// explicitly enabled traits and a flag that + /// determines whether all traits are enabled. public func enabledTraits2(using traitConfiguration: TraitConfiguration) throws -> Set? { guard supportsTraits else { -// if var explicitTraits { -// explicitTraits.remove("default") -// if !explicitTraits.isEmpty { -// throw TraitError.traitsNotSupported( -// package: displayName, -// explicitlyEnabledTraits: traits.map(\.name) -// ) -// } -// } - // if no defaults enabled AND enabled traits is not empty (minus default traits) - if var explicitTraits = traitConfiguration.enabledTraits { - explicitTraits.remove("default") - if !explicitTraits.isEmpty { - throw TraitError.traitsNotSupported( - package: displayName, - explicitlyEnabledTraits: traits.map(\.name) - ) - } + // If this manifest does not support traits, but the passed configuration either + // disables default traits or enables non-default traits (i.e. traits that would + // not exist for this manifest) then we must throw an error. + + if !traitConfiguration.enablesDefaultTraits && traitConfiguration.enablesNonDefaultTraits { + // TODO: bp add parent package to error message like in modulesgraph+loading.swift + // TODO: bp fix explicitlyEnabledTraits value passed here + throw TraitError.traitsNotSupported( + package: displayName, + explicitlyEnabledTraits: traits.map(\.name) + ) } return nil diff --git a/Sources/PackageGraph/TraitConfiguration.swift b/Sources/PackageGraph/TraitConfiguration.swift index 9bef4f1001c..c03d59c1cec 100644 --- a/Sources/PackageGraph/TraitConfiguration.swift +++ b/Sources/PackageGraph/TraitConfiguration.swift @@ -11,22 +11,6 @@ //===----------------------------------------------------------------------===// /// The trait configuration. -//public struct TraitConfiguration: Codable, Hashable { -// /// The traits to enable for the package. -// package var enabledTraits: Set? -// -// /// Enables all traits of the package. -// package var enableAllTraits: Bool -// -// public init( -// enabledTraits: Set? = nil, -// enableAllTraits: Bool = false -// ) { -// self.enabledTraits = enabledTraits -// self.enableAllTraits = enableAllTraits -// } -//} - public enum TraitConfiguration: Codable, Hashable { case enableAllTraits case noConfiguration @@ -80,6 +64,29 @@ public enum TraitConfiguration: Codable, Hashable { return false } + public var enablesDefaultTraits: Bool { + switch self { + case .enabledTraits(let traits): + return traits.contains("default") + case .noConfiguration, .enableAllTraits: + return true + case .noEnabledTraits: + return false + } + } + + public var enablesNonDefaultTraits: Bool { + switch self { + case .enabledTraits(let traits): + let traitsWithoutDefault = traits.subtracting(["default"]) + return !traitsWithoutDefault.isEmpty + case .enableAllTraits: + return true + case .noConfiguration, .noEnabledTraits: + return false + } + } + // public var enablesDefaultTraits: Bool { // switch self { // case .enableAllTraits, .noConfiguration: diff --git a/Sources/PackageModel/Manifest/Manifest.swift b/Sources/PackageModel/Manifest/Manifest.swift index 951c821ba09..e2d926a0b4c 100644 --- a/Sources/PackageModel/Manifest/Manifest.swift +++ b/Sources/PackageModel/Manifest/Manifest.swift @@ -286,7 +286,6 @@ public final class Manifest: Sendable { return dependencies } #else -// let explicitlyEnabledTraits: Set? = try self.enabledTraits(using: enabledTraits, enableAllTraits: enableAllTraits) guard self.toolsVersion >= .v5_2 && !self.packageKind.isRoot else { var dependencies = self.dependencies @@ -298,8 +297,6 @@ public final class Manifest: Sendable { return dependencies } - // calculate explicitly enabled traits through config: - // using .nothing as cache key while ENABLE_TARGET_BASED_DEPENDENCY_RESOLUTION is false if var dependencies = self._requiredDependencies[.nothing] { if self.pruneDependencies { @@ -764,10 +761,7 @@ extension Manifest { return allEnabledTraits.contains(trait.name) } - /// Calculates and returns a set of all enabled traits, beginning with a set of explicitly enabled traits (either - /// defined by default traits of - /// this manifest, or by a user-generated traits configuration) and determines which traits are transitively - /// enabled. + /// Calculates and returns a set of all enabled traits, beginning with a set of explicitly enabled traits (which can either be the default traits of a manifest, or a configuration of enabled traits determined from a user-generated trait configuration) and determines which traits are transitively enabled. public func calculateAllEnabledTraits(explictlyEnabledTraits: Set?) throws -> Set { // This the point where we flatten the enabled traits and resolve the recursive traits var enabledTraits = explictlyEnabledTraits ?? [] @@ -780,6 +774,19 @@ extension Manifest { } } + if !(explictlyEnabledTraits == nil || areDefaultsEnabled) && !self.supportsTraits { + // We throw an error when default traits are disabled for a package without any traits + // This allows packages to initially move new API behind traits once. +// throw ModuleError.disablingDefaultTraitsOnEmptyTraits( +// parentPackage: parentPackage, +// packageName: manifest.displayName +// ) + throw TraitError.traitsNotSupported( + package: displayName, + explicitlyEnabledTraits: explictlyEnabledTraits?.map({ $0 }) ?? [] + ) + } + // We have to enable all default traits if no traits are enabled or the defaults are explicitly enabled if explictlyEnabledTraits == nil || areDefaultsEnabled { if let defaultTraits { diff --git a/Sources/Workspace/PackageContainer/FileSystemPackageContainer.swift b/Sources/Workspace/PackageContainer/FileSystemPackageContainer.swift index 052c4e9027a..1a2aea3d2fb 100644 --- a/Sources/Workspace/PackageContainer/FileSystemPackageContainer.swift +++ b/Sources/Workspace/PackageContainer/FileSystemPackageContainer.swift @@ -142,6 +142,7 @@ public struct FileSystemPackageContainer: PackageContainer { } let manifest = try await loadManifest() guard manifest.packageKind.isRoot else { + // TODO: bp throw? return [] } let enabledTraits = try manifest.enabledTraits2(using: traitConfiguration) diff --git a/Sources/Workspace/ResolverPrecomputationProvider.swift b/Sources/Workspace/ResolverPrecomputationProvider.swift index c98f3f3bc86..77a67a80786 100644 --- a/Sources/Workspace/ResolverPrecomputationProvider.swift +++ b/Sources/Workspace/ResolverPrecomputationProvider.swift @@ -182,13 +182,9 @@ private struct LocalPackageContainer: PackageContainer { return [] } -// let configurationEnabledTraits = traitConfiguration?.enabledTraits -// let enableAllTraits = traitConfiguration?.enableAllTraits ?? false - if let version { switch dependency?.state { case .sourceControlCheckout(.version(version, revision: _)): -// return try manifest.enabledTraits2(using: configurationEnabledTraits, enableAllTraits: enableAllTraits) ?? [] return try manifest.enabledTraits2(using: traitConfiguration) ?? [] case .registryDownload(version: version): return try manifest.enabledTraits2(using: traitConfiguration) ?? [] diff --git a/Sources/Workspace/Workspace+Dependencies.swift b/Sources/Workspace/Workspace+Dependencies.swift index e1b97b4312f..1a241619de5 100644 --- a/Sources/Workspace/Workspace+Dependencies.swift +++ b/Sources/Workspace/Workspace+Dependencies.swift @@ -82,7 +82,7 @@ extension Workspace { let resolvedFileOriginHash = try self.computeResolvedFileOriginHash(root: root) // Load the current manifests. - let graphRoot = PackageGraphRoot( + let graphRoot = try PackageGraphRoot( input: root, manifests: rootManifests, dependencyMapper: self.dependencyMapper, @@ -350,7 +350,7 @@ extension Workspace { packages: root.packages, observabilityScope: observabilityScope ) - let graphRoot = PackageGraphRoot( + let graphRoot = try PackageGraphRoot( input: root, manifests: rootManifests, explicitProduct: explicitProduct, @@ -517,7 +517,7 @@ extension Workspace { let resolvedFileOriginHash = try self.computeResolvedFileOriginHash(root: root) // Load the current manifests. - let graphRoot = PackageGraphRoot( + let graphRoot = try PackageGraphRoot( input: root, manifests: rootManifests, explicitProduct: explicitProduct, diff --git a/Sources/Workspace/Workspace+Manifests.swift b/Sources/Workspace/Workspace+Manifests.swift index 5aa4d1207e9..2d56b7f3643 100644 --- a/Sources/Workspace/Workspace+Manifests.swift +++ b/Sources/Workspace/Workspace+Manifests.swift @@ -599,9 +599,8 @@ extension Workspace { let rootManifests = try root.manifests.mapValues { manifest in let deps = try manifest.dependencies.filter { dep in guard configuration.pruneDependencies else { return true } -// let explicitlyEnabledTraits = try manifest.enabledTraits(using: configuration.traitConfiguration?.enabledTraits, enableAllTraits: configuration.traitConfiguration?.enableAllTraits ?? false) - let explicitlyEnabledTraits = try manifest.enabledTraits2(using: configuration.traitConfiguration) - let isDepUsed = try manifest.isPackageDependencyUsed(dep, enabledTraits: explicitlyEnabledTraits) + let enabledTraits = root.enabledTraits[manifest.packageIdentity] + let isDepUsed = try manifest.isPackageDependencyUsed(dep, enabledTraits: enabledTraits) return isDepUsed } @@ -637,40 +636,39 @@ extension Workspace { let firstLevelDependencies = try topLevelManifests.values.map { manifest in try manifest.dependencies.filter { dep in guard configuration.pruneDependencies else { return true } - var config = configuration.traitConfiguration + var enabledTraits: Set? = [] if manifest.packageKind.isRoot { -// let manifestEnabledTraits = try manifest.enabledTraits(using: configuration.traitConfiguration?.enabledTraits, enableAllTraits: configuration.traitConfiguration?.enableAllTraits ?? false) - let manifestEnabledTraits = try manifest.enabledTraits2(using: configuration.traitConfiguration) - config = .init(enabledTraits: manifestEnabledTraits) - } else { - let rootManifests = root.manifests.values.filter(\.packageKind.isRoot) - - // find the package dependency in each of the root manifests - let packageDependencyInRoots = rootManifests - .compactMap { - $0.dependencies - .first(where: { $0.identity.description == manifest.displayName.lowercased() }) - } - - // pluck out the enabled traits defined by the package dependency struct - let enabledTraitsPerPackageDep = packageDependencyInRoots.map(\.traits) - - // create a union of the sets; if all are nil, then there is no config - var manifestEnabledTraits: Set? - for enabledTraits in enabledTraitsPerPackageDep { - if let enabledTraits = enabledTraits?.map(\.name) { - if let resultSet = manifestEnabledTraits { - manifestEnabledTraits = resultSet.union(Set(enabledTraits)) - } else { - manifestEnabledTraits = Set(enabledTraits) - } - } - } - -// manifestEnabledTraits = try manifest.enabledTraits(using: manifestEnabledTraits) - config = .init(enabledTraits: manifestEnabledTraits) + enabledTraits = root.enabledTraits[manifest.packageIdentity] } - let isDepUsed = try manifest.isPackageDependencyUsed(dep, enabledTraits: config.enabledTraits) +// else { +// let rootManifests = root.manifests.values.filter(\.packageKind.isRoot) +// +// // find the package dependency in each of the root manifests +// let packageDependencyInRoots = rootManifests +// .compactMap { +// $0.dependencies +// .first(where: { $0.identity.description == manifest.displayName.lowercased() }) +// } +// +// // pluck out the enabled traits defined by the package dependency struct +// let enabledTraitsPerPackageDep = packageDependencyInRoots.map(\.traits) +// +// // create a union of the sets; if all are nil, then there is no config +// var manifestEnabledTraits: Set? +// for enabledTraits in enabledTraitsPerPackageDep { +// if let enabledTraits = enabledTraits?.map(\.name) { +// if let resultSet = manifestEnabledTraits { +// manifestEnabledTraits = resultSet.union(Set(enabledTraits)) +// } else { +// manifestEnabledTraits = Set(enabledTraits) +// } +// } +// } +//// manifestEnabledTraits = try manifest.enabledTraits(using: manifestEnabledTraits) +//// config = .init(enabledTraits: manifestEnabledTraits) +// +// } + let isDepUsed = try manifest.isPackageDependencyUsed(dep, enabledTraits: enabledTraits) return isDepUsed }.map(\.packageRef) }.flatMap(\.self) @@ -703,7 +701,7 @@ extension Workspace { ) dependenciesManifests.forEach { loadedManifests[$0.key] = $0.value } return try (dependenciesRequired + dependenciesGuarded).compactMap { dependency in - try loadedManifests[dependency.identity].flatMap { + try loadedManifests[dependency.identity].flatMap { manifest in // we also compare the location as this function may attempt to load // dependencies that have the same identity but from a different location // which is an error case we diagnose an report about in the GraphLoading part which @@ -715,20 +713,20 @@ extension Workspace { return !conditionTraits.intersection(node.item.enabledTraits).isEmpty }.map(\.name) - var allEnabledTraits: Set = [] - if let explicitlyEnabledTraits -// let calculatedTraits = try $0.enabledTraits(using: Set(explicitlyEnabledTraits)) - { - allEnabledTraits = Set(explicitlyEnabledTraits) - } + // TODO: bp must now calculate all transitively enabled traits of the deps + let calculatedTraits = try manifest.calculateAllEnabledTraits( +// parentPackage: node.item.identity, +// identity: dependency.identity, + explictlyEnabledTraits: explicitlyEnabledTraits.flatMap { Set($0) } + ) - return $0.canonicalPackageLocation == dependency.packageRef.canonicalLocation ? + return manifest.canonicalPackageLocation == dependency.packageRef.canonicalLocation ? try KeyedPair( GraphLoadingNode( identity: dependency.identity, - manifest: $0, + manifest: manifest, productFilter: dependency.productFilter, - enabledTraits: allEnabledTraits + enabledTraits: calculatedTraits ), key: dependency.identity ) : @@ -741,10 +739,8 @@ extension Workspace { do { let manifestGraphRoots = try topLevelManifests.map { identity, manifest in - // Since these represent the root manifests, can pass in the enabled traits from the trait configuration - // here, as that is what it represents. let isRoot = manifest.packageKind.isRoot - let enabledTraits = isRoot ? try manifest.enabledTraits2(using: configuration.traitConfiguration) : [] + let enabledTraits = isRoot ? root.enabledTraits[identity] : [] return try KeyedPair( GraphLoadingNode( identity: identity, diff --git a/Sources/_InternalTestSupport/MockWorkspace.swift b/Sources/_InternalTestSupport/MockWorkspace.swift index d3e25851cd2..5d47e940fe0 100644 --- a/Sources/_InternalTestSupport/MockWorkspace.swift +++ b/Sources/_InternalTestSupport/MockWorkspace.swift @@ -679,7 +679,7 @@ public final class MockWorkspace { packages: rootInput.packages, observabilityScope: observability.topScope ) - let root = PackageGraphRoot( + let root = try PackageGraphRoot( input: rootInput, manifests: rootManifests, observabilityScope: observability.topScope @@ -946,7 +946,7 @@ public final class MockWorkspace { packages: rootInput.packages, observabilityScope: observability.topScope ) - let graphRoot = PackageGraphRoot( + let graphRoot = try PackageGraphRoot( input: rootInput, manifests: rootManifests, observabilityScope: observability.topScope diff --git a/Sources/swift-bootstrap/main.swift b/Sources/swift-bootstrap/main.swift index aaac11435eb..e902ec0e666 100644 --- a/Sources/swift-bootstrap/main.swift +++ b/Sources/swift-bootstrap/main.swift @@ -404,7 +404,7 @@ struct SwiftBootstrapBuildTool: AsyncParsableCommand { } } - let packageGraphRoot = PackageGraphRoot( + let packageGraphRoot = try PackageGraphRoot( input: .init(packages: [packagePath]), manifests: [packagePath: rootPackageManifest], observabilityScope: observabilityScope diff --git a/Tests/FunctionalTests/TraitTests.swift b/Tests/FunctionalTests/TraitTests.swift index b9bef2aaac5..e2022c28b21 100644 --- a/Tests/FunctionalTests/TraitTests.swift +++ b/Tests/FunctionalTests/TraitTests.swift @@ -313,14 +313,17 @@ final class TraitTests: XCTestCase { } } - func testPackageDisablinDefaultsTrait_whenNoTraits() async throws { + func testPackageDisablingDefaultsTrait_whenNoTraits() async throws { try await fixture(name: "Traits") { fixturePath in do { - let _ = try await executeSwiftRun( + let (stdout, stderr) = try await executeSwiftRun( fixturePath.appending("DisablingEmptyDefaultsExample"), - "DisablingEmptyDefaultsExample" + "DisablingEmptyDefaultsExample", + extraArgs: ["--experimental-prune-unused-dependencies"] ) + print("noop") } catch let error as SwiftPMError { + // TODO: bp this isn't being hit switch error { case .packagePathNotFound: throw error diff --git a/Tests/PackageModelTests/ManifestTests.swift b/Tests/PackageModelTests/ManifestTests.swift index bbf732d9536..59534857502 100644 --- a/Tests/PackageModelTests/ManifestTests.swift +++ b/Tests/PackageModelTests/ManifestTests.swift @@ -358,7 +358,7 @@ class ManifestTests: XCTestCase { // Calculate the enabled traits without an explicitly declared set of enabled traits. // This should default to fetching the default traits, if they exist (which in this test case // they do), and then will calculate the transitive set of traits that are enabled. - let allEnabledTraits = try manifest.enabledTraits2(using: .default)?.sorted() + let allEnabledTraits = try XCTUnwrap(manifest.enabledTraits2(using: .default)).sorted() XCTAssertEqual(allEnabledTraits, ["Trait1", "Trait2"]) } } @@ -782,7 +782,7 @@ class ManifestTests: XCTestCase { let calculatedDependenciesWithAllTraitsEnabled = try manifest.dependenciesRequired( for: .everything, - [] + ["Trait1", "Trait2"] ) XCTAssertEqual( calculatedDependenciesWithAllTraitsEnabled.map(\.identity).sorted(), From fe96c8efdb3c91d4355fb1ffd0945c86b6630bbd Mon Sep 17 00:00:00 2001 From: Bri Peticca Date: Wed, 19 Mar 2025 13:27:45 -0400 Subject: [PATCH 4/8] Update trait test assertion + change case names for TraitConfiguration - Updated the test that asserts for an error thrown when disabling default traits for a package with no traits; prior, there was a chance to encounter a false positive if there was no error being thrown due to the do/catch block. Now, we assert for an error being thrown and check that it's the expected error message. - Changed the case names for TraitConfiguration: - noConfiguration -> none - noEnabledTraits -> disableAllTraits - Added optional parameter to calculateAllEnabledTraits entitled 'parentPackage' that keeps a reference to the parent package (if any) when computing the enabled traits of a package; this way, we can meaningfully report an error if a parent package sets an unallowable set of traits to be enabled/disabled for a dependency. --- .../PackageGraph/ModulesGraph+Loading.swift | 1 - Sources/PackageGraph/PackageGraphRoot.swift | 7 ++- .../Resolution/DependencyResolutionNode.swift | 4 +- Sources/PackageGraph/TraitConfiguration.swift | 18 +++---- Sources/PackageModel/Manifest/Manifest.swift | 47 ++++++++++++------- Sources/Workspace/Workspace+Manifests.swift | 3 +- Tests/FunctionalTests/TraitTests.swift | 33 ++++++------- 7 files changed, 60 insertions(+), 53 deletions(-) diff --git a/Sources/PackageGraph/ModulesGraph+Loading.swift b/Sources/PackageGraph/ModulesGraph+Loading.swift index 939246126ef..33eb5aa1c26 100644 --- a/Sources/PackageGraph/ModulesGraph+Loading.swift +++ b/Sources/PackageGraph/ModulesGraph+Loading.swift @@ -966,7 +966,6 @@ private func calculateEnabledTraits( } } - // explicitlyEnabledTraits != nil && !areDefaultsEnabled if let parentPackage, !(explictlyEnabledTraits == nil || areDefaultsEnabled) && !manifest.supportsTraits { // We throw an error when default traits are disabled for a package without any traits // This allows packages to initially move new API behind traits once. diff --git a/Sources/PackageGraph/PackageGraphRoot.swift b/Sources/PackageGraph/PackageGraphRoot.swift index 8e5c1876c3f..8f5a0f18a05 100644 --- a/Sources/PackageGraph/PackageGraphRoot.swift +++ b/Sources/PackageGraph/PackageGraphRoot.swift @@ -245,9 +245,8 @@ extension Manifest { // not exist for this manifest) then we must throw an error. if !traitConfiguration.enablesDefaultTraits && traitConfiguration.enablesNonDefaultTraits { - // TODO: bp add parent package to error message like in modulesgraph+loading.swift - // TODO: bp fix explicitlyEnabledTraits value passed here throw TraitError.traitsNotSupported( + parentPackage: nil, package: displayName, explicitlyEnabledTraits: traits.map(\.name) ) @@ -261,11 +260,11 @@ extension Manifest { switch traitConfiguration { case .enableAllTraits: enabledTraits = Set(traits.map(\.name)) - case .noConfiguration: + case .none: if let defaultTraits = defaultTraits?.map(\.name) { enabledTraits = Set(defaultTraits) } - case .noEnabledTraits: + case .disableAllTraits: return [] case .enabledTraits(let explicitlyEnabledTraits): enabledTraits = explicitlyEnabledTraits diff --git a/Sources/PackageGraph/Resolution/DependencyResolutionNode.swift b/Sources/PackageGraph/Resolution/DependencyResolutionNode.swift index c9e9b53d35f..6376c1adc66 100644 --- a/Sources/PackageGraph/Resolution/DependencyResolutionNode.swift +++ b/Sources/PackageGraph/Resolution/DependencyResolutionNode.swift @@ -97,9 +97,9 @@ public enum DependencyResolutionNode { switch config { case .enabledTraits(let traits): return traits - case .noEnabledTraits: + case .disableAllTraits: return [] - case .noConfiguration, .enableAllTraits: // TODO: bp fix + case .none, .enableAllTraits: // TODO: bp fix return nil } case .product(_, _, let enabledTraits): diff --git a/Sources/PackageGraph/TraitConfiguration.swift b/Sources/PackageGraph/TraitConfiguration.swift index c03d59c1cec..39ab635d7d2 100644 --- a/Sources/PackageGraph/TraitConfiguration.swift +++ b/Sources/PackageGraph/TraitConfiguration.swift @@ -13,9 +13,9 @@ /// The trait configuration. public enum TraitConfiguration: Codable, Hashable { case enableAllTraits - case noConfiguration - case noEnabledTraits + case disableAllTraits case enabledTraits(Set) + case none public init( enabledTraits: Set? = nil, @@ -29,14 +29,14 @@ public enum TraitConfiguration: Codable, Hashable { if let enabledTraits { if enabledTraits.isEmpty { - self = .noEnabledTraits + self = .disableAllTraits } else { self = .enabledTraits(enabledTraits) } } else { // Since enableAllTraits isn't enabled and there isn't a set of traits set, // there is no configuration passed by the user. - self = .noConfiguration + self = .none } } @@ -49,9 +49,9 @@ public enum TraitConfiguration: Codable, Hashable { switch self { case .enabledTraits(let traits): return traits - case .noConfiguration, .enableAllTraits: + case .none, .enableAllTraits: return nil - case .noEnabledTraits: + case .disableAllTraits: return [] } } @@ -68,9 +68,9 @@ public enum TraitConfiguration: Codable, Hashable { switch self { case .enabledTraits(let traits): return traits.contains("default") - case .noConfiguration, .enableAllTraits: + case .none, .enableAllTraits: return true - case .noEnabledTraits: + case .disableAllTraits: return false } } @@ -82,7 +82,7 @@ public enum TraitConfiguration: Codable, Hashable { return !traitsWithoutDefault.isEmpty case .enableAllTraits: return true - case .noConfiguration, .noEnabledTraits: + case .none, .disableAllTraits: return false } } diff --git a/Sources/PackageModel/Manifest/Manifest.swift b/Sources/PackageModel/Manifest/Manifest.swift index e2d926a0b4c..36b0db0020f 100644 --- a/Sources/PackageModel/Manifest/Manifest.swift +++ b/Sources/PackageModel/Manifest/Manifest.swift @@ -206,7 +206,6 @@ public final class Manifest: Sendable { } let traitGuardedDeps = self.traitGuardedDependencies(lowercasedKeys: true) -// let explicitlyEnabledTraits = try? self.enabledTraits(using: enabledTraits, enableAllTraits: enableAllTraits) guard self.toolsVersion >= .v5_2 && !self.packageKind.isRoot else { let deps = self.dependencies.filter({ if let guardTraits = traitGuardedDeps[$0.identity.description]?.values.flatMap({ $0 }), @@ -762,7 +761,10 @@ extension Manifest { } /// Calculates and returns a set of all enabled traits, beginning with a set of explicitly enabled traits (which can either be the default traits of a manifest, or a configuration of enabled traits determined from a user-generated trait configuration) and determines which traits are transitively enabled. - public func calculateAllEnabledTraits(explictlyEnabledTraits: Set?) throws -> Set { + public func calculateAllEnabledTraits( + explictlyEnabledTraits: Set?, + _ parentPackage: String? = nil + ) throws -> Set { // This the point where we flatten the enabled traits and resolve the recursive traits var enabledTraits = explictlyEnabledTraits ?? [] let areDefaultsEnabled = enabledTraits.remove("default") != nil @@ -777,11 +779,8 @@ extension Manifest { if !(explictlyEnabledTraits == nil || areDefaultsEnabled) && !self.supportsTraits { // We throw an error when default traits are disabled for a package without any traits // This allows packages to initially move new API behind traits once. -// throw ModuleError.disablingDefaultTraitsOnEmptyTraits( -// parentPackage: parentPackage, -// packageName: manifest.displayName -// ) throw TraitError.traitsNotSupported( + parentPackage: parentPackage, package: displayName, explicitlyEnabledTraits: explictlyEnabledTraits?.map({ $0 }) ?? [] ) @@ -840,10 +839,7 @@ extension Manifest { } /// Computes the set of package dependencies that are used by targets of this manifest. - public func usedDependencies( - withTraits enabledTraits: Set?, - enableAllTraits: Bool = false - ) throws -> (knownPackage: Set, unknownPackage: Set) { + public func usedDependencies(withTraits enabledTraits: Set?) throws -> (knownPackage: Set, unknownPackage: Set) { let deps = try self.usedTargetDependencies(withTraits: enabledTraits) .values .flatMap { $0 } @@ -936,6 +932,7 @@ public indirect enum TraitError: Swift.Error { /// Indicates that the manifest does not support traits, yet a method was called with a configuration of enabled /// traits. case traitsNotSupported( + parentPackage: String?, package: String, explicitlyEnabledTraits: [String] ) @@ -955,15 +952,29 @@ extension TraitError: CustomStringConvertible { " The available traits defined for this package are: \(availableTraits.joined(separator: ", "))." } return errorMsg - case .traitsNotSupported(let package, let explicitlyEnabledTraits): - return """ - Package \( - package - ) does not have any available traits defined, yet an explicit configuration of enabled traits were provided: \( - explicitlyEnabledTraits - .joined(separator: ", ") - ). + case .traitsNotSupported(let parentPackage, let package, let explicitlyEnabledTraits): + if explicitlyEnabledTraits.isEmpty { + if let parentPackage { + return """ + Disabled default traits by package '\(parentPackage)' on package '\(package)' that declares no traits. This is prohibited to allow packages to adopt traits initially without causing an API break. + """ + } else { + return """ + Disabled default traits on package '\(package)' that declares no traits. This is prohibited to allow packages to adopt traits initially without causing an API break. """ + } + } else { + if let parentPackage { + return """ + Package \(parentPackage) enables traits [\(explicitlyEnabledTraits.joined(separator: ", "))] on package '\(package)' that declares no traits. + """ + } else { + // TODO: bp fix msg + return """ + Enabled traits [\(explicitlyEnabledTraits.joined(separator: ", "))] on package '\(package)' that declares no traits. + """ + } + } } } } diff --git a/Sources/Workspace/Workspace+Manifests.swift b/Sources/Workspace/Workspace+Manifests.swift index 2d56b7f3643..b954e0ff8ba 100644 --- a/Sources/Workspace/Workspace+Manifests.swift +++ b/Sources/Workspace/Workspace+Manifests.swift @@ -717,7 +717,8 @@ extension Workspace { let calculatedTraits = try manifest.calculateAllEnabledTraits( // parentPackage: node.item.identity, // identity: dependency.identity, - explictlyEnabledTraits: explicitlyEnabledTraits.flatMap { Set($0) } + explictlyEnabledTraits: explicitlyEnabledTraits.flatMap { Set($0) }, + node.item.identity.description ) return manifest.canonicalPackageLocation == dependency.packageRef.canonicalLocation ? diff --git a/Tests/FunctionalTests/TraitTests.swift b/Tests/FunctionalTests/TraitTests.swift index e2022c28b21..9c745484358 100644 --- a/Tests/FunctionalTests/TraitTests.swift +++ b/Tests/FunctionalTests/TraitTests.swift @@ -315,25 +315,22 @@ final class TraitTests: XCTestCase { func testPackageDisablingDefaultsTrait_whenNoTraits() async throws { try await fixture(name: "Traits") { fixturePath in - do { - let (stdout, stderr) = try await executeSwiftRun( - fixturePath.appending("DisablingEmptyDefaultsExample"), - "DisablingEmptyDefaultsExample", - extraArgs: ["--experimental-prune-unused-dependencies"] - ) - print("noop") - } catch let error as SwiftPMError { - // TODO: bp this isn't being hit - switch error { - case .packagePathNotFound: - throw error - case .executionFailure(_, _, let stderr): - let expectedErr = """ - error: Disabled default traits by package 'disablingemptydefaultsexample' on package 'Package11' that declares no traits. This is prohibited to allow packages to adopt traits initially without causing an API break. - - """ - XCTAssertTrue(stderr.contains(expectedErr)) + await XCTAssertAsyncThrowsError(try await executeSwiftRun( + fixturePath.appending("DisablingEmptyDefaultsExample"), + "DisablingEmptyDefaultsExample" + //extraArgs: ["--experimental-prune-unused-dependencies"] + )) { error in + guard case SwiftPMError.executionFailure(let underlying, let stdout, let stderr) = error else { + XCTFail() + return } + + let expectedErr = """ + error: Disabled default traits by package 'disablingemptydefaultsexample' on package 'Package11' that declares no traits. This is prohibited to allow packages to adopt traits initially without causing an API break. + + """ + XCTAssertTrue(stderr.contains(expectedErr)) + } } } From 9a64694009144926ec1775a48c6f912e3af6edfb Mon Sep 17 00:00:00 2001 From: Bri Peticca Date: Wed, 19 Mar 2025 16:02:21 -0400 Subject: [PATCH 5/8] Create separate file for Manifest trait helpers - Move the trait helper and validation methods to a separate file (Manifest/Manifest+Traits.swift) and move the TraitConfiguration into the PackageModel instead of PackageGraph; updated the appropriate CMakeLists.txt - Update TraitConfiguration computed variable for enabled traits; '.none' case should return '["default"]', as that is what its behaviour defaults to. Removed other computed variables in favor of checking the enum cases themselves where appropriate. --- Sources/Build/LLBuildDescription.swift | 2 + .../Commands/PackageCommands/Resolve.swift | 2 +- Sources/CoreCommands/BuildSystemSupport.swift | 1 + Sources/CoreCommands/Options.swift | 2 +- Sources/PackageGraph/CMakeLists.txt | 1 - Sources/PackageGraph/PackageGraphRoot.swift | 46 +- Sources/PackageModel/CMakeLists.txt | 2 + .../Manifest/Manifest+Traits.swift | 423 ++++++++++++++++++ Sources/PackageModel/Manifest/Manifest.swift | 307 +------------ .../Manifest}/TraitConfiguration.swift | 62 +-- .../BuildSystem/BuildSystem.swift | 2 + .../SourceKitLSPAPI/BuildDescription.swift | 2 +- .../PluginTargetBuildDescription.swift | 2 +- .../FileSystemPackageContainer.swift | 2 +- .../RegistryPackageContainer.swift | 2 +- .../SourceControlPackageContainer.swift | 2 +- .../ResolverPrecomputationProvider.swift | 6 +- .../Workspace/Workspace+Dependencies.swift | 2 +- Sources/Workspace/Workspace+Manifests.swift | 2 +- Tests/PackageModelTests/ManifestTests.swift | 8 +- 20 files changed, 458 insertions(+), 420 deletions(-) create mode 100644 Sources/PackageModel/Manifest/Manifest+Traits.swift rename Sources/{PackageGraph => PackageModel/Manifest}/TraitConfiguration.swift (55%) diff --git a/Sources/Build/LLBuildDescription.swift b/Sources/Build/LLBuildDescription.swift index 81aeec613dd..c39fb643568 100644 --- a/Sources/Build/LLBuildDescription.swift +++ b/Sources/Build/LLBuildDescription.swift @@ -16,6 +16,8 @@ import LLBuildManifest import SPMBuildCore import PackageGraph +import enum PackageModel.TraitConfiguration + import struct TSCBasic.ByteString /// Contains the description of the build that is needed during the execution. diff --git a/Sources/Commands/PackageCommands/Resolve.swift b/Sources/Commands/PackageCommands/Resolve.swift index 07a639014c6..d88176213db 100644 --- a/Sources/Commands/PackageCommands/Resolve.swift +++ b/Sources/Commands/PackageCommands/Resolve.swift @@ -14,7 +14,7 @@ import ArgumentParser import CoreCommands import TSCUtility -import enum PackageGraph.TraitConfiguration +import enum PackageModel.TraitConfiguration extension SwiftPackageCommand { struct ResolveOptions: ParsableArguments { diff --git a/Sources/CoreCommands/BuildSystemSupport.swift b/Sources/CoreCommands/BuildSystemSupport.swift index 30a00599b9f..3a3d1cfc8a8 100644 --- a/Sources/CoreCommands/BuildSystemSupport.swift +++ b/Sources/CoreCommands/BuildSystemSupport.swift @@ -21,6 +21,7 @@ import class Basics.ObservabilityScope import struct PackageGraph.ModulesGraph import struct PackageLoading.FileRuleDescription import protocol TSCBasic.OutputByteStream +import enum PackageModel.TraitConfiguration private struct NativeBuildSystemFactory: BuildSystemFactory { let swiftCommandState: SwiftCommandState diff --git a/Sources/CoreCommands/Options.swift b/Sources/CoreCommands/Options.swift index 86afd00fd9f..0dee3045344 100644 --- a/Sources/CoreCommands/Options.swift +++ b/Sources/CoreCommands/Options.swift @@ -27,7 +27,7 @@ import class PackageModel.Manifest import enum PackageModel.Sanitizer @_spi(SwiftPMInternal) import struct PackageModel.SwiftSDK -import enum PackageGraph.TraitConfiguration +import enum PackageModel.TraitConfiguration import struct SPMBuildCore.BuildParameters import struct SPMBuildCore.BuildSystemProvider diff --git a/Sources/PackageGraph/CMakeLists.txt b/Sources/PackageGraph/CMakeLists.txt index c6f6b7fba40..46bd6580f5c 100644 --- a/Sources/PackageGraph/CMakeLists.txt +++ b/Sources/PackageGraph/CMakeLists.txt @@ -19,7 +19,6 @@ add_library(PackageGraph PackageModel+Extensions.swift PackageRequirement.swift ResolvedPackagesStore.swift - TraitConfiguration.swift Resolution/PubGrub/Assignment.swift Resolution/PubGrub/ContainerProvider.swift Resolution/PubGrub/DiagnosticReportBuilder.swift diff --git a/Sources/PackageGraph/PackageGraphRoot.swift b/Sources/PackageGraph/PackageGraphRoot.swift index 8f5a0f18a05..efccb13dca6 100644 --- a/Sources/PackageGraph/PackageGraphRoot.swift +++ b/Sources/PackageGraph/PackageGraphRoot.swift @@ -111,7 +111,7 @@ public struct PackageGraphRoot { let traitConfiguration = input.traitConfiguration // Should only ever have to use trait configuration here for roots. - let enabledTraits = try manifest.enabledTraits2(using: traitConfiguration) + let enabledTraits = try manifest.enabledTraits(using: traitConfiguration) traitsMap[package.key] = enabledTraits } @@ -233,47 +233,3 @@ extension PackageDependency.Registry.Requirement { } } -// TODO: bp to move to Manifest+Traits.swift file -extension Manifest { - /// Calculates the set of all transitive traits that are enabled for this manifest using the passed set of - /// explicitly enabled traits and a flag that - /// determines whether all traits are enabled. - public func enabledTraits2(using traitConfiguration: TraitConfiguration) throws -> Set? { - guard supportsTraits else { - // If this manifest does not support traits, but the passed configuration either - // disables default traits or enables non-default traits (i.e. traits that would - // not exist for this manifest) then we must throw an error. - - if !traitConfiguration.enablesDefaultTraits && traitConfiguration.enablesNonDefaultTraits { - throw TraitError.traitsNotSupported( - parentPackage: nil, - package: displayName, - explicitlyEnabledTraits: traits.map(\.name) - ) - } - - return nil - } - - var enabledTraits: Set = [] - - switch traitConfiguration { - case .enableAllTraits: - enabledTraits = Set(traits.map(\.name)) - case .none: - if let defaultTraits = defaultTraits?.map(\.name) { - enabledTraits = Set(defaultTraits) - } - case .disableAllTraits: - return [] - case .enabledTraits(let explicitlyEnabledTraits): - enabledTraits = explicitlyEnabledTraits - } - - if let allEnabledTraits = try? calculateAllEnabledTraits(explictlyEnabledTraits: enabledTraits) { - enabledTraits = allEnabledTraits - } - - return enabledTraits - } -} diff --git a/Sources/PackageModel/CMakeLists.txt b/Sources/PackageModel/CMakeLists.txt index aa978e3f181..8e42f906f12 100644 --- a/Sources/PackageModel/CMakeLists.txt +++ b/Sources/PackageModel/CMakeLists.txt @@ -17,6 +17,8 @@ add_library(PackageModel IdentityResolver.swift InstalledSwiftPMConfiguration.swift Manifest/Manifest.swift + Manifest/Manifest+Traits.swift + Manifest/TraitConfiguration.swift Manifest/PackageConditionDescription.swift Manifest/PackageDependencyDescription.swift Manifest/PlatformDescription.swift diff --git a/Sources/PackageModel/Manifest/Manifest+Traits.swift b/Sources/PackageModel/Manifest/Manifest+Traits.swift new file mode 100644 index 00000000000..2f0b7918ed6 --- /dev/null +++ b/Sources/PackageModel/Manifest/Manifest+Traits.swift @@ -0,0 +1,423 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the Swift.org open source project +// +// Copyright (c) 2025 Apple Inc. and the Swift project authors +// Licensed under Apache License v2.0 with Runtime Library Exception +// +// See https://swift.org/LICENSE.txt for license information +// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors +// +//===----------------------------------------------------------------------===// + +import Basics +import Foundation + +// MARK: - Traits Validation + +/// Validator methods that check the correctness of traits and their support as defined in the manifest. +extension Manifest { + /// Determines whether traits are supported for this Manifest. + public var supportsTraits: Bool { + !self.traits.isEmpty + } + + /// Validates a trait by checking that it is defined in the manifest; if not, an error is thrown. + private func validateTrait(_ trait: TraitDescription) throws { + try self.validateTrait(trait.name) + } + + /// Validates a trait by checking that it is defined in the manifest; if not, an error is thrown. + private func validateTrait(_ trait: String) throws { + // Check if the enabled trait is a valid trait + if self.traits.first(where: { $0.name == trait }) == nil { + throw TraitError.invalidTrait(package: self.displayName, trait: trait) + } + } + + /// Validates a set of traits that is intended to be enabled for the manifest; if there are any discrepencies in the + /// set of enabled traits and whether the manifest defines these traits (or if it defines any traits at all), then an + /// error indicating the issue will be thrown. + public func validateEnabledTraits( + _ explicitlyEnabledTraits: Set?, + _ parentPackage: String? = nil + ) throws { + guard supportsTraits else { + if let explicitlyEnabledTraits { + let enabledTraitsWithoutDefault = explicitlyEnabledTraits.subtracting(["default"]) + if !enabledTraitsWithoutDefault.isEmpty { + throw TraitError.traitsNotSupported( + parentPackage: parentPackage, + package: self.displayName, + explicitlyEnabledTraits: explicitlyEnabledTraits.map({ $0 }) + ) + } + } + + return + } + + let enabledTraits = explicitlyEnabledTraits ?? [] + + // Validate each trait to assure it's defined in the current package. + for trait in enabledTraits { + try validateTrait(trait) + } + + let areDefaultsEnabled = enabledTraits.contains("default") + + // Ensure that disabling default traits is disallowed for packages that don't define any traits. + if !(explicitlyEnabledTraits == nil || areDefaultsEnabled) && !self.supportsTraits { + // We throw an error when default traits are disabled for a package without any traits + // This allows packages to initially move new API behind traits once. + throw TraitError.traitsNotSupported( + parentPackage: parentPackage, + package: displayName, + explicitlyEnabledTraits: enabledTraits.map({ $0 }) + ) + } + } + + private func validateTraitConfiguration(_ traitConfiguration: TraitConfiguration) throws { + guard supportsTraits else { + switch traitConfiguration { + case .disableAllTraits: + throw TraitError.traitsNotSupported( + parentPackage: nil, + package: displayName, + explicitlyEnabledTraits: [] + ) + case .enabledTraits(let traits): + throw TraitError.traitsNotSupported( + parentPackage: nil, + package: displayName, + explicitlyEnabledTraits: traits.map({ $0 }) + ) + case .enableAllTraits, .none: + return + } + } + + var enabledTraits = traitConfiguration.enabledTraits + + try validateEnabledTraits(enabledTraits) + } +} + + +// MARK: - Traits + +/// Helper methods to calculate states of the manifest and its dependencies when given a set of enabled traits. +extension Manifest { + /// The default traits as defined in this package as the root. + public var defaultTraits: Set? { + // First, guard against whether this package actually has traits. + guard self.supportsTraits else { return nil } + return self.traits.filter(\.isDefault) + } + + /// A map of trait names to the trait description. + public var traitsMap: [String: TraitDescription] { + self.traits.reduce(into: [String: TraitDescription]()) { traitsMap, trait in + traitsMap[trait.name] = trait + } + } + + /// Calculates the set of all transitive traits that are enabled for this manifest using the passed set of + /// explicitly enabled traits and a flag that + /// determines whether all traits are enabled. + public func enabledTraits(using traitConfiguration: TraitConfiguration) throws -> Set? { + guard supportsTraits else { + // If this manifest does not support traits, but the passed configuration either + // disables default traits or enables non-default traits (i.e. traits that would + // not exist for this manifest) then we must throw an error. + + // TODO: bp redundant guards? may be able to change this to a bool + try validateTraitConfiguration(traitConfiguration) + + return nil + } + + var enabledTraits: Set = [] + + switch traitConfiguration { + case .enableAllTraits: + enabledTraits = Set(traits.map(\.name)) + case .none: + if let defaultTraits = defaultTraits?.map(\.name) { + enabledTraits = Set(defaultTraits) + } + case .disableAllTraits: + return [] + case .enabledTraits(let explicitlyEnabledTraits): + enabledTraits = explicitlyEnabledTraits + } + + if let allEnabledTraits = try? calculateAllEnabledTraits(explictlyEnabledTraits: enabledTraits) { + enabledTraits = allEnabledTraits + } + + return enabledTraits + } + + + /// Given a trait, determine if the trait is enabled given the current set of enabled traits. + public func isTraitEnabled(_ trait: TraitDescription, _ enabledTraits: Set?) throws -> Bool { + guard supportsTraits else { + if var enabledTraits { + enabledTraits.remove("default") + if !enabledTraits.isEmpty { + throw TraitError.invalidTrait( + package: self.displayName, + trait: trait.name, + availableTraits: self.traits.map(\.name) + ) + } + } + + return false + } + + // TODO: bp double check that this guard/check is doing what it's meant to do... + // i.e. what if the set of enabled traits does not specify default? + guard !trait.isDefault else { + if self.traits.contains(where: \.isDefault) { + // TODO: bp if there is either no config (nil enabledTraits) or the enabledTraits contains "default", then we can just return true. else, return false. + if let enabledTraits, enabledTraits.contains(trait.name) { + return true + } else if enabledTraits == nil { + return true + } else { + return false + } + } + throw TraitError.invalidTrait( + package: self.displayName, + trait: trait.name, + availableTraits: self.traits.map(\.name) + ) + } + + let allEnabledTraits = try calculateAllEnabledTraits(explictlyEnabledTraits: enabledTraits) + + return allEnabledTraits.contains(trait.name) + } + + /// Calculates and returns a set of all enabled traits, beginning with a set of explicitly enabled traits (which can either be the default traits of a manifest, or a configuration of enabled traits determined from a user-generated trait configuration) and determines which traits are transitively enabled. + public func calculateAllEnabledTraits( + explictlyEnabledTraits: Set?, + _ parentPackage: String? = nil + ) throws -> Set { + // This the point where we flatten the enabled traits and resolve the recursive traits + var enabledTraits = explictlyEnabledTraits ?? [] + let areDefaultsEnabled = enabledTraits.remove("default") != nil + + for trait in enabledTraits { + // Check if the enabled trait is a valid trait + if self.traits.first(where: { $0.name == trait }) == nil { + throw TraitError.invalidTrait(package: self.displayName, trait: trait) + } + } + + if !(explictlyEnabledTraits == nil || areDefaultsEnabled) && !self.supportsTraits { + // We throw an error when default traits are disabled for a package without any traits + // This allows packages to initially move new API behind traits once. + throw TraitError.traitsNotSupported( + parentPackage: parentPackage, + package: displayName, + explicitlyEnabledTraits: explictlyEnabledTraits?.map({ $0 }) ?? [] + ) + } + + // We have to enable all default traits if no traits are enabled or the defaults are explicitly enabled + if explictlyEnabledTraits == nil || areDefaultsEnabled { + if let defaultTraits { + enabledTraits.formUnion(defaultTraits.flatMap(\.enabledTraits)) + } + } + + // Iteratively flatten transitively enabled traits; stop when all transitive traits have been found. + while true { + let transitivelyEnabledTraits = try Set( + // We are going to calculate which traits are actually enabled for a node here. To do this + // we have to check if default traits should be used and then flatten all the enabled traits. + enabledTraits + .flatMap { trait in + guard let traitDescription = traitsMap[trait] else { + throw TraitError.invalidTrait(package: self.displayName, trait: trait) + } + return traitDescription.enabledTraits + } + ) + + let appendedList = enabledTraits.union(transitivelyEnabledTraits) + if appendedList.count == enabledTraits.count { + break + } else { + enabledTraits = appendedList + } + } + + return enabledTraits + } + + /// Computes the dependencies that are in use per target in this manifest. + public func usedTargetDependencies(withTraits enabledTraits: Set?) throws -> [String: Set] { + try self.targets.reduce(into: [String: Set]()) { depMap, target in + let nonTraitDeps = target.dependencies.filter { + $0.condition?.traits?.isEmpty ?? true + } + + let traitGuardedDeps = try target.dependencies.filter { dep in + let traits = dep.condition?.traits ?? [] + + // For each trait that is a condition on this target dependency, assure that + // each one is enabled in the manifest. + return try traits.allSatisfy({ try isTraitEnabled(.init(stringLiteral: $0), enabledTraits) }) + } + + let deps = nonTraitDeps + traitGuardedDeps + depMap[target.name] = Set(deps) + } + } + + /// Computes the set of package dependencies that are used by targets of this manifest. + public func usedDependencies(withTraits enabledTraits: Set?) throws -> (knownPackage: Set, unknownPackage: Set) { + let deps = try self.usedTargetDependencies(withTraits: enabledTraits) + .values + .flatMap { $0 } + .compactMap(\.package) + + var known: Set = [] + var unknown: Set = [] + + for item in deps { + if let dep = self.packageDependency(referencedBy: item) { + known.insert(dep.identity.description) + } else if self.targetMap[item] == nil { + // Marking this dependency as tentatively used, given that we cannot find the package ref at this stage. + unknown.insert(item) + } + } + + return (knownPackage: known, unknownPackage: unknown) + } + + /// Returns the set of package dependencies that are potentially guarded by traits, per target. This does not + /// calculate enabled and disabled dependencies enforced by traits. + public func traitGuardedDependencies(lowercasedKeys: Bool = false) -> [String: [String: Set]] { + self.targets.reduce(into: [String: [String: Set]]()) { depMap, target in + let traitGuardedTargetDependencies = target.dependencies.filter { + !($0.condition?.traits?.isEmpty ?? true) + } + traitGuardedTargetDependencies.forEach { + guard let package = lowercasedKeys ? $0.package?.lowercased() : $0.package else { return } + depMap[package, default: [:]][target.name, default: []].formUnion($0.condition?.traits ?? []) + } + } + } + + /// Computes the enabled traits for a given target dependency + public func enabledTraits(forDependency dependency: TargetDescription.Dependency) -> Set? { + guard let package = self.packageDependency(referencedBy: dependency), + let traits = package.traits?.compactMap(\.name) + else { + return nil + } + + return Set(traits) + } + + /// Determines whether a target dependency is enabled given a set of enabled traits for this manifest. + public func isTargetDependencyEnabled( + target: String, + _ dependency: TargetDescription.Dependency, + enabledTraits: Set?, + enableAllTraits: Bool = false + ) throws -> Bool { + guard self.supportsTraits, !enableAllTraits else { return true } + guard let package = dependency.package, let target = self.targetMap[target] else { return false } + guard target.dependencies.contains(where: { $0 == dependency }) else { + throw InternalError( + "target dependency \(dependency.name) not found for target \(target.name) in package \(self.displayName)" + ) + } + let traitsThatEnableDependency = self.traitGuardedDependencies()[package]?[target.name] ?? [] + + let isEnabled = try traitsThatEnableDependency.allSatisfy({ try isTraitEnabled(.init(stringLiteral: $0), enabledTraits) }) + + return traitsThatEnableDependency.isEmpty || isEnabled + } + + /// Determines whether a given package dependency is used by this manifest given a set of enabled traits. + public func isPackageDependencyUsed(_ dependency: PackageDependency, enabledTraits: Set?) throws -> Bool { + let usedDependencies = try self.usedDependencies(withTraits: enabledTraits) + let foundKnownPackage = usedDependencies.knownPackage.contains(where: { + $0.caseInsensitiveCompare(dependency.identity.description) == .orderedSame + }) + + // if there is a target dependency referenced by name and the package it originates from is unknown, default to + // tentatively marking the package dependency as used. to be resolved later on. + return foundKnownPackage || (!foundKnownPackage && !usedDependencies.unknownPackage.isEmpty) + } +} + +// MARK: - Trait Error + +public enum TraitError: Swift.Error { + /// Indicates that an invalid trait was enabled. + case invalidTrait( + package: String, + trait: String, + availableTraits: [String] = [] + ) + + /// Indicates that the manifest does not support traits, yet a method was called with a configuration of enabled + /// traits. + case traitsNotSupported( + parentPackage: String?, + package: String, + explicitlyEnabledTraits: [String] + ) +} + +extension TraitError: CustomStringConvertible { + public var description: String { + switch self { + case .invalidTrait(let package, let trait, let availableTraits): + var errorMsg = """ + Trait '"\(trait)"' is not declared by package '\(package)'. + """ + if availableTraits.isEmpty { + errorMsg += " There are no available traits defined by this package." + } else { + errorMsg += + " The available traits defined for this package are: \(availableTraits.joined(separator: ", "))." + } + return errorMsg + case .traitsNotSupported(let parentPackage, let package, let explicitlyEnabledTraits): + if explicitlyEnabledTraits.isEmpty { + if let parentPackage { + return """ + Disabled default traits by package '\(parentPackage)' on package '\(package)' that declares no traits. This is prohibited to allow packages to adopt traits initially without causing an API break. + """ + } else { + return """ + Disabled default traits on package '\(package)' that declares no traits. This is prohibited to allow packages to adopt traits initially without causing an API break. + """ + } + } else { + if let parentPackage { + return """ + Package \(parentPackage) enables traits [\(explicitlyEnabledTraits.joined(separator: ", "))] on package '\(package)' that declares no traits. + """ + } else { + // TODO: bp fix msg + return """ + Enabled traits [\(explicitlyEnabledTraits.joined(separator: ", "))] on package '\(package)' that declares no traits. + """ + } + } + } + } +} diff --git a/Sources/PackageModel/Manifest/Manifest.swift b/Sources/PackageModel/Manifest/Manifest.swift index 36b0db0020f..d83462db181 100644 --- a/Sources/PackageModel/Manifest/Manifest.swift +++ b/Sources/PackageModel/Manifest/Manifest.swift @@ -459,7 +459,7 @@ public final class Manifest: Sendable { } } - private func packageDependency( + internal func packageDependency( referencedBy packageName: String ) -> PackageDependency? { self.dependencies.first(where: { @@ -673,308 +673,3 @@ extension Manifest: Encodable { try container.encode(self.packageKind, forKey: .packageKind) } } - -// MARK: - Traits - -/// Helper methods that enable data collection through traits configurations in manifests. -extension Manifest { - /// Determines whether traits are supported for this Manifest. - public var supportsTraits: Bool { - !self.traits.isEmpty - } - - /// The default traits as defined in this package as the root. - public var defaultTraits: Set? { - // First, guard against whether this package actually has traits. - guard self.supportsTraits else { return nil } - return self.traits.filter(\.isDefault) - } - - /// A map of trait names to the trait description. - public var traitsMap: [String: TraitDescription] { - self.traits.reduce(into: [String: TraitDescription]()) { traitsMap, trait in - traitsMap[trait.name] = trait - } - } - - /// Calculates the set of all transitive traits that are enabled for this manifest using the passed set of - /// explicitly enabled traits and a flag that - /// determines whether all traits are enabled. -// public func enabledTraits(using explicitTraits: Set?, enableAllTraits: Bool = false) throws -> Set? { -// guard supportsTraits else { -// if var explicitTraits { -// explicitTraits.remove("default") -// if !explicitTraits.isEmpty { -// throw TraitError.traitsNotSupported( -// package: displayName, -// explicitlyEnabledTraits: traits.map(\.name) -// ) -// } -// } -// -// return nil -// } -// -// var enabledTraits = explicitTraits -// -// if enableAllTraits { -// enabledTraits = (enabledTraits ?? []).union(Set(traits.map(\.name))) -// } -// -// if let allEnabledTraits = try? calculateAllEnabledTraits(explictlyEnabledTraits: enabledTraits) { -// enabledTraits = allEnabledTraits -// } -// -// return enabledTraits -// } - - /// Given a trait, determine if the trait is enabled given the current set of enabled traits. - public func isTraitEnabled(_ trait: TraitDescription, _ enabledTraits: Set?) throws -> Bool { - guard supportsTraits else { - if var enabledTraits { - enabledTraits.remove("default") - if !enabledTraits.isEmpty { - throw TraitError.invalidTrait( - package: self.displayName, - trait: trait.name, - availableTraits: self.traits.map(\.name) - ) - } - } - - return false - } - guard !trait.isDefault else { - if self.traits.contains(where: \.isDefault) { - return true - } - throw TraitError.invalidTrait( - package: self.displayName, - trait: trait.name, - availableTraits: self.traits.map(\.name) - ) - } - - let allEnabledTraits = try calculateAllEnabledTraits(explictlyEnabledTraits: enabledTraits) - - return allEnabledTraits.contains(trait.name) - } - - /// Calculates and returns a set of all enabled traits, beginning with a set of explicitly enabled traits (which can either be the default traits of a manifest, or a configuration of enabled traits determined from a user-generated trait configuration) and determines which traits are transitively enabled. - public func calculateAllEnabledTraits( - explictlyEnabledTraits: Set?, - _ parentPackage: String? = nil - ) throws -> Set { - // This the point where we flatten the enabled traits and resolve the recursive traits - var enabledTraits = explictlyEnabledTraits ?? [] - let areDefaultsEnabled = enabledTraits.remove("default") != nil - - for trait in enabledTraits { - // Check if the enabled trait is a valid trait - if self.traits.first(where: { $0.name == trait }) == nil { - throw TraitError.invalidTrait(package: self.displayName, trait: trait) - } - } - - if !(explictlyEnabledTraits == nil || areDefaultsEnabled) && !self.supportsTraits { - // We throw an error when default traits are disabled for a package without any traits - // This allows packages to initially move new API behind traits once. - throw TraitError.traitsNotSupported( - parentPackage: parentPackage, - package: displayName, - explicitlyEnabledTraits: explictlyEnabledTraits?.map({ $0 }) ?? [] - ) - } - - // We have to enable all default traits if no traits are enabled or the defaults are explicitly enabled - if explictlyEnabledTraits == nil || areDefaultsEnabled { - if let defaultTraits { - enabledTraits.formUnion(defaultTraits.flatMap(\.enabledTraits)) - } - } - - // Iteratively flatten transitively enabled traits; stop when all transitive traits have been found. - while true { - let transitivelyEnabledTraits = try Set( - // We are going to calculate which traits are actually enabled for a node here. To do this - // we have to check if default traits should be used and then flatten all the enabled traits. - enabledTraits - .flatMap { trait in - guard let traitDescription = traitsMap[trait] else { - throw TraitError.invalidTrait(package: self.displayName, trait: trait) - } - return traitDescription.enabledTraits - } - ) - - let appendedList = enabledTraits.union(transitivelyEnabledTraits) - if appendedList.count == enabledTraits.count { - break - } else { - enabledTraits = appendedList - } - } - - return enabledTraits - } - - /// Computes the dependencies that are in use per target in this manifest. - public func usedTargetDependencies(withTraits enabledTraits: Set?) throws -> [String: Set] { - try self.targets.reduce(into: [String: Set]()) { depMap, target in - let nonTraitDeps = target.dependencies.filter { - $0.condition?.traits?.isEmpty ?? true - } - - let traitGuardedDeps = try target.dependencies.filter { dep in - let traits = dep.condition?.traits ?? [] - - // For each trait that is a condition on this target dependency, assure that - // each one is enabled in the manifest. - return try traits.allSatisfy({ try isTraitEnabled(.init(stringLiteral: $0), enabledTraits) }) - } - - let deps = nonTraitDeps + traitGuardedDeps - depMap[target.name] = Set(deps) - } - } - - /// Computes the set of package dependencies that are used by targets of this manifest. - public func usedDependencies(withTraits enabledTraits: Set?) throws -> (knownPackage: Set, unknownPackage: Set) { - let deps = try self.usedTargetDependencies(withTraits: enabledTraits) - .values - .flatMap { $0 } - .compactMap(\.package) - - var known: Set = [] - var unknown: Set = [] - - for item in deps { - if let dep = self.packageDependency(referencedBy: item) { - known.insert(dep.identity.description) - } else if self.targetMap[item] == nil { - // Marking this dependency as tentatively used, given that we cannot find the package ref at this stage. - unknown.insert(item) - } - } - - return (knownPackage: known, unknownPackage: unknown) - } - - /// Returns the set of package dependencies that are potentially guarded by traits, per target. This does not - /// calculate enabled and disabled dependencies enforced by traits. - public func traitGuardedDependencies(lowercasedKeys: Bool = false) -> [String: [String: Set]] { - self.targets.reduce(into: [String: [String: Set]]()) { depMap, target in - let traitGuardedTargetDependencies = target.dependencies.filter { - !($0.condition?.traits?.isEmpty ?? true) - } - traitGuardedTargetDependencies.forEach { - guard let package = lowercasedKeys ? $0.package?.lowercased() : $0.package else { return } - depMap[package, default: [:]][target.name, default: []].formUnion($0.condition?.traits ?? []) - } - } - } - - /// Computes the enabled traits for a given target dependency - public func enabledTraits(forDependency dependency: TargetDescription.Dependency) -> Set? { - guard let package = self.packageDependency(referencedBy: dependency), - let traits = package.traits?.compactMap(\.name) - else { - return nil - } - - return Set(traits) - } - - /// Determines whether a target dependency is enabled given a set of enabled traits for this manifest. - public func isTargetDependencyEnabled( - target: String, - _ dependency: TargetDescription.Dependency, - enabledTraits: Set?, - enableAllTraits: Bool = false - ) throws -> Bool { - guard self.supportsTraits, !enableAllTraits else { return true } - guard let package = dependency.package, let target = self.targetMap[target] else { return false } - guard target.dependencies.contains(where: { $0 == dependency }) else { - throw InternalError( - "target dependency \(dependency.name) not found for target \(target.name) in package \(self.displayName)" - ) - } - let traitsThatEnableDependency = self.traitGuardedDependencies()[package]?[target.name] ?? [] - - let isEnabled = try traitsThatEnableDependency.allSatisfy({ try isTraitEnabled(.init(stringLiteral: $0), enabledTraits) }) - - return traitsThatEnableDependency.isEmpty || isEnabled - } - - /// Determines whether a given package dependency is used by this manifest given a set of enabled traits. - public func isPackageDependencyUsed(_ dependency: PackageDependency, enabledTraits: Set?) throws -> Bool { - let usedDependencies = try self.usedDependencies(withTraits: enabledTraits) - let foundKnownPackage = usedDependencies.knownPackage.contains(where: { - $0.caseInsensitiveCompare(dependency.identity.description) == .orderedSame - }) - - // if there is a target dependency referenced by name and the package it originates from is unknown, default to - // tentatively marking the package dependency as used. to be resolved later on. - return foundKnownPackage || (!foundKnownPackage && !usedDependencies.unknownPackage.isEmpty) - } -} - -// MARK: - Trait Error - -public indirect enum TraitError: Swift.Error { - /// Indicates that an invalid trait was enabled. - case invalidTrait( - package: String, - trait: String, - availableTraits: [String] = [] - ) - - /// Indicates that the manifest does not support traits, yet a method was called with a configuration of enabled - /// traits. - case traitsNotSupported( - parentPackage: String?, - package: String, - explicitlyEnabledTraits: [String] - ) -} - -extension TraitError: CustomStringConvertible { - public var description: String { - switch self { - case .invalidTrait(let package, let trait, let availableTraits): - var errorMsg = """ - Trait '"\(trait)"' is not declared by package '\(package)'. - """ - if availableTraits.isEmpty { - errorMsg += " There are no available traits defined by this package." - } else { - errorMsg += - " The available traits defined for this package are: \(availableTraits.joined(separator: ", "))." - } - return errorMsg - case .traitsNotSupported(let parentPackage, let package, let explicitlyEnabledTraits): - if explicitlyEnabledTraits.isEmpty { - if let parentPackage { - return """ - Disabled default traits by package '\(parentPackage)' on package '\(package)' that declares no traits. This is prohibited to allow packages to adopt traits initially without causing an API break. - """ - } else { - return """ - Disabled default traits on package '\(package)' that declares no traits. This is prohibited to allow packages to adopt traits initially without causing an API break. - """ - } - } else { - if let parentPackage { - return """ - Package \(parentPackage) enables traits [\(explicitlyEnabledTraits.joined(separator: ", "))] on package '\(package)' that declares no traits. - """ - } else { - // TODO: bp fix msg - return """ - Enabled traits [\(explicitlyEnabledTraits.joined(separator: ", "))] on package '\(package)' that declares no traits. - """ - } - } - } - } -} diff --git a/Sources/PackageGraph/TraitConfiguration.swift b/Sources/PackageModel/Manifest/TraitConfiguration.swift similarity index 55% rename from Sources/PackageGraph/TraitConfiguration.swift rename to Sources/PackageModel/Manifest/TraitConfiguration.swift index 39ab635d7d2..c94295efca9 100644 --- a/Sources/PackageGraph/TraitConfiguration.swift +++ b/Sources/PackageModel/Manifest/TraitConfiguration.swift @@ -27,6 +27,9 @@ public enum TraitConfiguration: Codable, Hashable { return } + // There can be two possible cases here: + // - The set of enabled traits is empty, which means that no traits are enabled. + // - The set of enabled traits is not empty and specifies which traits are enabled. if let enabledTraits { if enabledTraits.isEmpty { self = .disableAllTraits @@ -34,7 +37,7 @@ public enum TraitConfiguration: Codable, Hashable { self = .enabledTraits(enabledTraits) } } else { - // Since enableAllTraits isn't enabled and there isn't a set of traits set, + // Since enableAllTraits isn't enabled and there isn't a set of enabled traits, // there is no configuration passed by the user. self = .none } @@ -45,62 +48,17 @@ public enum TraitConfiguration: Codable, Hashable { .init(enabledTraits: nil) } + /// The set of enabled traits, if available. public var enabledTraits: Set? { switch self { + case .none: + ["default"] case .enabledTraits(let traits): - return traits - case .none, .enableAllTraits: - return nil + traits case .disableAllTraits: - return [] - } - } - - public var enableAllTraits: Bool { - if case .enableAllTraits = self { - return true - } - - return false - } - - public var enablesDefaultTraits: Bool { - switch self { - case .enabledTraits(let traits): - return traits.contains("default") - case .none, .enableAllTraits: - return true - case .disableAllTraits: - return false - } - } - - public var enablesNonDefaultTraits: Bool { - switch self { - case .enabledTraits(let traits): - let traitsWithoutDefault = traits.subtracting(["default"]) - return !traitsWithoutDefault.isEmpty + [] case .enableAllTraits: - return true - case .none, .disableAllTraits: - return false + nil } } - -// public var enablesDefaultTraits: Bool { -// switch self { -// case .enableAllTraits, .noConfiguration: -// return true -// case .enabledTraits(let traits): -// return traits.contains("default") -// case .noEnabledTraits: -// return false -// } -// } - -// public var enabledTraitsWithoutDefault: Set? { -// switch self { -// case .enabledTraits(let traits): -// } -// } } diff --git a/Sources/SPMBuildCore/BuildSystem/BuildSystem.swift b/Sources/SPMBuildCore/BuildSystem/BuildSystem.swift index c874ef1a9d5..658c70f7c92 100644 --- a/Sources/SPMBuildCore/BuildSystem/BuildSystem.swift +++ b/Sources/SPMBuildCore/BuildSystem/BuildSystem.swift @@ -13,6 +13,8 @@ import Basics import PackageGraph +import enum PackageModel.TraitConfiguration + import protocol TSCBasic.OutputByteStream /// An enum representing what subset of the package to build. diff --git a/Sources/SourceKitLSPAPI/BuildDescription.swift b/Sources/SourceKitLSPAPI/BuildDescription.swift index 568eb442631..f13a3b9ad92 100644 --- a/Sources/SourceKitLSPAPI/BuildDescription.swift +++ b/Sources/SourceKitLSPAPI/BuildDescription.swift @@ -19,7 +19,7 @@ import Basics import Build import PackageGraph internal import PackageLoading -internal import PackageModel +import PackageModel import SPMBuildCore public enum BuildDestination { diff --git a/Sources/SourceKitLSPAPI/PluginTargetBuildDescription.swift b/Sources/SourceKitLSPAPI/PluginTargetBuildDescription.swift index 2f80f0839c4..44e109e737e 100644 --- a/Sources/SourceKitLSPAPI/PluginTargetBuildDescription.swift +++ b/Sources/SourceKitLSPAPI/PluginTargetBuildDescription.swift @@ -15,7 +15,7 @@ import Foundation import Basics import PackageGraph internal import PackageLoading -internal import PackageModel +import PackageModel struct PluginTargetBuildDescription: BuildTarget { private let target: ResolvedModule diff --git a/Sources/Workspace/PackageContainer/FileSystemPackageContainer.swift b/Sources/Workspace/PackageContainer/FileSystemPackageContainer.swift index 1a2aea3d2fb..0d9ef523c48 100644 --- a/Sources/Workspace/PackageContainer/FileSystemPackageContainer.swift +++ b/Sources/Workspace/PackageContainer/FileSystemPackageContainer.swift @@ -145,7 +145,7 @@ public struct FileSystemPackageContainer: PackageContainer { // TODO: bp throw? return [] } - let enabledTraits = try manifest.enabledTraits2(using: traitConfiguration) + let enabledTraits = try manifest.enabledTraits(using: traitConfiguration) return enabledTraits ?? [] } } diff --git a/Sources/Workspace/PackageContainer/RegistryPackageContainer.swift b/Sources/Workspace/PackageContainer/RegistryPackageContainer.swift index 1a56c93b0cd..e3febfef586 100644 --- a/Sources/Workspace/PackageContainer/RegistryPackageContainer.swift +++ b/Sources/Workspace/PackageContainer/RegistryPackageContainer.swift @@ -262,7 +262,7 @@ public class RegistryPackageContainer: PackageContainer { guard manifest.packageKind.isRoot else { return [] } - let enabledTraits = try manifest.enabledTraits2(using: traitConfiguration) + let enabledTraits = try manifest.enabledTraits(using: traitConfiguration) return enabledTraits ?? [] } } diff --git a/Sources/Workspace/PackageContainer/SourceControlPackageContainer.swift b/Sources/Workspace/PackageContainer/SourceControlPackageContainer.swift index 4b40888eacf..da1f56f959f 100644 --- a/Sources/Workspace/PackageContainer/SourceControlPackageContainer.swift +++ b/Sources/Workspace/PackageContainer/SourceControlPackageContainer.swift @@ -424,7 +424,7 @@ internal final class SourceControlPackageContainer: PackageContainer, CustomStri return [] } let manifest = try await self.loadManifest(tag: tag, version: version) - return try manifest.enabledTraits2(using: traitConfiguration) ?? [] + return try manifest.enabledTraits(using: traitConfiguration) ?? [] } public var isRemoteContainer: Bool? { diff --git a/Sources/Workspace/ResolverPrecomputationProvider.swift b/Sources/Workspace/ResolverPrecomputationProvider.swift index 77a67a80786..5319db97c10 100644 --- a/Sources/Workspace/ResolverPrecomputationProvider.swift +++ b/Sources/Workspace/ResolverPrecomputationProvider.swift @@ -185,14 +185,14 @@ private struct LocalPackageContainer: PackageContainer { if let version { switch dependency?.state { case .sourceControlCheckout(.version(version, revision: _)): - return try manifest.enabledTraits2(using: traitConfiguration) ?? [] + return try manifest.enabledTraits(using: traitConfiguration) ?? [] case .registryDownload(version: version): - return try manifest.enabledTraits2(using: traitConfiguration) ?? [] + return try manifest.enabledTraits(using: traitConfiguration) ?? [] default: throw InternalError("expected version based state, but state was \(String(describing: dependency?.state))") } } else { - return try manifest.enabledTraits2(using: traitConfiguration) ?? [] + return try manifest.enabledTraits(using: traitConfiguration) ?? [] } } } diff --git a/Sources/Workspace/Workspace+Dependencies.swift b/Sources/Workspace/Workspace+Dependencies.swift index 1a241619de5..c5e080404cf 100644 --- a/Sources/Workspace/Workspace+Dependencies.swift +++ b/Sources/Workspace/Workspace+Dependencies.swift @@ -47,7 +47,7 @@ import struct SourceControl.Revision import struct TSCUtility.Version import struct PackageModel.TargetDescription import struct PackageModel.TraitDescription -import enum PackageGraph.TraitConfiguration +import enum PackageModel.TraitConfiguration import class PackageModel.Manifest extension Workspace { diff --git a/Sources/Workspace/Workspace+Manifests.swift b/Sources/Workspace/Workspace+Manifests.swift index b954e0ff8ba..8d0334dc11e 100644 --- a/Sources/Workspace/Workspace+Manifests.swift +++ b/Sources/Workspace/Workspace+Manifests.swift @@ -27,7 +27,6 @@ import protocol PackageGraph.CustomPackageContainer import struct PackageGraph.GraphLoadingNode import struct PackageGraph.PackageContainerConstraint import struct PackageGraph.PackageGraphRoot -import enum PackageGraph.TraitConfiguration import class PackageLoading.ManifestLoader import struct PackageLoading.ManifestValidator import struct PackageLoading.ToolsVersionParser @@ -36,6 +35,7 @@ import struct PackageModel.PackageIdentity import struct PackageModel.PackageReference import enum PackageModel.ProductFilter import struct PackageModel.ToolsVersion +import enum PackageModel.TraitConfiguration import protocol TSCBasic.FileSystem import func TSCBasic.findCycle import struct TSCBasic.KeyedPair diff --git a/Tests/PackageModelTests/ManifestTests.swift b/Tests/PackageModelTests/ManifestTests.swift index 59534857502..6413edc6015 100644 --- a/Tests/PackageModelTests/ManifestTests.swift +++ b/Tests/PackageModelTests/ManifestTests.swift @@ -358,7 +358,7 @@ class ManifestTests: XCTestCase { // Calculate the enabled traits without an explicitly declared set of enabled traits. // This should default to fetching the default traits, if they exist (which in this test case // they do), and then will calculate the transitive set of traits that are enabled. - let allEnabledTraits = try XCTUnwrap(manifest.enabledTraits2(using: .default)).sorted() + let allEnabledTraits = try XCTUnwrap(manifest.enabledTraits(using: .default)).sorted() XCTAssertEqual(allEnabledTraits, ["Trait1", "Trait2"]) } } @@ -392,13 +392,13 @@ class ManifestTests: XCTestCase { // Calculate the enabled traits with an explicitly declared set of enabled traits. // This should override the default traits (since it isn't explicitly passed in here). - let allEnabledTraitsWithoutDefaults = try manifest.enabledTraits2(using: .enabledTraits(["Trait3"]))?.sorted() + let allEnabledTraitsWithoutDefaults = try manifest.enabledTraits(using: .enabledTraits(["Trait3"]))?.sorted() XCTAssertEqual(allEnabledTraitsWithoutDefaults, ["Trait3"]) // Calculate the enabled traits with an explicitly declared set of enabled traits, // including the default traits. Since default traits are explicitly enabled in the // passed set of traits, this will be factored into the calculation. - let allEnabledTraitsWithDefaults = try manifest.enabledTraits2(using: .enabledTraits(["default", "Trait3"]))?.sorted() + let allEnabledTraitsWithDefaults = try manifest.enabledTraits(using: .enabledTraits(["default", "Trait3"]))?.sorted() XCTAssertEqual(allEnabledTraitsWithDefaults, ["Trait1", "Trait2", "Trait3"]) } } @@ -431,7 +431,7 @@ class ManifestTests: XCTestCase { ) // Calculate the enabled traits with all traits enabled flag. - let allEnabledTraits = try manifest.enabledTraits2(using: .enableAllTraits)?.sorted() + let allEnabledTraits = try manifest.enabledTraits(using: .enableAllTraits)?.sorted() XCTAssertEqual(allEnabledTraits, ["Trait1", "Trait2", "Trait3"]) } } From c359e4d406f36b30c7d2944d58670d8534619377 Mon Sep 17 00:00:00 2001 From: Bri Peticca Date: Tue, 25 Mar 2025 11:42:38 -0400 Subject: [PATCH 6/8] Fix trait validators + clean up error messages --- .../Resolution/DependencyResolutionNode.swift | 20 +-- .../PubGrub/PubGrubPackageContainer.swift | 6 +- .../Manifest/Manifest+Traits.swift | 116 +++++++++--------- .../FileSystemPackageContainer.swift | 4 - Sources/Workspace/Workspace+Manifests.swift | 9 +- 5 files changed, 74 insertions(+), 81 deletions(-) diff --git a/Sources/PackageGraph/Resolution/DependencyResolutionNode.swift b/Sources/PackageGraph/Resolution/DependencyResolutionNode.swift index 6376c1adc66..8865707b651 100644 --- a/Sources/PackageGraph/Resolution/DependencyResolutionNode.swift +++ b/Sources/PackageGraph/Resolution/DependencyResolutionNode.swift @@ -94,14 +94,7 @@ public enum DependencyResolutionNode { public var traits: Set? { switch self { case .root(_, let config): - switch config { - case .enabledTraits(let traits): - return traits - case .disableAllTraits: - return [] - case .none, .enableAllTraits: // TODO: bp fix - return nil - } + return config.enabledTraits case .product(_, _, let enabledTraits): return enabledTraits default: @@ -109,6 +102,17 @@ public enum DependencyResolutionNode { } } + public var traitConfiguration: TraitConfiguration { + switch self { + case .root(_, let config): + return config + case .product(_, _, let enabledTraits): + return .init(enabledTraits: enabledTraits) + case .empty: + return .default + } + } + /// Returns the dependency that a product has on its own package, if relevant. /// /// This is the constraint that requires all products from a package resolve to the same version. diff --git a/Sources/PackageGraph/Resolution/PubGrub/PubGrubPackageContainer.swift b/Sources/PackageGraph/Resolution/PubGrub/PubGrubPackageContainer.swift index 7974e0dbb03..37893363a04 100644 --- a/Sources/PackageGraph/Resolution/PubGrub/PubGrubPackageContainer.swift +++ b/Sources/PackageGraph/Resolution/PubGrub/PubGrubPackageContainer.swift @@ -154,8 +154,7 @@ final class PubGrubPackageContainer { at version: Version, node: DependencyResolutionNode, overriddenPackages: [PackageReference: (version: BoundVersion, products: ProductFilter)], - root: DependencyResolutionNode, - traitConfiguration: TraitConfiguration = .default + root: DependencyResolutionNode ) async throws -> [Incompatibility] { // FIXME: It would be nice to compute bounds for this as well. if await !self.underlying.isToolsVersionCompatible(at: version) { @@ -168,10 +167,11 @@ final class PubGrubPackageContainer { )] } + let enabledTraits = node.package.kind.isRoot ? try await self.underlying.getEnabledTraits(traitConfiguration: node.traitConfiguration) : node.traits var unprocessedDependencies = try await self.underlying.getDependencies( at: version, productFilter: node.productFilter, - node.traits + enabledTraits ) if let sharedVersion = node.versionLock(version: version) { unprocessedDependencies.append(sharedVersion) diff --git a/Sources/PackageModel/Manifest/Manifest+Traits.swift b/Sources/PackageModel/Manifest/Manifest+Traits.swift index 2f0b7918ed6..5ab9e37e8df 100644 --- a/Sources/PackageModel/Manifest/Manifest+Traits.swift +++ b/Sources/PackageModel/Manifest/Manifest+Traits.swift @@ -29,7 +29,7 @@ extension Manifest { /// Validates a trait by checking that it is defined in the manifest; if not, an error is thrown. private func validateTrait(_ trait: String) throws { - // Check if the enabled trait is a valid trait + // Check if the passed trait is a valid trait. if self.traits.first(where: { $0.name == trait }) == nil { throw TraitError.invalidTrait(package: self.displayName, trait: trait) } @@ -98,7 +98,10 @@ extension Manifest { } } - var enabledTraits = traitConfiguration.enabledTraits + // Get the enabled traits; if the trait configuration's `.enabledTraits` returns nil, + // we know that it's the `.enableAllTraits` case, since the config does not store + // all the defined traits of the manifest itself. + let enabledTraits = traitConfiguration.enabledTraits ?? Set(self.traits.map({ $0.name })) try validateEnabledTraits(enabledTraits) } @@ -123,18 +126,14 @@ extension Manifest { } } - /// Calculates the set of all transitive traits that are enabled for this manifest using the passed set of - /// explicitly enabled traits and a flag that - /// determines whether all traits are enabled. + /// Calculates the set of all transitive traits that are enabled for this manifest using the passed trait configuration. + /// Since a trait configuration is only used for root packages, this method is intended for use with root packages only. public func enabledTraits(using traitConfiguration: TraitConfiguration) throws -> Set? { - guard supportsTraits else { - // If this manifest does not support traits, but the passed configuration either - // disables default traits or enables non-default traits (i.e. traits that would - // not exist for this manifest) then we must throw an error. - - // TODO: bp redundant guards? may be able to change this to a bool - try validateTraitConfiguration(traitConfiguration) - + // If this manifest does not support traits, but the passed configuration either + // disables default traits or enables non-default traits (i.e. traits that would + // not exist for this manifest) then we must throw an error. + try validateTraitConfiguration(traitConfiguration) + guard supportsTraits, packageKind.isRoot else { return nil } @@ -153,36 +152,58 @@ extension Manifest { enabledTraits = explicitlyEnabledTraits } - if let allEnabledTraits = try? calculateAllEnabledTraits(explictlyEnabledTraits: enabledTraits) { + if let allEnabledTraits = try? self.enabledTraits(using: enabledTraits, nil) { enabledTraits = allEnabledTraits } return enabledTraits } + /// Calculates the set of all transitive traits that are enabled for this manifest using the passed set of + /// explicitly enabled traits, and the parent package that defines the enabled traits for this package. + /// This method is intended for use with non-root packages. + public func enabledTraits(using explicitlyEnabledTraits: Set?, _ parentPackage: String?) throws -> Set? { + // If this manifest does not support traits, but the passed configuration either + // disables default traits or enables non-default traits (i.e. traits that would + // not exist for this manifest) then we must throw an error. + try validateEnabledTraits(explicitlyEnabledTraits, parentPackage) + guard supportsTraits else { + return nil + } - /// Given a trait, determine if the trait is enabled given the current set of enabled traits. + var enabledTraits: Set = [] + + if let allEnabledTraits = try? calculateAllEnabledTraits(explictlyEnabledTraits: explicitlyEnabledTraits, parentPackage) { + enabledTraits = allEnabledTraits + } + + return enabledTraits + + } + + /// Determines if a trait is enabled with a given set of enabled traits. public func isTraitEnabled(_ trait: TraitDescription, _ enabledTraits: Set?) throws -> Bool { - guard supportsTraits else { - if var enabledTraits { - enabledTraits.remove("default") - if !enabledTraits.isEmpty { - throw TraitError.invalidTrait( - package: self.displayName, - trait: trait.name, - availableTraits: self.traits.map(\.name) - ) - } - } + // First, check if the trait that we want to determine is enabled is a valid trait. + try validateTrait(trait) + // Then, check if the list of enabled traits is valid. + try validateEnabledTraits(enabledTraits) + guard supportsTraits else { + // If the above checks pass without throwing an error, then we simply return false + // if the manifest does not support traits. return false } - // TODO: bp double check that this guard/check is doing what it's meant to do... - // i.e. what if the set of enabled traits does not specify default? + // Special case for dealing with whether a default trait is enabled. guard !trait.isDefault else { + // Check that the manifest defines default traits. if self.traits.contains(where: \.isDefault) { - // TODO: bp if there is either no config (nil enabledTraits) or the enabledTraits contains "default", then we can just return true. else, return false. + // If the trait is a default trait, then we must do the following checks: + // - If there exists a list of enabled traits, ensure that the default trait + // is declared in the set. + // - If there is no existing list of enabled traits (nil), and we know that the + // manifest has defined default traits, then just return true. + // - If none of these conditions are met, then defaults aren't enabled and we return false. if let enabledTraits, enabledTraits.contains(trait.name) { return true } else if enabledTraits == nil { @@ -191,6 +212,8 @@ extension Manifest { return false } } + + // If manifest does not define default traits, then throw an invalid trait error. throw TraitError.invalidTrait( package: self.displayName, trait: trait.name, @@ -198,37 +221,22 @@ extension Manifest { ) } + // Compute all transitively enabled traits. let allEnabledTraits = try calculateAllEnabledTraits(explictlyEnabledTraits: enabledTraits) return allEnabledTraits.contains(trait.name) } /// Calculates and returns a set of all enabled traits, beginning with a set of explicitly enabled traits (which can either be the default traits of a manifest, or a configuration of enabled traits determined from a user-generated trait configuration) and determines which traits are transitively enabled. - public func calculateAllEnabledTraits( + private func calculateAllEnabledTraits( explictlyEnabledTraits: Set?, _ parentPackage: String? = nil ) throws -> Set { + try validateEnabledTraits(explictlyEnabledTraits, parentPackage) // This the point where we flatten the enabled traits and resolve the recursive traits var enabledTraits = explictlyEnabledTraits ?? [] let areDefaultsEnabled = enabledTraits.remove("default") != nil - for trait in enabledTraits { - // Check if the enabled trait is a valid trait - if self.traits.first(where: { $0.name == trait }) == nil { - throw TraitError.invalidTrait(package: self.displayName, trait: trait) - } - } - - if !(explictlyEnabledTraits == nil || areDefaultsEnabled) && !self.supportsTraits { - // We throw an error when default traits are disabled for a package without any traits - // This allows packages to initially move new API behind traits once. - throw TraitError.traitsNotSupported( - parentPackage: parentPackage, - package: displayName, - explicitlyEnabledTraits: explictlyEnabledTraits?.map({ $0 }) ?? [] - ) - } - // We have to enable all default traits if no traits are enabled or the defaults are explicitly enabled if explictlyEnabledTraits == nil || areDefaultsEnabled { if let defaultTraits { @@ -317,17 +325,6 @@ extension Manifest { } } - /// Computes the enabled traits for a given target dependency - public func enabledTraits(forDependency dependency: TargetDescription.Dependency) -> Set? { - guard let package = self.packageDependency(referencedBy: dependency), - let traits = package.traits?.compactMap(\.name) - else { - return nil - } - - return Set(traits) - } - /// Determines whether a target dependency is enabled given a set of enabled traits for this manifest. public func isTargetDependencyEnabled( target: String, @@ -412,9 +409,8 @@ extension TraitError: CustomStringConvertible { Package \(parentPackage) enables traits [\(explicitlyEnabledTraits.joined(separator: ", "))] on package '\(package)' that declares no traits. """ } else { - // TODO: bp fix msg return """ - Enabled traits [\(explicitlyEnabledTraits.joined(separator: ", "))] on package '\(package)' that declares no traits. + Traits [\(explicitlyEnabledTraits.joined(separator: ", "))] have been enabled on package '\(package)' that declares no traits. """ } } diff --git a/Sources/Workspace/PackageContainer/FileSystemPackageContainer.swift b/Sources/Workspace/PackageContainer/FileSystemPackageContainer.swift index 0d9ef523c48..c1892c25aee 100644 --- a/Sources/Workspace/PackageContainer/FileSystemPackageContainer.swift +++ b/Sources/Workspace/PackageContainer/FileSystemPackageContainer.swift @@ -141,10 +141,6 @@ public struct FileSystemPackageContainer: PackageContainer { throw InternalError("File system package container does not support versioning.") } let manifest = try await loadManifest() - guard manifest.packageKind.isRoot else { - // TODO: bp throw? - return [] - } let enabledTraits = try manifest.enabledTraits(using: traitConfiguration) return enabledTraits ?? [] } diff --git a/Sources/Workspace/Workspace+Manifests.swift b/Sources/Workspace/Workspace+Manifests.swift index 8d0334dc11e..9600ba06545 100644 --- a/Sources/Workspace/Workspace+Manifests.swift +++ b/Sources/Workspace/Workspace+Manifests.swift @@ -713,11 +713,8 @@ extension Workspace { return !conditionTraits.intersection(node.item.enabledTraits).isEmpty }.map(\.name) - // TODO: bp must now calculate all transitively enabled traits of the deps - let calculatedTraits = try manifest.calculateAllEnabledTraits( -// parentPackage: node.item.identity, -// identity: dependency.identity, - explictlyEnabledTraits: explicitlyEnabledTraits.flatMap { Set($0) }, + let calculatedTraits = try manifest.enabledTraits( + using: explicitlyEnabledTraits.flatMap { Set($0) }, node.item.identity.description ) @@ -727,7 +724,7 @@ extension Workspace { identity: dependency.identity, manifest: manifest, productFilter: dependency.productFilter, - enabledTraits: calculatedTraits + enabledTraits: calculatedTraits ?? [] ), key: dependency.identity ) : From 0ac83c3df19be4c79c1f12eca9fecee61e079975 Mon Sep 17 00:00:00 2001 From: Bri Peticca Date: Tue, 25 Mar 2025 16:54:22 -0400 Subject: [PATCH 7/8] Modify MockDependency trait support and TraitError messages - To support further testing on traits and their possible configurations, set traits where appropriate in the MockDependency + make PackageDependency.Trait public to allow its use in the mocks. - Fixed up the error messages displayed for traits - Added more tests to cover various possible configurations of traits --- .../Manifest/Manifest+Traits.swift | 112 +++++++--- .../PackageDependencyDescription.swift | 2 +- .../ManifestExtensions.swift | 6 +- .../_InternalTestSupport/MockDependency.swift | 46 ++-- .../_InternalTestSupport/MockPackage.swift | 6 +- Tests/PackageModelTests/ManifestTests.swift | 200 +++++++++++++++++- Tests/WorkspaceTests/WorkspaceTests.swift | 121 +++++++++++ 7 files changed, 431 insertions(+), 62 deletions(-) diff --git a/Sources/PackageModel/Manifest/Manifest+Traits.swift b/Sources/PackageModel/Manifest/Manifest+Traits.swift index 5ab9e37e8df..9ffe7f5aa3a 100644 --- a/Sources/PackageModel/Manifest/Manifest+Traits.swift +++ b/Sources/PackageModel/Manifest/Manifest+Traits.swift @@ -24,34 +24,60 @@ extension Manifest { /// Validates a trait by checking that it is defined in the manifest; if not, an error is thrown. private func validateTrait(_ trait: TraitDescription) throws { + guard !trait.isDefault else { + if !supportsTraits { + throw TraitError.invalidTrait( + package: self.displayName, + trait: trait.name, + availableTraits: traits.map({ $0.name }) + ) + } + + return + } + try self.validateTrait(trait.name) } /// Validates a trait by checking that it is defined in the manifest; if not, an error is thrown. - private func validateTrait(_ trait: String) throws { + private func validateTrait(_ trait: String, parentPackage: String? = nil) throws { + guard trait != "default" else { + if !supportsTraits { + throw TraitError.invalidTrait( + package: self.displayName, + trait: trait, + availableTraits: traits.map({ $0.name }) + ) + } + + return + } + // Check if the passed trait is a valid trait. if self.traits.first(where: { $0.name == trait }) == nil { - throw TraitError.invalidTrait(package: self.displayName, trait: trait) + throw TraitError.invalidTrait( + package: self.displayName, + trait: trait, + availableTraits: self.traits.map({ $0.name }), + parentPackage: parentPackage + ) } } /// Validates a set of traits that is intended to be enabled for the manifest; if there are any discrepencies in the /// set of enabled traits and whether the manifest defines these traits (or if it defines any traits at all), then an /// error indicating the issue will be thrown. - public func validateEnabledTraits( + private func validateEnabledTraits( _ explicitlyEnabledTraits: Set?, _ parentPackage: String? = nil ) throws { guard supportsTraits else { - if let explicitlyEnabledTraits { - let enabledTraitsWithoutDefault = explicitlyEnabledTraits.subtracting(["default"]) - if !enabledTraitsWithoutDefault.isEmpty { - throw TraitError.traitsNotSupported( - parentPackage: parentPackage, - package: self.displayName, - explicitlyEnabledTraits: explicitlyEnabledTraits.map({ $0 }) - ) - } + if let explicitlyEnabledTraits, !explicitlyEnabledTraits.contains("default") { + throw TraitError.traitsNotSupported( + parentPackage: parentPackage, + package: self.displayName, + explicitlyEnabledTraits: explicitlyEnabledTraits.map({ $0 }) + ) } return @@ -61,7 +87,7 @@ extension Manifest { // Validate each trait to assure it's defined in the current package. for trait in enabledTraits { - try validateTrait(trait) + try validateTrait(trait, parentPackage: parentPackage) } let areDefaultsEnabled = enabledTraits.contains("default") @@ -183,11 +209,38 @@ extension Manifest { /// Determines if a trait is enabled with a given set of enabled traits. public func isTraitEnabled(_ trait: TraitDescription, _ enabledTraits: Set?) throws -> Bool { - // First, check if the trait that we want to determine is enabled is a valid trait. + // First, check that the queried trait is valid. try validateTrait(trait) - // Then, check if the list of enabled traits is valid. + // Then, check that the list of enabled traits is valid. try validateEnabledTraits(enabledTraits) + // Special case for dealing with whether a default trait is enabled. + guard !trait.isDefault else { + // Check that the manifest defines default traits. + if self.traits.contains(where: \.isDefault) { + // If the trait is a default trait, then we must do the following checks: + // - If there exists a list of enabled traits, ensure that the default trait + // is declared in the set. + // - If there is no existing list of enabled traits (nil), and we know that the + // manifest has defined default traits, then just return true. + // - If none of these conditions are met, then defaults aren't enabled and we return false. + if let enabledTraits, enabledTraits.contains(trait.name) { + return true + } else if enabledTraits == nil { + return true + } else { + return false + } + } + + // If manifest does not define default traits, then throw an invalid trait error. + throw TraitError.invalidTrait( + package: self.displayName, + trait: trait.name, + availableTraits: self.traits.map(\.name) + ) + } + guard supportsTraits else { // If the above checks pass without throwing an error, then we simply return false // if the manifest does not support traits. @@ -252,7 +305,11 @@ extension Manifest { enabledTraits .flatMap { trait in guard let traitDescription = traitsMap[trait] else { - throw TraitError.invalidTrait(package: self.displayName, trait: trait) + throw TraitError.invalidTrait( + package: self.displayName, + trait: trait, + parentPackage: parentPackage + ) } return traitDescription.enabledTraits } @@ -366,7 +423,8 @@ public enum TraitError: Swift.Error { case invalidTrait( package: String, trait: String, - availableTraits: [String] = [] + availableTraits: [String] = [], + parentPackage: String? = nil ) /// Indicates that the manifest does not support traits, yet a method was called with a configuration of enabled @@ -381,18 +439,22 @@ public enum TraitError: Swift.Error { extension TraitError: CustomStringConvertible { public var description: String { switch self { - case .invalidTrait(let package, let trait, let availableTraits): - var errorMsg = """ - Trait '"\(trait)"' is not declared by package '\(package)'. - """ + case .invalidTrait(let package, let trait, var availableTraits, let parentPackage): + availableTraits = availableTraits.sorted() + var errorMsg = "Trait '\(trait)'" + if let parentPackage { + errorMsg += " enabled by parent package '\(parentPackage)'" + } + errorMsg += " is not declared by package '\(package)'." if availableTraits.isEmpty { - errorMsg += " There are no available traits defined by this package." + errorMsg += " There are no available traits declared by this package." } else { errorMsg += - " The available traits defined for this package are: \(availableTraits.joined(separator: ", "))." + " The available traits declared by this package are: \(availableTraits.joined(separator: ", "))." } return errorMsg - case .traitsNotSupported(let parentPackage, let package, let explicitlyEnabledTraits): + case .traitsNotSupported(let parentPackage, let package, var explicitlyEnabledTraits): + explicitlyEnabledTraits = explicitlyEnabledTraits.sorted() if explicitlyEnabledTraits.isEmpty { if let parentPackage { return """ @@ -406,7 +468,7 @@ extension TraitError: CustomStringConvertible { } else { if let parentPackage { return """ - Package \(parentPackage) enables traits [\(explicitlyEnabledTraits.joined(separator: ", "))] on package '\(package)' that declares no traits. + Package '\(parentPackage)' enables traits [\(explicitlyEnabledTraits.joined(separator: ", "))] on package '\(package)' that declares no traits. """ } else { return """ diff --git a/Sources/PackageModel/Manifest/PackageDependencyDescription.swift b/Sources/PackageModel/Manifest/PackageDependencyDescription.swift index 647fe161a0d..8cc316b3e81 100644 --- a/Sources/PackageModel/Manifest/PackageDependencyDescription.swift +++ b/Sources/PackageModel/Manifest/PackageDependencyDescription.swift @@ -20,7 +20,7 @@ import struct TSCUtility.Version /// Represents a package dependency. public enum PackageDependency: Equatable, Hashable, Sendable { /// A struct representing an enabled trait of a dependency. - package struct Trait: Hashable, Sendable, Codable, ExpressibleByStringLiteral { + public struct Trait: Hashable, Sendable, Codable, ExpressibleByStringLiteral { /// A condition that limits the application of a dependencies trait. package struct Condition: Hashable, Sendable, Codable { /// The set of traits of this package that enable the dependency's trait. diff --git a/Sources/_InternalTestSupport/ManifestExtensions.swift b/Sources/_InternalTestSupport/ManifestExtensions.swift index 7a4cf3b9b10..7f65ad5cf2d 100644 --- a/Sources/_InternalTestSupport/ManifestExtensions.swift +++ b/Sources/_InternalTestSupport/ManifestExtensions.swift @@ -32,7 +32,7 @@ extension Manifest { dependencies: [PackageDependency] = [], products: [ProductDescription] = [], targets: [TargetDescription] = [], - traits: Set = [.init(name: "defaults")], + traits: Set = [.init(name: "default")], pruneDependencies: Bool = false ) -> Manifest { Self.createManifest( @@ -73,7 +73,7 @@ extension Manifest { dependencies: [PackageDependency] = [], products: [ProductDescription] = [], targets: [TargetDescription] = [], - traits: Set = [.init(name: "defaults")], + traits: Set = [.init(name: "default")], pruneDependencies: Bool = false ) -> Manifest { Self.createManifest( @@ -237,7 +237,7 @@ extension Manifest { dependencies: [PackageDependency] = [], products: [ProductDescription] = [], targets: [TargetDescription] = [], - traits: Set = [.init(name: "defaults")], + traits: Set = [.init(name: "default")], pruneDependencies: Bool = false ) -> Manifest { return Manifest( diff --git a/Sources/_InternalTestSupport/MockDependency.swift b/Sources/_InternalTestSupport/MockDependency.swift index f0e59a78070..1553decf02b 100644 --- a/Sources/_InternalTestSupport/MockDependency.swift +++ b/Sources/_InternalTestSupport/MockDependency.swift @@ -22,7 +22,7 @@ public struct MockDependency { public let deprecatedName: String? public let location: Location public let products: ProductFilter - package let traits: Set + public let traits: Set init( deprecatedName: String? = nil, @@ -49,7 +49,8 @@ public struct MockDependency { identity: identity, deprecatedName: self.deprecatedName, path: mappedPath, - productFilter: self.products + productFilter: self.products, + traits: self.traits ) case .localSourceControl(let path, let requirement): let absolutePath = baseURL.appending(path) @@ -63,7 +64,8 @@ public struct MockDependency { deprecatedName: self.deprecatedName, path: mappedPath, requirement: requirement, - productFilter: self.products + productFilter: self.products, + traits: self.traits ) case .remoteSourceControl(let url, let _requirement): let mappedLocation = identityResolver.mappedLocation(for: url.absoluteString) @@ -93,7 +95,8 @@ public struct MockDependency { deprecatedName: self.deprecatedName, url: mappedURL, requirement: _requirement, - productFilter: self.products + productFilter: self.products, + traits: self.traits ) } case .registry(let identity, let _requirement): @@ -121,43 +124,44 @@ public struct MockDependency { deprecatedName: self.deprecatedName, url: mappedURL, requirement: requirement, - productFilter: self.products + productFilter: self.products, + traits: self.traits ) } } } - public static func fileSystem(path: String, products: ProductFilter = .everything) -> MockDependency { - try! MockDependency(location: .fileSystem(path: RelativePath(validating: path)), products: products) + public static func fileSystem(path: String, products: ProductFilter = .everything, traits: Set = []) -> MockDependency { + try! MockDependency(location: .fileSystem(path: RelativePath(validating: path)), products: products, traits: traits) } - public static func sourceControl(path: String, requirement: SourceControlRequirement, products: ProductFilter = .everything) -> MockDependency { - try! .sourceControl(path: RelativePath(validating: path), requirement: requirement, products: products) + public static func sourceControl(path: String, requirement: SourceControlRequirement, products: ProductFilter = .everything, traits: Set = []) -> MockDependency { + try! .sourceControl(path: RelativePath(validating: path), requirement: requirement, products: products, traits: traits) } - public static func sourceControl(path: RelativePath, requirement: SourceControlRequirement, products: ProductFilter = .everything) -> MockDependency { - MockDependency(location: .localSourceControl(path: path, requirement: requirement), products: products) + public static func sourceControl(path: RelativePath, requirement: SourceControlRequirement, products: ProductFilter = .everything, traits: Set = []) -> MockDependency { + MockDependency(location: .localSourceControl(path: path, requirement: requirement), products: products, traits: traits) } - public static func sourceControlWithDeprecatedName(name: String, path: String, requirement: SourceControlRequirement, products: ProductFilter = .everything) -> MockDependency { - try! MockDependency(deprecatedName: name, location: .localSourceControl(path: RelativePath(validating: path), requirement: requirement), products: products) + public static func sourceControlWithDeprecatedName(name: String, path: String, requirement: SourceControlRequirement, products: ProductFilter = .everything, traits: Set = []) -> MockDependency { + try! MockDependency(deprecatedName: name, location: .localSourceControl(path: RelativePath(validating: path), requirement: requirement), products: products, traits: traits) } - public static func sourceControl(url: String, requirement: SourceControlRequirement, products: ProductFilter = .everything) -> MockDependency { - .sourceControl(url: SourceControlURL(url), requirement: requirement, products: products) + public static func sourceControl(url: String, requirement: SourceControlRequirement, products: ProductFilter = .everything, traits: Set = []) -> MockDependency { + .sourceControl(url: SourceControlURL(url), requirement: requirement, products: products, traits: traits) } - public static func sourceControl(url: SourceControlURL, requirement: SourceControlRequirement, products: ProductFilter = .everything) -> MockDependency { - MockDependency(location: .remoteSourceControl(url: url, requirement: requirement), products: products) + public static func sourceControl(url: SourceControlURL, requirement: SourceControlRequirement, products: ProductFilter = .everything, traits: Set = []) -> MockDependency { + MockDependency(location: .remoteSourceControl(url: url, requirement: requirement), products: products, traits: traits) } - public static func registry(identity: String, requirement: RegistryRequirement, products: ProductFilter = .everything) -> MockDependency { - .registry(identity: .plain(identity), requirement: requirement) + public static func registry(identity: String, requirement: RegistryRequirement, products: ProductFilter = .everything, traits: Set = []) -> MockDependency { + .registry(identity: .plain(identity), requirement: requirement, traits: traits) } - public static func registry(identity: PackageIdentity, requirement: RegistryRequirement, products: ProductFilter = .everything) -> MockDependency { - MockDependency(location: .registry(identity: identity, requirement: requirement), products: products) + public static func registry(identity: PackageIdentity, requirement: RegistryRequirement, products: ProductFilter = .everything, traits: Set = []) -> MockDependency { + MockDependency(location: .registry(identity: identity, requirement: requirement), products: products, traits: traits) } public enum Location { diff --git a/Sources/_InternalTestSupport/MockPackage.swift b/Sources/_InternalTestSupport/MockPackage.swift index 06018c20b96..0185514e486 100644 --- a/Sources/_InternalTestSupport/MockPackage.swift +++ b/Sources/_InternalTestSupport/MockPackage.swift @@ -35,7 +35,7 @@ public struct MockPackage { targets: [MockTarget], products: [MockProduct] = [], dependencies: [MockDependency] = [], - traits: Set = [.init(name: "defaults")], + traits: Set = [.init(name: "default")], versions: [String?] = [], revisionProvider: ((String) -> String)? = nil, toolsVersion: ToolsVersion? = nil @@ -60,7 +60,7 @@ public struct MockPackage { targets: [MockTarget], products: [MockProduct], dependencies: [MockDependency] = [], - traits: Set = [.init(name: "defaults")], + traits: Set = [.init(name: "default")], versions: [String?] = [], revisionProvider: ((String) -> String)? = nil, toolsVersion: ToolsVersion? = nil @@ -86,7 +86,7 @@ public struct MockPackage { targets: [MockTarget], products: [MockProduct], dependencies: [MockDependency] = [], - traits: Set = [.init(name: "defaults")], + traits: Set = [.init(name: "default")], versions: [String?] = [], revisionProvider: ((String) -> String)? = nil, toolsVersion: ToolsVersion? = nil diff --git a/Tests/PackageModelTests/ManifestTests.swift b/Tests/PackageModelTests/ManifestTests.swift index 6413edc6015..ac9ee7d691c 100644 --- a/Tests/PackageModelTests/ManifestTests.swift +++ b/Tests/PackageModelTests/ManifestTests.swift @@ -165,7 +165,7 @@ class ManifestTests: XCTestCase { } } - func testEnabledTraits_WhenNoTraitsInManifest() throws { + func testIsTraitEnabled_WhenNoTraitsInManifest() throws { let products = try [ ProductDescription(name: "Foo", type: .library(.automatic), targets: ["Foo"]), ProductDescription(name: "Bar", type: .library(.automatic), targets: ["Bar"]), @@ -205,17 +205,154 @@ class ManifestTests: XCTestCase { for trait in traits.sorted(by: { $0.name < $1.name }) { XCTAssertThrowsError(try manifest.isTraitEnabled(trait, Set(traits.map(\.name)))) { error in XCTAssertEqual("\(error)", """ - Trait '"\( + Trait '\( trait .name - )"' is not declared by package 'Foo'. There are no available traits defined by this package. + )' is not declared by package 'Foo'. There are no available traits declared by this package. """) } } } } - func testEnabledTraits_WhenNoDefaultTraitsAndNoConfig() throws { + func testIsTraitEnabled_WhenInvalidTraitQueried() throws { + let products = try [ + ProductDescription(name: "Foo", type: .library(.automatic), targets: ["Foo"]), + ProductDescription(name: "Bar", type: .library(.automatic), targets: ["Bar"]), + ] + + let targets = try [ + TargetDescription(name: "Foo", dependencies: ["Bar"]), + TargetDescription(name: "Bar", dependencies: ["Baz"]), + TargetDescription(name: "Baz", dependencies: ["MyPlugin"]), + TargetDescription(name: "FooBar", dependencies: []), + TargetDescription(name: "MyPlugin", type: .plugin, pluginCapability: .buildTool), + ] + + let traits: Set = [ + TraitDescription(name: "Trait1", enabledTraits: ["Trait2"]), + TraitDescription(name: "Trait2"), + ] + + let dependencies: [PackageDependency] = [ + .localSourceControl(path: "/Bar", requirement: .upToNextMajor(from: "1.0.0")), + .localSourceControl(path: "/Baz", requirement: .upToNextMajor(from: "1.0.0")), + .localSourceControl(path: "/MyPlugin", requirement: .upToNextMajor(from: "1.0.0")), + ] + + do { + let manifest = Manifest.createRootManifest( + displayName: "Foo", + path: "/Foo", + toolsVersion: .v5_2, + dependencies: dependencies, + products: products, + targets: targets, + traits: traits, + pruneDependencies: true // Since all dependencies are used, this shouldn't affect the outcome. + ) + + // Test `isTraitEnabled` when the trait we're querying for does not exist. + XCTAssertThrowsError(try manifest.isTraitEnabled(.init(stringLiteral: "IDontExist"), nil)) { error in + XCTAssertEqual("\(error)", """ + Trait 'IDontExist' is not declared by package 'Foo'. The available traits declared by this package are: Trait1, Trait2. + """) + } + + // Test `isTraitEnabled` when the set of enabled traits contains a trait that isn't defined in the package. + XCTAssertThrowsError(try manifest.isTraitEnabled(.init(stringLiteral: "Trait1"), ["IDontExist"])) { error in + XCTAssertEqual("\(error)", """ + Trait 'IDontExist' is not declared by package 'Foo'. The available traits declared by this package are: Trait1, Trait2. + """) + } + + // Test `isTraitEnabled` when the set of enabled traits contains a trait that isn't defined in the package, and the queried trait is the same non-existant trait. + XCTAssertThrowsError(try manifest.isTraitEnabled(.init(stringLiteral: "IDontExist"), ["IDontExist"])) { error in + XCTAssertEqual("\(error)", """ + Trait 'IDontExist' is not declared by package 'Foo'. The available traits declared by this package are: Trait1, Trait2. + """) + } + + // Test `isTraitEnabled` when the set of enabled traits contains a trait that isn't defined in the package, and the queried trait is another non-existant trait. + XCTAssertThrowsError(try manifest.isTraitEnabled(.init(stringLiteral: "IDontExistPart2"), ["IDontExist"])) { error in + XCTAssertEqual("\(error)", """ + Trait 'IDontExistPart2' is not declared by package 'Foo'. The available traits declared by this package are: Trait1, Trait2. + """) + } + + } + } + + func testEnabledTraits_WhenTraitsNotSupported() throws { + let products = try [ + ProductDescription(name: "Foo", type: .library(.automatic), targets: ["Foo"]), + ProductDescription(name: "Bar", type: .library(.automatic), targets: ["Bar"]), + ] + + let targets = try [ + TargetDescription(name: "Foo", dependencies: ["Bar"]), + TargetDescription(name: "Bar", dependencies: ["Baz"]), + TargetDescription(name: "Baz", dependencies: ["MyPlugin"]), + TargetDescription(name: "FooBar", dependencies: []), + TargetDescription(name: "MyPlugin", type: .plugin, pluginCapability: .buildTool), + ] + + let dependencies: [PackageDependency] = [ + .localSourceControl(path: "/Bar", requirement: .upToNextMajor(from: "1.0.0")), + .localSourceControl(path: "/Baz", requirement: .upToNextMajor(from: "1.0.0")), + .localSourceControl(path: "/MyPlugin", requirement: .upToNextMajor(from: "1.0.0")), + ] + + do { + let manifest = Manifest.createRootManifest( + displayName: "Foo", + path: "/Foo", + toolsVersion: .v5_2, + dependencies: dependencies, + products: products, + targets: targets, + traits: [], + pruneDependencies: true // Since all dependencies are used, this shouldn't affect the outcome. + ) + + // Enabled Traits when passed a TraitConfiguration: + + // When passed .disableAllTraits configuration + XCTAssertThrowsError(try manifest.enabledTraits(using: .disableAllTraits)) { error in + XCTAssertEqual("\(error)", """ + Disabled default traits on package 'Foo' that declares no traits. This is prohibited to allow packages to adopt traits initially without causing an API break. + """) + } + + // When passed .enableAllTraits configuration + XCTAssertThrowsError(try manifest.enabledTraits(using: .enabledTraits(["Trait1"]))) { error in + XCTAssertEqual("\(error)", """ + Traits [Trait1] have been enabled on package 'Foo' that declares no traits. + """) + } + + XCTAssertNoThrow(try manifest.enabledTraits(using: .enableAllTraits)) + XCTAssertNoThrow(try manifest.enabledTraits(using: .none)) + + // Enabled Traits when passed explicitly enabled traits list: + + // If given a parent package, and the enabled traits being passed don't exist: + XCTAssertThrowsError(try manifest.enabledTraits(using: ["Trait1"], "Qux")) { error in + XCTAssertEqual("\(error)", """ + Package 'Qux' enables traits [Trait1] on package 'Foo' that declares no traits. + """) + } + + // If given a parent package, and the default traits are disabled: + XCTAssertThrowsError(try manifest.enabledTraits(using: [], "Qux")) { error in + XCTAssertEqual("\(error)", """ + Disabled default traits by package 'Qux' on package 'Foo' that declares no traits. This is prohibited to allow packages to adopt traits initially without causing an API break. + """) + } + } + } + + func testIsTraitEnabled_WhenNoDefaultTraitsAndNoConfig() throws { let dependencies: [PackageDependency] = [ .localSourceControl(path: "/Baz", requirement: .upToNextMajor(from: "1.0.0")), .localSourceControl(path: "/Buzz", requirement: .upToNextMajor(from: "1.0.0")), @@ -292,7 +429,7 @@ class ManifestTests: XCTestCase { } } - func testEnabledTraits_WhenDefaultTraitsAndNoTraitConfig() throws { + func testIsTraitEnabled_WhenDefaultTraitsAndNoTraitConfig() throws { let products = try [ ProductDescription(name: "Foo", type: .library(.automatic), targets: ["Foo"]), ProductDescription(name: "Bar", type: .library(.automatic), targets: ["Bar"]), @@ -328,7 +465,52 @@ class ManifestTests: XCTestCase { } } - func testCalculateAllEnabledTraits_WithOnlyDefaultTraitsEnabled() throws { + func testEnabledTraits_WithAllTraitsDisabled() throws { + let products = try [ + ProductDescription(name: "Foo", type: .library(.automatic), targets: ["Foo"]), + ProductDescription(name: "Bar", type: .library(.automatic), targets: ["Bar"]), + ] + + let targets = try [ + TargetDescription(name: "Foo", dependencies: ["Bar"]), + TargetDescription(name: "Bar", dependencies: ["Baz"]), + TargetDescription(name: "Baz", dependencies: ["MyPlugin"]), + TargetDescription(name: "FooBar", dependencies: []), + TargetDescription(name: "MyPlugin", type: .plugin, pluginCapability: .buildTool), + ] + + let dependencies: [PackageDependency] = [ + .localSourceControl(path: "/Bar", requirement: .upToNextMajor(from: "1.0.0")), + .localSourceControl(path: "/Baz", requirement: .upToNextMajor(from: "1.0.0")), + .localSourceControl(path: "/MyPlugin", requirement: .upToNextMajor(from: "1.0.0")), + ] + + let traits: Set = [ + TraitDescription(name: "Trait1", enabledTraits: ["Trait2"]), + TraitDescription(name: "Trait2"), + TraitDescription(name: "Trait3") + ] + + do { + let manifest = Manifest.createRootManifest( + displayName: "Foo", + path: "/Foo", + toolsVersion: .v5_2, + dependencies: dependencies, + products: products, + targets: targets, + traits: traits, + pruneDependencies: true // Since all dependencies are used, this shouldn't affect the outcome. + ) + + XCTAssertNoThrow(try { + let enabledTraits = try XCTUnwrap(manifest.enabledTraits(using: .disableAllTraits)) + XCTAssertEqual(enabledTraits, []) + }()) + } + } + + func testEnabledTraits_WithOnlyDefaultTraitsEnabled() throws { let products = try [ ProductDescription(name: "Foo", type: .library(.automatic), targets: ["Foo"]), ] @@ -358,12 +540,12 @@ class ManifestTests: XCTestCase { // Calculate the enabled traits without an explicitly declared set of enabled traits. // This should default to fetching the default traits, if they exist (which in this test case // they do), and then will calculate the transitive set of traits that are enabled. - let allEnabledTraits = try XCTUnwrap(manifest.enabledTraits(using: .default)).sorted() + let allEnabledTraits = try XCTUnwrap(manifest.enabledTraits(using: .none)).sorted() XCTAssertEqual(allEnabledTraits, ["Trait1", "Trait2"]) } } - func testCalculateAllEnabledTraits_WithExplicitTraitsEnabled() throws { + func testEnabledTraits_WithExplicitTraitsEnabled() throws { let products = try [ ProductDescription(name: "Foo", type: .library(.automatic), targets: ["Foo"]), ] @@ -403,7 +585,7 @@ class ManifestTests: XCTestCase { } } - func testCalculateAllEnabledTraits_WithAllTraitsEnabled() throws { + func testEnabledTraits_WithAllTraitsEnabled() throws { let products = try [ ProductDescription(name: "Foo", type: .library(.automatic), targets: ["Foo"]), ] diff --git a/Tests/WorkspaceTests/WorkspaceTests.swift b/Tests/WorkspaceTests/WorkspaceTests.swift index bbf019445be..99b61947b3e 100644 --- a/Tests/WorkspaceTests/WorkspaceTests.swift +++ b/Tests/WorkspaceTests/WorkspaceTests.swift @@ -15844,6 +15844,127 @@ final class WorkspaceTests: XCTestCase { } } + func testInvalidTrait_WhenParentPackageEnablesTraits() async throws { + let sandbox = AbsolutePath("/tmp/ws/") + let fs = InMemoryFileSystem() + + let workspace = try await MockWorkspace( + sandbox: sandbox, + fileSystem: fs, + roots: [ + MockPackage( + name: "Foo", + targets: [ + MockTarget( + name: "Foo", + dependencies: [ + .product( + name: "Baz", + package: "Baz", + condition: .init(traits: ["Trait1"]) + ), + ] + ), + MockTarget(name: "Bar", dependencies: ["Baz"]), + ], + products: [ + MockProduct(name: "Foo", modules: ["Foo", "Bar"]), + ], + dependencies: [ + .sourceControl(path: "./Baz", requirement: .upToNextMajor(from: "1.0.0"), traits: ["TraitNotFound"]), + ], + traits: [.init(name: "default", enabledTraits: ["Trait2"]), "Trait1", "Trait2"] + ), + ], + packages: [ + MockPackage( + name: "Baz", + targets: [ + MockTarget(name: "Baz"), + ], + products: [ + MockProduct(name: "Baz", modules: ["Baz"]), + ], + traits: ["TraitFound"], + versions: ["1.0.0", "1.5.0"] + ), + ] + ) + + let deps: [MockDependency] = [ + .sourceControl(path: "./Baz", requirement: .exact("1.0.0"), products: .specific(["Baz"]), traits: ["TraitFound"]), + ] + + try await workspace.checkPackageGraphFailure(roots: ["Foo"], deps: deps) { diagnostics in + testDiagnostics(diagnostics) { result in + result.check(diagnostic: .equal("Trait 'TraitNotFound' enabled by parent package 'foo' is not declared by package 'Baz'. The available traits declared by this package are: TraitFound."), severity: .error) + } + } + await workspace.checkManagedDependencies { result in + result.check(dependency: "baz", at: .checkout(.version("1.0.0"))) + } + } + + func testInvalidTraitConfiguration_ForRootPackage() async throws { + let sandbox = AbsolutePath("/tmp/ws/") + let fs = InMemoryFileSystem() + + let workspace = try await MockWorkspace( + sandbox: sandbox, + fileSystem: fs, + roots: [ + MockPackage( + name: "Foo", + targets: [ + MockTarget( + name: "Foo", + dependencies: [ + .product( + name: "Baz", + package: "Baz", + condition: .init(traits: ["Trait1"]) + ), + ] + ), + MockTarget(name: "Bar", dependencies: ["Baz"]), + ], + products: [ + MockProduct(name: "Foo", modules: ["Foo", "Bar"]), + ], + dependencies: [ + .sourceControl(path: "./Baz", requirement: .upToNextMajor(from: "1.0.0"), traits: ["TraitFound"]), + ], + traits: [.init(name: "default", enabledTraits: ["Trait2"]), "Trait1", "Trait2"] + ), + ], + packages: [ + MockPackage( + name: "Baz", + targets: [ + MockTarget(name: "Baz"), + ], + products: [ + MockProduct(name: "Baz", modules: ["Baz"]), + ], + traits: ["TraitFound"], + versions: ["1.0.0", "1.5.0"] + ), + ], + // Trait configuration containing trait that isn't defined in the root package. + traitConfiguration: .enabledTraits(["TraitNotFound"]), + ) + + let deps: [MockDependency] = [ + .sourceControl(path: "./Baz", requirement: .exact("1.0.0"), products: .specific(["Baz"]), traits: ["TraitFound"]), + ] + + try await workspace.checkPackageGraphFailure(roots: ["Foo"], deps: deps) { diagnostics in + testDiagnostics(diagnostics) { result in + result.check(diagnostic: .equal("Trait 'TraitNotFound' is not declared by package 'Foo'. The available traits declared by this package are: Trait1, Trait2, default."), severity: .error) + } + } + } + func makeRegistryClient( packageIdentity: PackageIdentity, packageVersion: Version, From 0536c5254a0e411bf3b483c839f05d1f63d23f31 Mon Sep 17 00:00:00 2001 From: Bri Peticca Date: Wed, 9 Apr 2025 15:43:17 -0400 Subject: [PATCH 8/8] Remove comments and ensure enabled traits applies to root deps - Cleaned up commented code - Assure that enabledTraits on the PackageGraphRoot also considers the root's dependencies enabled traits, specified by the root itself --- Sources/PackageGraph/PackageGraphRoot.swift | 8 ++++ Sources/PackageModel/Manifest/Manifest.swift | 12 +---- Sources/Workspace/Workspace+Manifests.swift | 46 ++------------------ Tests/FunctionalTests/TraitTests.swift | 3 +- 4 files changed, 14 insertions(+), 55 deletions(-) diff --git a/Sources/PackageGraph/PackageGraphRoot.swift b/Sources/PackageGraph/PackageGraphRoot.swift index efccb13dca6..8091ba65656 100644 --- a/Sources/PackageGraph/PackageGraphRoot.swift +++ b/Sources/PackageGraph/PackageGraphRoot.swift @@ -113,6 +113,14 @@ public struct PackageGraphRoot { // Should only ever have to use trait configuration here for roots. let enabledTraits = try manifest.enabledTraits(using: traitConfiguration) traitsMap[package.key] = enabledTraits + + // Calculate the enabled traits for each dependency of this root: + manifest.dependencies.forEach { dependency in + if let traits = dependency.traits { + let traitNames = traits.map(\.name) + traitsMap[dependency.identity, default: []].formUnion(Set(traitNames)) + } + } } self.enabledTraits = enableTraitsMap diff --git a/Sources/PackageModel/Manifest/Manifest.swift b/Sources/PackageModel/Manifest/Manifest.swift index 1b74f0dcfb8..798a4f6543b 100644 --- a/Sources/PackageModel/Manifest/Manifest.swift +++ b/Sources/PackageModel/Manifest/Manifest.swift @@ -256,17 +256,7 @@ public final class Manifest: Sendable { } } -// target.pluginUsages?.forEach { -// guard let dependency = self.packageDependency(referencedBy: $0), -// let guardingTraits = traitGuardedDeps[dependency.identity.description]?[target.name] -// else { -// return -// } -// if let enabledTraits, -// guardingTraits.intersection(enabledTraits) != guardingTraits { -// guardedDependencies.insert(dependency.identity) -// } -// } + // Since plugins cannot specify traits as a guarding condition, we can skip them. } let dependencies = self.dependencies.filter { guardedDependencies.contains($0.identity) } diff --git a/Sources/Workspace/Workspace+Manifests.swift b/Sources/Workspace/Workspace+Manifests.swift index 9600ba06545..1eaaccba606 100644 --- a/Sources/Workspace/Workspace+Manifests.swift +++ b/Sources/Workspace/Workspace+Manifests.swift @@ -226,10 +226,8 @@ extension Workspace { let inputNodes: [GraphLoadingNode] = try root.packages.map { identity, package in inputIdentities.append(package.reference) - var traits: Set? = [] - if let enabledTraits = rootEnabledTraitsMap[package.reference.identity] { - traits = enabledTraits//try package.manifest.enabledTraits(using: enabledTraits) - } + var traits: Set? = rootEnabledTraitsMap[package.reference.identity] ?? [] + let node = try GraphLoadingNode( identity: identity, manifest: package.manifest, @@ -241,12 +239,8 @@ extension Workspace { let package = dependency.packageRef inputIdentities.append(package) return try manifestsMap[dependency.identity].map { manifest in - var traits: Set? = [] + var traits: Set? = rootDependenciesEnabledTraitsMap[dependency.identity] ?? [] - if let enabledTraits = rootDependenciesEnabledTraitsMap[dependency.identity] { - // Recursively calculate the enabled traits of this package. - traits = enabledTraits//try manifest.enabledTraits(using: enabledTraits) - } return try GraphLoadingNode( identity: dependency.identity, manifest: manifest, @@ -347,7 +341,6 @@ extension Workspace { var allEnabledTraits: Set = [] if let explicitlyEnabledTraits - // let calculatedTraits = try manifest.enabledTraits(using: Set(explicitlyEnabledTraits)) { allEnabledTraits = Set(explicitlyEnabledTraits) } @@ -636,38 +629,7 @@ extension Workspace { let firstLevelDependencies = try topLevelManifests.values.map { manifest in try manifest.dependencies.filter { dep in guard configuration.pruneDependencies else { return true } - var enabledTraits: Set? = [] - if manifest.packageKind.isRoot { - enabledTraits = root.enabledTraits[manifest.packageIdentity] - } -// else { -// let rootManifests = root.manifests.values.filter(\.packageKind.isRoot) -// -// // find the package dependency in each of the root manifests -// let packageDependencyInRoots = rootManifests -// .compactMap { -// $0.dependencies -// .first(where: { $0.identity.description == manifest.displayName.lowercased() }) -// } -// -// // pluck out the enabled traits defined by the package dependency struct -// let enabledTraitsPerPackageDep = packageDependencyInRoots.map(\.traits) -// -// // create a union of the sets; if all are nil, then there is no config -// var manifestEnabledTraits: Set? -// for enabledTraits in enabledTraitsPerPackageDep { -// if let enabledTraits = enabledTraits?.map(\.name) { -// if let resultSet = manifestEnabledTraits { -// manifestEnabledTraits = resultSet.union(Set(enabledTraits)) -// } else { -// manifestEnabledTraits = Set(enabledTraits) -// } -// } -// } -//// manifestEnabledTraits = try manifest.enabledTraits(using: manifestEnabledTraits) -//// config = .init(enabledTraits: manifestEnabledTraits) -// -// } + var enabledTraits: Set? = root.enabledTraits[manifest.packageIdentity] let isDepUsed = try manifest.isPackageDependencyUsed(dep, enabledTraits: enabledTraits) return isDepUsed }.map(\.packageRef) diff --git a/Tests/FunctionalTests/TraitTests.swift b/Tests/FunctionalTests/TraitTests.swift index 9c745484358..51f890721c3 100644 --- a/Tests/FunctionalTests/TraitTests.swift +++ b/Tests/FunctionalTests/TraitTests.swift @@ -318,9 +318,8 @@ final class TraitTests: XCTestCase { await XCTAssertAsyncThrowsError(try await executeSwiftRun( fixturePath.appending("DisablingEmptyDefaultsExample"), "DisablingEmptyDefaultsExample" - //extraArgs: ["--experimental-prune-unused-dependencies"] )) { error in - guard case SwiftPMError.executionFailure(let underlying, let stdout, let stderr) = error else { + guard case SwiftPMError.executionFailure(_, _, let stderr) = error else { XCTFail() return }