Skip to content
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

Suspicious AppBaseCompilationAssemblyResolver behaviour #1047

Closed
MarcoRossignoli opened this issue Dec 19, 2019 · 4 comments
Closed

Suspicious AppBaseCompilationAssemblyResolver behaviour #1047

MarcoRossignoli opened this issue Dec 19, 2019 · 4 comments
Labels
area-DependencyModel question Answer questions and provide assistance, not an issue with source code or documentation.

Comments

@MarcoRossignoli
Copy link
Member

MarcoRossignoli commented Dec 19, 2019

On coverlet there are some scenario where we need to manually search asm to pass to Cecil during instrumentation.
During some user issue analisys we found a strange behaviour for AppBaseCompilationAssemblyResolver that lead to a resolution issue.

This is the point coverlet-coverage/coverlet#655 (comment)

foreach (var assembly in library.Assemblies)
{
bool resolved = false;
var assemblyFile = Path.GetFileName(assembly);
foreach (var directory in directories)
{
string fullName;
if (ResolverUtils.TryResolveAssemblyFile(_fileSystem, directory, assemblyFile, out fullName))
{
paths.Add(fullName);
resolved = true;
break;
}
}
if (!resolved)
{
return false;
}
}
// only modify the assemblies parameter if we've resolved all files
assemblies?.AddRange(paths);
return true;

Is it correct that in case of empty Assemblies resolver return true but actually without add any path to assemblies list?

cc: @jhartmann123

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Dec 19, 2019
@MarcoRossignoli
Copy link
Member Author

Any news on this?

@eerhardt
Copy link
Member

My understanding of the logic is that the method returns true if it resolved all the Assemblies in the CompilationLibrary. In this case there were no .Assemblies, so they all were resolved. The other resolvers that we provide behave that way. Here are the other 2:

foreach (var assembly in library.Assemblies)
{
string fullName;
if (!ResolverUtils.TryResolveAssemblyFile(fileSystem, basePath, assembly, out fullName))
{
// if one of the files can't be found, skip this package path completely.
// there are package paths that don't include all of the "ref" assemblies
// (ex. ones created by 'dotnet store')
results = null;
return false;
}
paths.Add(fullName);
}
results = paths;
return true;

foreach (var assembly in library.Assemblies)
{
string fullName;
if (!TryResolveReferenceAssembly(assembly, out fullName))
{
throw new InvalidOperationException($"Cannot find reference assembly '{assembly}' file for package {library.Name}");
}
assemblies.Add(fullName);
}
return true;

If you want/need to handle this situation separately, I would advise injecting your own resolver to the beginning of the list when invoking CompilationLibrary.ResolveReferencePaths(params ICompilationAssemblyResolver[] customResolvers).

@eerhardt eerhardt added question Answer questions and provide assistance, not an issue with source code or documentation. and removed untriaged New issue has not been triaged by the area owner labels Feb 26, 2020
@eerhardt
Copy link
Member

Note that if the resolvers didn’t have this behavior, an exception would be thrown any time this situation occurred.

throw new InvalidOperationException($"Cannot find compilation library location for package '{Name}'");

@MarcoRossignoli
Copy link
Member Author

Understood thank you @eerhardt!

@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-DependencyModel question Answer questions and provide assistance, not an issue with source code or documentation.
Projects
None yet
Development

No branches or pull requests

4 participants