-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[WIP] Fix for required dependencies calculation with traits #9269
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
bripeticca
wants to merge
12
commits into
swiftlang:main
Choose a base branch
from
bripeticca:traits/requireddeps
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+3,740
−2,107
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The enabled traits property was essentially being erased to defaults rather than propagating what was calculated earlier due to how we were returning the DependencyResolutionNode during the pub grub stage. This fix should now assure that we are passing the appropriate enabled traits to these nodes, and are being considered when making calls to PackageContainer's dependency-related methods.
* an EnabledTrait model has been created to store additional data about an enabled trait, namely the origins of its enablement. * EnabledTraits is a wrapper for a Set<EnabledTrait> that handles the special edge cases when dealing with traits. * move traits-related tests in the WorkspaceTests to their own file
* Add new file to PackageModelTests/ for EnabledTrait tests - Tests EnabledTrait, EnabledTraits, and EnabledTraitsMap - Extensions to EnabledTraits added as helpers for testing * Cleanup some TODOs; added some other prints to help debug, will remove once ready for review + tests passing
|
@swift-ci please test |
|
@swift-ci please test |
|
@swift-ci please test |
* Augment EnabledTraits and EnabledTraitsMap to consider the case where a package's default traits are disabled through some parent configuration. * Update methods in Manifest+Traits.swift * Add tests for disable default traits behaviours in EnabledTraitTests.swift and WorkspaceTests+Traits.swift.
|
@swift-ci please test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces the
EnabledTraitmodel and associated infrastructure to properly track and manage trait enablement throughout the dependency resolution process. The new system replaces simple string-based trait tracking with a rich model that maintains provenance information about how and why each trait was enabled.Core Changes
New
EnabledTraitModel (EnabledTrait.swift)Introduces three main components for tracking enabled traits for packages:
EnabledTrait - Represents an individual enabled trait with:
Settervalues tracking how the trait was enabled (via command-line config, parent package, or another trait)EnabledTraitfor a specific package, and that many packages can define their own (unique) trait that shares a name with a trait from another package but are treated as fundamentally different instances.EnabledTraits - Collection wrapper around
IdentifiableSet<EnabledTrait>that:Settersfor traits of the same name)EnabledTraitsMap - Thread-safe map storing enabled traits per package that:
PackageIdentitytoEnabledTraits["default"]for packages with no explicitly enabled traitsThe introduction of these models have also provided some API conveniences:
String Interoperability -
EnabledTraitandEnabledTraitsprovide seamless string integration:ExpressibleByStringLiteralandExpressibleByArrayLiteralfor natural initialization of enabled traits and sets of enabled traits:let enabledTrait: EnabledTrait = "foo"orlet enabledTraits: EnabledTraits = ["foo", "bar"]enabledTrait == "foo"and"foo" == enabledTraitboth workEnabledTraitConvertibleprotocol allows APIs to accept string collections and auto-convert them toEnabledTraitCollection Protocol Support -
EnabledTraitsconforms toCollectionwith set-like operations (union, intersection, contains) and overloaded methods that accept eitherCollection<String>orCollection<EnabledTrait>, enabling flexible APIs that work with both typesRefactored Trait Handling (
Manifest+Traits.swift)Set<String>trait representations with richerEnabledTraitsmodelEnabledTraitinstead of raw stringsEnabledTrait.Setter)Workspace+Traits.swift(new file)updateEnabledTraits(for:)method determines enabled traits for loaded manifestsupdateEnabledTraits(forDependency:_:)handles dependency-specific trait propagationBug Fixes
Required Dependencies Calculation
Problem: During PubGrub dependency resolution, enabled traits were being reset to defaults instead of propagating previously calculated values when creating
DependencyResolutionNodeinstances, which relied on an accurate set ofenabledTraits.Impact: Wrong trait sets were considered when evaluating required dependencies, leading to incorrect dependency graphs.
Solution: Properly propagate enabled traits through the
nodes()method forPackageContainerConstraint, which creates instances ofDependencyResolutionNodefrom these constraints during PubGrub dependency resolution.IdentifiableSet Intersection
Problem: The intersection method created an empty set instead of starting with self, causing loss of element data.
Solution: Changed
var result = Self()tovar result = selfinIdentifiableSet.swift:86, ensuring proper preservation of element data during intersections.Test Improvements
ModulesGraphTests+Traits.swiftWorkspaceTests+Traits.swiftEnabledTraitTests.swiftAdditional Improvements
IdentifiableSetwithremove(id:)andremove(_:)methodsWorkspace+Traits.swiftfor trait-specific workspace operations