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

Feature Request: Reference package instead of referencing Dll files #272

Open
moh-hassan opened this issue Dec 14, 2019 · 12 comments
Open

Comments

@moh-hassan
Copy link

The generated project reference dll files and it's supposed to reference the nuget package

so instead of any dll file which is defined in HintPath:

<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>

It should be:

<ItemGroup>
  <PackageReference Include="Autofac" Version="4.2.1" />
  <PackageReference Include="Autofac.Extras.Plugins" Version="1.0.0" />
</ItemGroup>

These packages are defined in the file: Packages.config.

Also, these Assemblies are reference by default in the project, so no need to reference it:

System
System.Core
System.Data
System.Drawing
System.Io.Compression.FileSystem
System.Numerics
System.RunTime.Serialization
System.Xml
System.Xml.Linq
@andrew-boyarshin
Copy link
Collaborator

@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
Copy link
Author

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

@andrew-boyarshin
Copy link
Collaborator

andrew-boyarshin commented Dec 16, 2019

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Author

@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
Copy link
Author

@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
Copy link
Collaborator

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

  1. 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
Copy link
Collaborator

@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
Copy link
Author

@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.

@andrew-boyarshin
Copy link
Collaborator

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.

@moh-hassan
Copy link
Author

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.

@moh-hassan moh-hassan changed the title Reference package instead of referencing Dll files Feature Request: Reference package instead of referencing Dll files Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants