While working on indexing, it became clear that the Package trait contains methods that the solvers do no use or need to use. Some of the things in the trait seem better suited for building or testing or information only rather than solving. The trait could be split up along the lines of "things solvers needs" and possibly several sets of "things solvers don't need".
Methods that aren't implemented in IndexedPackage because the solver's don't call them:
runtime_environment() from RuntimeEnvironmentmetadata() from Package`sources() from Packageget_all_tests() from Packagedownstream_build_requirements() from DownstreamRequirementsset_build() from PackageMutinsert_or_merge_install_requirement() from PackageMut
Duplicated because of traits involved:
matches_all_filters() from Package, but the method relies on the separate Satisfy<...> trait
Existing traits involved (not an exhaustive list):
PackageDeprecateMut because Spec enum requires itRuntimeEnvironmentDownstreamRequirementsPackageMut because Spec enum requires itSatisfy<...>
Possible new traits (discussion starting point only):
SolverPackageBuilderPackageMetadataSourcesTests
Other possible new traits
VersionRange: most entries have a version() method that returns a Cow<Version>. But one entry does not (WildCard). There is an existing HasVersion trait that defines a fn version(&self) -> &Version method. But the VersionRange enum and its entries do not use that. The trait would need to change the return type (and/or allow Result, Option, or a sentinel value). Can this use another similar trait, is it too similar, could HasVersion be changed, or should the enum continue without a trait?
Related comments and background from the Indexing PRs:
Same idea here of something that shouldn't be in Package.
It does leave me wondering if we want to have a separate SolverPackage trait with a reduced set of requirements that the solver expects. We either need that or some combination of sub-dividing Package to cater to the different needs of different subsystems.
Originally posted by @jrray in #1338 (comment)
It's more work of course but I would like to refactor traits such that metadata isn't a required method on Package and then also the solver doesn't require an implementation of whatever [new] trait metadata moves to. Therefore you know for a fact this is unreachable; the code doesn't have to exist.
Originally posted by @jrray in #1338 (comment)
re: DeprecateMut trait:
sus if the solver requires this trait, but that's a battle for another time.
Originally posted by @jrray in #1338 (comment)
re: sources() method:
Similar to the idea above about moving metadata out of Package, I'm thinking Package shouldn't have a sources either -- wouldn't that be a property for Recipe to have? In the future with #1187 we won't have this property in a "package".
Originally posted by @jrray in #1338 (comment)
re: RuntimeEnvironment trait:
Add this to the list of traits to figure out why the solver would need it implemented.
Originally posted by @jrray in #1338 (comment)
re: matches_all_filters() method being duplicated for two implementors because of the traits involved:
If this is a literal copy, unchanged, then it could live in an extension trait that both these types share.
Originally posted by @jrray in #1338 (comment)
It is an exact duplicate. The code could almost be the default implementation for this methond in the Package trait, except it relies on check_satisfies_request() which is in the Satisfiy..,> trait that the Package trait does not rely on - but both PackageSpec and IndexedPackage do.
Originally posted by @jdcookspi in #1338 (comment)
While working on indexing, it became clear that the
Packagetrait contains methods that the solvers do no use or need to use. Some of the things in the trait seem better suited for building or testing or information only rather than solving. The trait could be split up along the lines of "things solvers needs" and possibly several sets of "things solvers don't need".Methods that aren't implemented in IndexedPackage because the solver's don't call them:
runtime_environment()fromRuntimeEnvironmentmetadata() fromPackage`sources()fromPackageget_all_tests()fromPackagedownstream_build_requirements()fromDownstreamRequirementsset_build()fromPackageMutinsert_or_merge_install_requirement()fromPackageMutDuplicated because of traits involved:
matches_all_filters()fromPackage, but the method relies on the separateSatisfy<...>traitExisting traits involved (not an exhaustive list):
PackageDeprecateMutbecauseSpecenum requires itRuntimeEnvironmentDownstreamRequirementsPackageMutbecauseSpecenum requires itSatisfy<...>Possible new traits (discussion starting point only):
SolverPackageBuilderPackageMetadataSourcesTestsOther possible new traits
VersionRange: most entries have aversion()method that returns aCow<Version>. But one entry does not (WildCard). There is an existingHasVersiontrait that defines afn version(&self) -> &Versionmethod. But theVersionRangeenum and its entries do not use that. The trait would need to change the return type (and/or allow Result, Option, or a sentinel value). Can this use another similar trait, is it too similar, could HasVersion be changed, or should the enum continue without a trait?Related comments and background from the Indexing PRs:
Same idea here of something that shouldn't be in
Package.It does leave me wondering if we want to have a separate
SolverPackagetrait with a reduced set of requirements that the solver expects. We either need that or some combination of sub-dividingPackageto cater to the different needs of different subsystems.Originally posted by @jrray in #1338 (comment)
It's more work of course but I would like to refactor traits such that metadata isn't a required method on Package and then also the solver doesn't require an implementation of whatever [new] trait metadata moves to. Therefore you know for a fact this is unreachable; the code doesn't have to exist.
Originally posted by @jrray in #1338 (comment)
sus if the solver requires this trait, but that's a battle for another time.
Originally posted by @jrray in #1338 (comment)
Similar to the idea above about moving metadata out of Package, I'm thinking Package shouldn't have a sources either -- wouldn't that be a property for Recipe to have? In the future with #1187 we won't have this property in a "package".
Originally posted by @jrray in #1338 (comment)
Add this to the list of traits to figure out why the solver would need it implemented.
Originally posted by @jrray in #1338 (comment)
If this is a literal copy, unchanged, then it could live in an extension trait that both these types share.
Originally posted by @jrray in #1338 (comment)
It is an exact duplicate. The code could almost be the default implementation for this methond in the Package trait, except it relies on check_satisfies_request() which is in the Satisfiy..,> trait that the Package trait does not rely on - but both PackageSpec and IndexedPackage do.
Originally posted by @jdcookspi in #1338 (comment)