fix: improvement on handling build-only dev dependencies #929
Conversation
This will still only work for cases where the MinVersion is included. Exotic cases will still fail as they would need a lookup to the source. Signed-off-by: Kraemer, Benjamin <benjamin.kraemer@alien-scripts.de>
|
I adjusted the code to not create the dummies for now but to at least fix the broken |
Dummy entries to be discussed further Signed-off-by: Kraemer, Benjamin <benjamin.kraemer@alien-scripts.de>
|
I looked into it longer and in my opinion, having an option to allow generating missing entries could solve this for non- |
Signed-off-by: Kraemer, Benjamin <benjamin.kraemer@alien-scripts.de>
| foreach (var item in dotnetDependencys) | ||
| { | ||
| item.IsDirectReference = true; | ||
| } | ||
|
|
There was a problem hiding this comment.
This was wrong in the first place and only compensated the missing IsDirectReference from PackagesFileService.GetDotnetDependencysAsync(). So by fixing it there, this is not necessary anymore. I assume this is here from before --includeProjectReferences was around, but that handles this already.
| // A higher version might already be added | ||
| allAddedDepedencyNames = dotnetDependencys.Select(dep => dep.Name); | ||
| dotnetDependencys.UnionWith(projectDotnetDependencys.Where(pr => !allAddedDepedencyNames.Contains(pr.Name)).Select(d => | ||
| { | ||
| // Ensure project references are not added as direct references | ||
| d.IsDirectReference = false; | ||
| return d; | ||
| })); |
There was a problem hiding this comment.
The current code would merge in older versions from referenced projects . In my case I got a second version of Serilog in which was incorrect.
Serilog.Extensions.Hosting@8.0.0 on the referenced project used referenced Serilog@3.1.1 but my main-project referenced already Serilog@4.0.0 which was also set as dependency for Serilog.Extensions.Hosting@8.0.0 in dotnetDependencys correctly. So Serilog@3.1.1 was added as component but never referenced due to Serilog.Extensions.Hosting@8.0.0 by UnionWith being taken from the main project.
| /// </summary> | ||
| private static void ResolveDependencyVersionRanges(HashSet<DotnetDependency> runtimePackages) | ||
| private static void ResolveDependencyVersionRanges(HashSet<DotnetDependency> runtimePackages, IDictionary<string, CentralPackageVersion> centralPackageVersions) |
There was a problem hiding this comment.
I split-up the logic to allow better resolution with -rs for missing dev-dependencies. This can be easily reproduced by referencing Grpc.Tools in any version and referencing that project by another project. The SBOM has to be generated for that second project. Grpc.Tools is a build-only dev dependency that is stripped by the project.assets.json generator as non-relevant (which cyclonedx would rely on).
So this new method tries to now only reduce the range to what was set in the central package management (if used) to be later on the Runner.ResolveDependencyVersionRanges doing the matching. For non-recursive mode, this will make no difference and generate the same warnings at that point (now with an added note to use recursive mode). In recursive mode, the files would have been added by that point through the package reference and therefore resolve without warnings to the correct non-ranged version.
| } | ||
|
|
||
| // Despite the name, this could already be a non-range version. | ||
| patchVersion = cpv.VersionRange.ToShortString(); |
There was a problem hiding this comment.
ToShortString generated the simplest name to be used:
- 1 -> 1.0.0
- [1, ) -> 1.0.0
- (1, ) -> (1.0.0, )
- [1, 2) -> [1.0.0, 2.0.0)
Non-min-inclusive version ranges would not be resolvable by the old version of the code, but will be resolvable in the new run for recursive project or solution runs.
| @@ -64,6 +63,7 @@ public async Task<HashSet<DotnetDependency>> GetDotnetDependencysAsync(string pa | |||
| Name = reader["id"], | |||
| Version = reader["version"], | |||
| IsDevDependency = reader["developmentDependency"] == "true", | |||
| IsDirectReference = true, | |||
There was a problem hiding this comment.
Was was defaulting to false but packages in the config are always directly referenced by definition and would be migrated as those for PackageReference.
| /// <summary> | ||
| /// Updates all dependencies with version ranges to the version it was resolved to where possible. | ||
| /// </summary> | ||
| private static void ResolveDependencyVersionRanges(HashSet<DotnetDependency> runtimePackages) |
There was a problem hiding this comment.
This will now resolve the packages and is basically the same code as before on the ProjectAssetFileService. Moving it ensures that it now really sees all packages, including build-only dev dependencies stripped by the project.assets.json generator.
It's important that this will still only resolve the packages for recursive project or solution mode. On non-recursive, these will still be missing and there is no way to fetch them. They COULD be added in non-recursive mode as IsDevDependency=true with Scope=Exclude but might still miss things like dependencies of the real package. So I (for now) decided against adding any workaround and only suggest the user in the warning to use recursive mode instead.
Signed-off-by: Kraemer, Benjamin <benjamin.kraemer@alien-scripts.de>
Fixes #894 for
--recursiveproject mode and solution mode.This also fixes the following bugs that were not reported yet to my knowledge but lead to wrong results:
packages.confignot being consideredIsDirectReferenceSo many bugs were related to
--recursivenot working correctly.