Assure trait-guarded dependencies are not included in resolution; precompute enabled traits before resolution #8852
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
|
@swift-ci please test |
- Create a 'precomputeTraits' method that cycles through the graph to unify all the traits - Modify the traits error message to include both the package identity and display name for better readability TODO - change instances of optional enabledTraits to non-optional, defaulting instead to ["default"] - assure that loaded manifests arent fetching guarded dependencies - this is contingent on how we are calculating the unified traits afterwards
Since there is a bit of redundancy when checking the conditions of an optional set of enabled traits, it makes more sense to simply have the set of enabled traits be non-optional, and to derive the checks for defaults by asserting whether the set contains the "default" keyword.
To help with the computation of traits, a wrapper struct for a dictionary of traits has been created wherein the default value when a key isn't found is no longer nil but instead ["default"], since if we haven't defined a set of explicitly enabled traits for a given package, then it should default to the defaults.
* Whenever this method is called, it will store a previously- created Workspace object in the SwiftCommandState. Some calls to this method were missing the traitConfiguration, thus instantiating the Workspace without the necessary trait config.
|
@swift-ci please test |
- Previous calls to getActiveWorkspace could have defaulted to using the default trait configuration (re: PluginCommand -> PluginDelegate.createSymbolGraphForPlugin case), therefore we should update the existing workspace's trait configuration if a new one is passed.
Since the trait configuration is used for a variety of swift package subcommands, it makes it a lot easier to add the trait options as a part of the GlobalOptions which is consumed by SwiftCommandState and persisted across all SwiftCommands. This also allows us to omit passing a trait configuration to the build system factory methods, and instead replacing it with a flag that can override the trait configuration to enable all traits for edge-case scenarios (e.g. fetching symbol graphs)
|
@swift-ci please test |
|
@swift-ci please test |
|
@swift-ci please test |
bripeticca
requested review from
AnthonyLatsis,
bnbarham,
xedin and
jakepetroules
as code owners
bripeticca
requested review from
dschaefer2,
shawnhyam,
plemarquand,
owenv,
bkhouri,
cmcgee1024 and
daveyc123
as code owners
bripeticca
changed the title
bripeticca
changed the title
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.
Trait-guarded dependencies were still being considered during dependency resolution, when they should be excluded if they aren't being used in any other scenario. Additionally, enabled traits should be pre-computed before entering the resolution stage in order to avoid possible race conditions when navigating through the dependency graph nodes.
Modifications:
Since we have the
--experimental-prune-unused-dependenciesfeature behind an experimental flag, we'll now consider an alternate path that will prune trait-guarded package dependencies from the dependency graph if and only if said dependency is not used in any other unguarded context.A dictionary wrapper titled
EnabledTraitsMapis used to store the enabled traits per package for the package graph and is stored as a property within theWorkspace, with some additional behaviour to return a["default"]set of traits if the package has not yet been stored in the dictionary, rather than returningnil.Following this behaviour, when passing a set of traits to methods that require them (e.g. for dependency computation, enabled trait computation, etc.) we now require that it is not Optional, since the checks done on a
nilset of traits were redundant.SwiftCommandState now also stores a
TraitConfiguration, since we'll want access to this across multipleSwiftCommands, and it is essentially a part of the state anyhow.TraitOptionsis now included in theGlobalOptionsforSwiftCommands to supplement this, so that when aSwiftCommandStateis created we will have access to the user-passed enabled traits. These options, as entitled, are available globally across all the swift package commands, so previous properties that declaredTraitOptionsin these commands has been removed in favour of using it straight from theGlobalOptions.Result:
Trait-guarded dependencies are excluded from dependency resolution, and since traits are pre-computed there should no longer be an issue with race conditions for traits in resolution as well.