diff --git a/Sources/PackageGraph/PackageContainer.swift b/Sources/PackageGraph/PackageContainer.swift index f1029ad3d5f..59e7a22e569 100644 --- a/Sources/PackageGraph/PackageContainer.swift +++ b/Sources/PackageGraph/PackageContainer.swift @@ -97,6 +97,10 @@ public protocol PackageContainer { /// after the container is available. The updated identifier is returned in result of the /// dependency resolution. func loadPackageReference(at boundVersion: BoundVersion) async throws -> PackageReference + + /// Get the updated set of traits at a bound version. + /// + func loadPackageTraits(at boundVersion: BoundVersion) async throws -> Set } extension PackageContainer { diff --git a/Sources/PackageGraph/PackageModel+Extensions.swift b/Sources/PackageGraph/PackageModel+Extensions.swift index 6c4084e49e0..9163b1ca7fb 100644 --- a/Sources/PackageGraph/PackageModel+Extensions.swift +++ b/Sources/PackageGraph/PackageModel+Extensions.swift @@ -35,7 +35,10 @@ extension PackageDependency { extension Manifest { /// Constructs constraints of the dependencies in the raw package. - public func dependencyConstraints(productFilter: ProductFilter, _ enabledTraits: EnabledTraits = ["default"]) throws -> [PackageContainerConstraint] { + public func dependencyConstraints( + productFilter: ProductFilter, + _ enabledTraits: EnabledTraits = ["default"] + ) throws -> [PackageContainerConstraint] { return try self.dependenciesRequired(for: productFilter, enabledTraits).map({ let explicitlyEnabledTraits = $0.traits?.filter { guard let condition = $0.condition else { return true } diff --git a/Sources/PackageGraph/Resolution/DependencyResolverBinding.swift b/Sources/PackageGraph/Resolution/DependencyResolverBinding.swift index 64af5af37a7..6d23c653987 100644 --- a/Sources/PackageGraph/Resolution/DependencyResolverBinding.swift +++ b/Sources/PackageGraph/Resolution/DependencyResolverBinding.swift @@ -17,4 +17,5 @@ public struct DependencyResolverBinding { public let package: PackageReference public let boundVersion: BoundVersion public let products: ProductFilter + public let traits: Set } diff --git a/Sources/PackageGraph/Resolution/PubGrub/PubGrubDependencyResolver.swift b/Sources/PackageGraph/Resolution/PubGrub/PubGrubDependencyResolver.swift index 762795b3a32..2e4a9811da0 100644 --- a/Sources/PackageGraph/Resolution/PubGrub/PubGrubDependencyResolver.swift +++ b/Sources/PackageGraph/Resolution/PubGrub/PubGrubDependencyResolver.swift @@ -240,12 +240,11 @@ public struct PubGrubDependencyResolver { try await self.run(state: state) let decisions = state.solution.assignments.filter(\.isDecision) - var flattenedAssignments: [PackageReference: (binding: BoundVersion, products: ProductFilter)] = [:] + var flattenedAssignments: [PackageReference: (binding: BoundVersion, products: ProductFilter, traits: Set)] = [:] for assignment in decisions { if assignment.term.node == state.root { continue } - let boundVersion: BoundVersion switch assignment.term.requirement { case .exact(let version): @@ -264,6 +263,7 @@ public struct PubGrubDependencyResolver { ) } let updatePackage = try await container.underlying.loadPackageReference(at: boundVersion) + let updatedTraits = try await container.underlying.loadPackageTraits(at: boundVersion).map(\.name) if var existing = flattenedAssignments[updatePackage] { guard existing.binding == boundVersion else { @@ -272,13 +272,22 @@ public struct PubGrubDependencyResolver { existing.products.formUnion(products) flattenedAssignments[updatePackage] = existing } else { - flattenedAssignments[updatePackage] = (binding: boundVersion, products: products) + flattenedAssignments[updatePackage] = ( + binding: boundVersion, + products: products, + traits: Set(updatedTraits) + ) } } var finalAssignments: [DependencyResolverBinding] = flattenedAssignments.keys.sorted(by: { $0.deprecatedName < $1.deprecatedName }).map { package in let details = flattenedAssignments[package]! - return .init(package: package, boundVersion: details.binding, products: details.products) + return .init( + package: package, + boundVersion: details.binding, + products: details.products, + traits: details.traits + ) } // Add overridden packages to the result. @@ -289,10 +298,12 @@ public struct PubGrubDependencyResolver { }) } let updatePackage = try await container.underlying.loadPackageReference(at: override.version) + let updatedTraits = try await container.underlying.loadPackageTraits(at: override.version).map(\.name) finalAssignments.append(.init( package: updatePackage, boundVersion: override.version, - products: override.products + products: override.products, + traits: Set(updatedTraits) )) } diff --git a/Sources/PackageLoading/ManifestLoader+Validation.swift b/Sources/PackageLoading/ManifestLoader+Validation.swift index cecc121d5bf..e2d1d076ccd 100644 --- a/Sources/PackageLoading/ManifestLoader+Validation.swift +++ b/Sources/PackageLoading/ManifestLoader+Validation.swift @@ -95,6 +95,7 @@ public struct ManifestValidator { } private func validateTraits() -> [Basics.Diagnostic] { + // TODO bp add trait validation here. var diagnostics = [Basics.Diagnostic]() if self.manifest.traits.count > 300 { diff --git a/Sources/PackageModel/Manifest/Manifest+Traits.swift b/Sources/PackageModel/Manifest/Manifest+Traits.swift index 5e772783b30..89b5d2f1eae 100644 --- a/Sources/PackageModel/Manifest/Manifest+Traits.swift +++ b/Sources/PackageModel/Manifest/Manifest+Traits.swift @@ -237,9 +237,9 @@ extension Manifest { /// Determines if a trait is enabled with a given set of enabled traits. public func isTraitEnabled(_ trait: TraitDescription, _ enabledTraits: EnabledTraits) throws -> Bool { // First, check that the queried trait is valid. - try validateTrait(trait) +// try validateTrait(trait) // Then, check that the list of enabled traits is valid. - try validateEnabledTraits(enabledTraits) +// try validateEnabledTraits(enabledTraits) // Special case for dealing with whether a default trait is enabled. guard !trait.isDefault else { @@ -271,7 +271,7 @@ 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. private func calculateAllEnabledTraits(explicitlyEnabledTraits: EnabledTraits) throws -> EnabledTraits { - try validateEnabledTraits(explicitlyEnabledTraits) +// try validateEnabledTraits(explicitlyEnabledTraits) // This the point where we flatten the enabled traits and resolve the recursive traits var enabledTraits = explicitlyEnabledTraits let areDefaultsEnabled = enabledTraits.remove("default") != nil @@ -296,10 +296,13 @@ extension Manifest { // we have to check if default traits should be used and then flatten all the enabled traits. let transitivelyEnabledTraits = try enabledTraits.flatMap { trait in guard let traitDescription = traitsMap[trait.name] else { - throw TraitError.invalidTrait( - package: .init(self), - trait: trait - ) + // TODO bp do somehting more meaningful here. + // testing for now. + return .init([], setBy: .trait("")) +// throw TraitError.invalidTrait( +// package: .init(self), +// trait: trait +// ) } return EnabledTraits( traitDescription.enabledTraits, @@ -447,7 +450,8 @@ extension Manifest { /// Given a set of enabled traits, determine whether a package dependecy of this manifest is /// guarded by traits. private func isPackageDependencyTraitGuarded(_ dependency: PackageDependency, enabledTraits: EnabledTraits) throws -> Bool { - try validateEnabledTraits(enabledTraits) + // TODO bp try doing trait validation after resolution to ensure that updated dependencies (which implicitly means a possible update to trait configurations in package manifests) are accounted for here. +// try validateEnabledTraits(enabledTraits) let targetDependenciesForPackageDependency = self.targets.flatMap({ $0.dependencies }) .filter({ diff --git a/Sources/Workspace/PackageContainer/FileSystemPackageContainer.swift b/Sources/Workspace/PackageContainer/FileSystemPackageContainer.swift index 4bbf8333ddc..b1f7a1569b9 100644 --- a/Sources/Workspace/PackageContainer/FileSystemPackageContainer.swift +++ b/Sources/Workspace/PackageContainer/FileSystemPackageContainer.swift @@ -105,6 +105,12 @@ public struct FileSystemPackageContainer: PackageContainer { return package.withName(manifest.displayName) } + public func loadPackageTraits(at boundVersion: BoundVersion) async throws -> Set { + assert(boundVersion == .unversioned, "Unexpected bound version \(boundVersion)") + let manifest = try await loadManifest() + return manifest.traits + } + public func isToolsVersionCompatible(at version: Version) -> Bool { fatalError("This should never be called") } diff --git a/Sources/Workspace/PackageContainer/RegistryPackageContainer.swift b/Sources/Workspace/PackageContainer/RegistryPackageContainer.swift index 7b42c5c6907..2407bb6b54e 100644 --- a/Sources/Workspace/PackageContainer/RegistryPackageContainer.swift +++ b/Sources/Workspace/PackageContainer/RegistryPackageContainer.swift @@ -121,6 +121,14 @@ public class RegistryPackageContainer: PackageContainer { return self.package } + public func loadPackageTraits(at boundVersion: BoundVersion) async throws -> Set { + guard case .version(let version) = boundVersion else { + throw InternalError("loadPackageTraits expects an exact version") + } + let manifest = try await self.loadManifest(version: version) + return manifest.traits + } + // marked internal for testing internal func loadManifest(version: Version) async throws -> Manifest { let result = try await self.getAvailableManifestsFilesystem(version: version) diff --git a/Sources/Workspace/PackageContainer/SourceControlPackageContainer.swift b/Sources/Workspace/PackageContainer/SourceControlPackageContainer.swift index 70b8affa0a9..d3625056d50 100644 --- a/Sources/Workspace/PackageContainer/SourceControlPackageContainer.swift +++ b/Sources/Workspace/PackageContainer/SourceControlPackageContainer.swift @@ -366,6 +366,27 @@ internal final class SourceControlPackageContainer: PackageContainer, CustomStri return self.package.withName(manifest.displayName) } + public func loadPackageTraits(at boundVersion: BoundVersion) async throws -> Set { + let revision: Revision + var version: Version? + switch boundVersion { + case .version(let v): + guard let tag = try self.knownVersions()[v] else { + throw StringError("unknown tag \(v)") + } + version = v + revision = try repository.resolveRevision(tag: tag) + case .revision(let identifier, _): + revision = try repository.resolveRevision(identifier: identifier) + case .unversioned, .excluded: + assertionFailure("Unexpected type requirement \(boundVersion)") + return [] + } + + let manifest = try await self.loadManifest(at: revision, version: version) + return manifest.traits + } + /// Returns true if the tools version is valid and can be used by this /// version of the package manager. private func isValidToolsVersion(_ toolsVersion: ToolsVersion) -> Bool { diff --git a/Sources/Workspace/ResolverPrecomputationProvider.swift b/Sources/Workspace/ResolverPrecomputationProvider.swift index b6ad1ec31ed..a9bdeef595c 100644 --- a/Sources/Workspace/ResolverPrecomputationProvider.swift +++ b/Sources/Workspace/ResolverPrecomputationProvider.swift @@ -172,4 +172,8 @@ private struct LocalPackageContainer: PackageContainer { return .root(identity: self.package.identity, path: self.manifest.path) } } + + func loadPackageTraits(at boundVersion: BoundVersion) async throws -> Set { + return manifest.traits + } } diff --git a/Sources/Workspace/Workspace+Dependencies.swift b/Sources/Workspace/Workspace+Dependencies.swift index 743e6ac926e..221cebce028 100644 --- a/Sources/Workspace/Workspace+Dependencies.swift +++ b/Sources/Workspace/Workspace+Dependencies.swift @@ -196,6 +196,13 @@ extension Workspace { observabilityScope: observabilityScope ) + // Update traits; validation check. + try await self.updateTraits( + manifests: updatedDependencyManifests, + addedOrUpdatedPackages: addedOrUpdatedPackages, + observabilityScope: observabilityScope + ) + return packageStateChanges } @@ -479,6 +486,13 @@ extension Workspace { observabilityScope: observabilityScope ) + // Update traits; validation check + try await self.updateTraits( + manifests: currentManifests, + addedOrUpdatedPackages: [], + observabilityScope: observabilityScope + ) + let precomputationResult = try await self.precomputeResolution( root: graphRoot, dependencyManifests: currentManifests, @@ -586,6 +600,12 @@ extension Workspace { observabilityScope: observabilityScope ) + try await self.updateTraits( + manifests: currentManifests, + addedOrUpdatedPackages: [], + observabilityScope: observabilityScope + ) + return currentManifests case .required(let reason): delegate?.willResolveDependencies(reason: reason) @@ -608,6 +628,8 @@ extension Workspace { observabilityScope: observabilityScope ) + // TODO bp; some possible trait validation here. + // Reset the active resolver. self.activeResolver = nil @@ -615,6 +637,9 @@ extension Workspace { return currentManifests } + // TODO bp: during an update to dependencies checkout, must + // also update/validate the traits here if previous versions of dependencies have been changed. + // Update the checkouts with dependency resolution result. let packageStateChanges = await self.updateDependenciesCheckouts( root: graphRoot, @@ -661,6 +686,13 @@ extension Workspace { observabilityScope: observabilityScope ) + // Update traits; validation check. + try await self.updateTraits( + manifests: updatedDependencyManifests, + addedOrUpdatedPackages: addedOrUpdatedPackages, + observabilityScope: observabilityScope + ) + return updatedDependencyManifests } @@ -907,7 +939,9 @@ extension Workspace { } guard let requiredDependencies = observabilityScope - .trap({ try dependencyManifests.requiredPackages.filter(\.kind.isResolvable) }) + .trap({ try + // TODO bp; don't do preemptive trait validation at this step; could be loading from cache re: old trait version + dependencyManifests.requiredPackages.filter(\.kind.isResolvable) }) else { return nil } diff --git a/Sources/Workspace/Workspace+Manifests.swift b/Sources/Workspace/Workspace+Manifests.swift index 5c6093268e0..53207c6901e 100644 --- a/Sources/Workspace/Workspace+Manifests.swift +++ b/Sources/Workspace/Workspace+Manifests.swift @@ -618,6 +618,7 @@ extension Workspace { >] = { node in // optimization: preload manifest we know about in parallel // avoid loading dependencies that are trait-guarded here since this is redundant. + // todo bp note that traits could have changed here if the package dependency is slated for an update and the user references the new trait but we still have an old checkout. avoid validating traits as a result, and wait until we have updated manifests after resolution. let dependenciesRequired = try node.item.manifest.dependenciesRequired( for: node.item.productFilter, node.item.enabledTraits @@ -681,7 +682,7 @@ extension Workspace { // before the dependency manifest is loaded and its default traits are known. let allManifests = allNodes.mapValues(\.manifest) for (_, manifest) in allManifests { - try updateEnabledTraits(for: manifest) + try await updateEnabledTraits(for: manifest, observabilityScope: observabilityScope) } let dependencyManifests = allNodes.filter { !$0.value.manifest.packageKind.isRoot } @@ -891,7 +892,7 @@ extension Workspace { } // Upon loading a new manifest, update enabled traits. - try self.updateEnabledTraits(for: manifest) + try await self.updateEnabledTraits(for: manifest, observabilityScope: observabilityScope) self.delegate?.didLoadManifest( packageIdentity: packageIdentity, diff --git a/Sources/Workspace/Workspace+Traits.swift b/Sources/Workspace/Workspace+Traits.swift index 20ed1b5e5cc..4d23499c19e 100644 --- a/Sources/Workspace/Workspace+Traits.swift +++ b/Sources/Workspace/Workspace+Traits.swift @@ -12,10 +12,12 @@ import class PackageModel.Manifest import struct PackageModel.PackageIdentity +import struct PackageModel.PackageReference import enum PackageModel.ProductFilter import enum PackageModel.PackageDependency import struct PackageModel.EnabledTrait import struct PackageModel.EnabledTraits +import class Basics.ObservabilityScope extension Workspace { /// Given a loaded `Manifest`, determine the traits that are enabled for it and @@ -24,7 +26,7 @@ extension Workspace { /// /// If the package defines a dependency with an explicit set of enabled traits, it will also /// add them to the enabled traits map. - public func updateEnabledTraits(for manifest: Manifest) throws { + public func updateEnabledTraits(for manifest: Manifest, observabilityScope: ObservabilityScope) async throws { // If the `Manifest` is a root, then we should default to using // the trait configuration set in the `Workspace`. Otherwise, // check the enabled traits map to see if there are traits @@ -92,3 +94,18 @@ extension Workspace { } } } + +extension Workspace { + internal func updateTraits( + manifests: DependencyManifests, + addedOrUpdatedPackages: [PackageReference], + observabilityScope: ObservabilityScope + ) async throws { + // TODO bp complete implementation + for manifest in addedOrUpdatedPackages { + // do validation check on the traits here. + // take in the enabled traits map; use validation methods + // available on the manifest, referenced from the DependencyManifests. + } + } +} diff --git a/Sources/Workspace/Workspace.swift b/Sources/Workspace/Workspace.swift index 76054b6988d..58d1d3c5b90 100644 --- a/Sources/Workspace/Workspace.swift +++ b/Sources/Workspace/Workspace.swift @@ -40,6 +40,9 @@ public enum WorkspaceResolveReason: Equatable { /// Requirements were added for new packages. case newPackages(packages: [PackageReference]) + // TODO bp search for instances of this to determine whether a package + // needs to be updated, and therefore possibly has a change in its + // traits. /// The requirement of a dependency has changed. case packageRequirementChange( package: PackageReference, diff --git a/Sources/_InternalTestSupport/MockPackageContainer.swift b/Sources/_InternalTestSupport/MockPackageContainer.swift index e0c87d3abb5..5e1e81a4bc6 100644 --- a/Sources/_InternalTestSupport/MockPackageContainer.swift +++ b/Sources/_InternalTestSupport/MockPackageContainer.swift @@ -71,6 +71,11 @@ public class MockPackageContainer: CustomPackageContainer { return self.package } + public func loadPackageTraits(at boundVersion: BoundVersion) async throws -> Set { + // TODO bp complete implementation + return [] + } + public func isToolsVersionCompatible(at version: Version) -> Bool { return true } diff --git a/Tests/PackageGraphTests/PubGrubTests.swift b/Tests/PackageGraphTests/PubGrubTests.swift index 7b0c2a13fba..144425643ee 100644 --- a/Tests/PackageGraphTests/PubGrubTests.swift +++ b/Tests/PackageGraphTests/PubGrubTests.swift @@ -3198,6 +3198,11 @@ public class MockContainer: PackageContainer { return self.package } + public func loadPackageTraits(at boundVersion: BoundVersion) async throws -> Set { + // TODO bp complete implementation + return [] + } + func appendVersion(_ version: BoundVersion) { self._versions.append(version) self._versions = self._versions