Feature Request: Reference package instead of referencing Dll files about csprojtovs2017

[andrew-boyarshin]

If NuGet.config file is not found, heuristics are used. If a project is part of the solution, packages directory is an immediate child of solution folder. Otherwise, it is the one adjacent to the project folder. The heuristics are sound, even if not overly sophisticated. In your example, the tool correctly follows the heuristics and decides the HintPath does not point to a valid NuGet package cache folder.
A good idea would be to implement additional stage in migration wizard to provide means to force the conversion to PackageReference when the heuristics decided HintPath to be invalid, but it still contains packages/$(Id).$(Version)/ pattern. Since this is a potentially breaking migration, it should be opt-in, not opt-out. In my opinion, this issue should be considered a feature request, not a bug report.

[andrew-boyarshin]

@moh-hassan thank you for your report. If I understood you correctly, both of these features are already implemented in this tool. Are they not working correctly? If so, could you please provide the case for us to reproduce the issue?

[moh-hassan]

Thanks @andrew-boyarshin for reply.
Here the project in the attached file including .csproj and Packages.config

[andrew-boyarshin]

When I run the tool on the your test case, I get the following csproj (you've provided a part of the result, I am posting it for further reference):

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <ProjectGuid>{7238CC8F-C533-4DF5-8770-5D9EF82D91A9}</ProjectGuid>
    <TargetFramework>net462</TargetFramework>
    <AssemblyTitle>CommonComponents</AssemblyTitle>
    <Product>CommonComponents</Product>
    <Copyright>Copyright ©  2016</Copyright>
    <OutputPath>bin\$(Configuration)\</OutputPath>
  </PropertyGroup>
  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
    <DebugType>full</DebugType>
  </PropertyGroup>
  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' ">
    <DebugType>pdbonly</DebugType>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="Autofac" Version="4.2.1" />
  </ItemGroup>
  <ItemGroup>
    <Reference Include="Autofac, Version=4.2.1.0, Culture=neutral, PublicKeyToken=17863af14b0044da, processorArchitecture=MSIL">
      <HintPath>..\..\..\packages\Autofac.4.2.1\lib\net45\Autofac.dll</HintPath>
      <Private>True</Private>
    </Reference>
    <Reference Include="System.Data.DataSetExtensions" />
    <Reference Include="Microsoft.CSharp" />
    <Reference Include="System.Net.Http" />
  </ItemGroup>
  <ItemGroup>
    <ProjectReference Include="..\..\Autofac.Extras.Plugins\Autofac.Extras.Plugins.csproj" />
  </ItemGroup>
</Project>

The only issue I see here is Autofac assembly being referenced twice: once properly (as PackageReference), and once (as Reference) erroneously. I'll look into it.
Do you expect more to be removed? The rest of References are not safe to remove.

[andrew-boyarshin]

After a short investigation I consider the Autofac behavior correct. HintPath does point at the path outside the project tree. The tool plays it safe. But maybe we should provide the user (of a wizard) the ability to handle these cases manually (like a prompt with a candidate for removal).

[mungojam]

I created this feature and it is supposed to look in your NuGet settings (i.e. nuget.config) to check that the packages folder is the one that you have configured for the solution. If the two match, then it will remove the HintPath as it can be pretty sure that it was managed by NuGet previously.

I haven't checked recently to make sure that it is still working as the newer versions of this migrator seem to have a couple of issues in this area and I ended up using the VS tool to migrate to PackageReference prior to doing a migration with this tool.

Sorry, that's a bit vague, but if you are able to check if your NuGet settings align with the HintPath, then we can establish for sure if it is a bug. There was a unit test for it I believe so perhaps it is still working as originally intended.

I've not got much time to look at this at the mo.

[moh-hassan]

@mungojam
The new style SDK projects (2017+) are using PackageReference and I think migration should insure converting package.config and drop any dll file (has hint path file) and have a corresponding package in package.config.

[moh-hassan]

@andrew-boyarshin
Have a look to the hintPath:

<HintPath>..\..\..\packages\Autofac.4.2.1\lib\net45\Autofac.dll</HintPath>

The HintPath Point to folder in the format:

packages\<packagename>.<version>\lib\FFW\<dll filename>  

which insure that it's a package.
In that case it's better to use packageReference than File Reference via HintPath.
Also that package is included in package.config.

[mungojam]

The new style SDK projects (2017+) are using PackageReference and I think migration should insure converting package.config and drop any dll file (has hint path file) and have a corresponding package in package.config.

It does this, but only if the NuGet package location the HintPath matches either:

1. The setting from a nuget.config (either global or in the path hierarchy above the project).

or

2. The automatic convention of having a packages folder alongside the solution. I think for this to work, you need to tell the migrator to migrate the solution file rather than the project file.

I've restructured your example's folder structure and added a nuget.config to demonstrate that it works for case 1 when run with dotnet migrate-2019 wizard.

CommonComponentsWithNugetConfig.zip

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <ProjectGuid>{7238CC8F-C533-4DF5-8770-5D9EF82D91A9}</ProjectGuid>
    <TargetFramework>net462</TargetFramework>
    <AssemblyTitle>CommonComponents</AssemblyTitle>
    <Product>CommonComponents</Product>
    <Copyright>Copyright ©  2016</Copyright>
    <OutputPath>bin\$(Configuration)\</OutputPath>
  </PropertyGroup>
  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
    <DebugType>full</DebugType>
  </PropertyGroup>
  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' ">
    <DebugType>pdbonly</DebugType>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="Autofac" Version="4.2.1" />
  </ItemGroup>
  <ItemGroup>
    <Reference Include="System.Data.DataSetExtensions" />
    <Reference Include="Microsoft.CSharp" />
    <Reference Include="System.Net.Http" />
  </ItemGroup>
  <ItemGroup>
    <ProjectReference Include="..\..\Autofac.Extras.Plugins\Autofac.Extras.Plugins.csproj" />
  </ItemGroup>
</Project>

[mungojam]

@moh-hassan. I meant to ask, which case are you relying on for the packages to work prior to the migration? 1 or 2

[moh-hassan]

@mungojam
Neither 1 nor 2
I used the wizard for a project in the folder without nuget.config.
It seem that nuget.config is needed.
It's nice if wiki documentation describe the valid ideal environment to run the wizard and the importance of nuget.config.

[moh-hassan]

this issue should be considered a feature request, not a bug report

@andrew-boyarshin
You are correct. It should be a feature request.
Some consideration may be taken into account:

• The project may move from one machine to another and NuGet.config file may not be exist or invalid.
• The package directory may be deleted when zipping the project and it shouldn't be considered.

An option in commandline to ignore/use package directory / or NuGet.config is good.