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

Source-Only NuGet packages add <Compile Include="..." /> to csproj, resulting in build errors. #3269

Closed
btodts opened this Issue Jun 26, 2018 · 14 comments

Comments

Projects
None yet
3 participants
@btodts

btodts commented Jun 26, 2018

Description

Paket Installing a source-only NuGet package adds a Compile element to the csproj file, which leads to build errors in the new dotnet sdk: Duplicate Compile items were included. The .NET SDK includes Compile items from your project directory by default. You can either remove these items from your project file, or set the 'EnableDefaultCompileItems' property to 'false' if you want to explicitly include them in your project file.

Repro steps

  1. Step A

Add e.g. TinyIoC to your paket.dependencies and paket.references of your dotnet 1.1+ console application.

  1. Step B

Run paket Install.

Expected behavior

The files in the Content folder of the NuGet package (in TinyIoC's case, TinyIoC.cs) are added to the project. Since the dotnet sdk moved the globs to include compile items to the properties of the sdk, it isn't necessary that an element (<Compile Include="TinyIoC.cs" />) is added to the cspoj. Expected behavior is therefore that this doesn't happen. (more info: https://aka.ms/sdkimplicititems)

Actual behavior

It does :) A Compile element is added to the csproj, which causes build errors (see above).

Known workarounds

The error goes away when you remove the element from the csproj, but it is added again after every paket Install, which is unfortunate.

@btodts

This comment has been minimized.

btodts commented Jul 4, 2018

Ping. Anybody there?

@forki

This comment has been minimized.

Member

forki commented Jul 4, 2018

can you please upload a zip with a repro. Maybe I find some time before next vacation

@btodts

This comment has been minimized.

btodts commented Jul 4, 2018

PaketRepro.zip
@forki Repro attached.

Steps to reproduce:

  1. (just to make sure) Build the project. This should succeed.
  2. Run paket install: this adds a file TinyIoC.cs to the project and adds an ItemGroup element to the csproj file.
  3. Build now breaks because of that ItemGroup element (specifically, the <Compile Include="TinyIoC.cs"> element).
@forki

This comment has been minimized.

Member

forki commented Jul 4, 2018

ok fixed in latest alpha

@forki forki closed this Jul 4, 2018

@btodts

This comment has been minimized.

btodts commented Jul 6, 2018

@forki Unfortunately, files from the NuGet packages aren't copied to the directory anymore now (using paket 5.174.0). You can reproduce this with the zip file I provided earlier.

@forki

This comment has been minimized.

Member

forki commented Jul 6, 2018

@btodts to which directory? we now rely fully on the nuget functionality. I thought that was the plan here

@btodts

This comment has been minimized.

btodts commented Jul 6, 2018

The project folder that contains the paket.references file.
For clarification:
Pre 5.174.0: files from the NuGet package's Content folder were copied to the project folder during paket install but also added an element to the csproj (which caused errors).
5.174.0: said files in the Content folder are no longer copied to the project folder

@forki

This comment has been minimized.

Member

forki commented Jul 6, 2018

ok, but should't the PackageReference handle this now? how would it wor in vanilla nuget? It makes no sense to me yet

@btodts

This comment has been minimized.

btodts commented Jul 6, 2018

If I understand https://aka.ms/sdkimplicititems correctly, the properties of the dotnet core SDK now contain globs to include files that should (or should not) be compiled during the MSBuild process, whereas the 'old' way is including those globs in the csproj file as <Compile Include="..." /> elements.

PackageReferences are used to manage packages directly from the project file (as opposed to a packages.config file). The point of NuGet packages like TinyIoC (commonly referred to as 'Source-Only NuGet packages') is to simply include the source files in your code without specifying a dependency that your consumers need to adhere to (so PackageReference isn't a solution I'm afraid).
Thanks to the aforementioned globs in the SDK properties, MSBuild knows that it should also compile those source files (in this case, because the file ends with .cs).

@forki

This comment has been minimized.

Member

forki commented Jul 6, 2018

yes, but what do we need to change? how would it work in vanilla nuget?

@btodts

This comment has been minimized.

btodts commented Jul 6, 2018

Well, upon paket install, the csproj file shouldn't be changed when using .NET core 1.0+.
I tried reproducing it with vanilla nuget, but unfortunately I can currently only get it work with the .NET Framework, not .NET core...

@btodts

This comment has been minimized.

btodts commented Jul 6, 2018

FYI, I've updated the issue I previously posted over @ NuGet to investigate the missing behaviour when using .NET Core: NuGet/Home#7053

@CumpsD

This comment has been minimized.

Contributor

CumpsD commented Jul 30, 2018

@forki I think this change introduced a new bug. After debugging with @btodts we found the following:

In this commit: 4f40b2f Map.empty was introduced, causing no more Content to be copied and no more Content nodes to be added to a csproj.

When we revert this to the old syntax, we get the desired functionality again, seeing these are generated inside the csproj:

    <Content Include="MunicipalityRegistry.Api.Legacy.xml">
      <Paket>True</Paket>
      <CopyToOutputDirectory>Always</CopyToOutputDirectory>
    </Content>

The problem @btodts had was it would generate <Compile Include=.... nodes for files ending on .cs, which the new Core compiler chokes on because it has duplicate Compile nodes.

This actually comes from:

| ext when Path.GetExtension project.FileName = ext + "proj"
-> BuildAction.Compile

 let determineBuildAction fileName (project:ProjectFile) =
        match (Path.GetExtension fileName).ToLowerInvariant() with
        | ext when Path.GetExtension project.FileName = ext + "proj"
            -> BuildAction.Compile
        | ".fsi" -> BuildAction.Compile
        | ".xaml" -> BuildAction.Page
        | ".ttf" | ".png" | ".ico" | ".jpg" | ".jpeg"| ".bmp" | ".gif"
        | ".wav" | ".mid" | ".midi"| ".wma" | ".mp3" | ".ogg" | ".rma"
        | ".avi" | ".mp4" | ".divx"| ".wmv"  //TODO: and other media types
            -> BuildAction.Resource
        | _ -> BuildAction.Content

I think (not sure how to do it), the match on ext when Path.GetExtension project.FileName = ext + "proj" should somehow take the toolsversion into account and not add cs as a Compile but as a Content.

If that makes any sense? Maybe you can shed a light on how to approach this change? Something along the lines of:

    let determineBuildAction fileName (project:ProjectFile) =
        match (Path.GetExtension fileName).ToLowerInvariant() with
        | ext when Path.GetExtension project.FileName = ext + "proj"
            -> match getToolsVersion project with
                | 15.0 -> BuildAction.Content
                | _ -> BuildAction.Compile
        | ".fsi" -> BuildAction.Compile
        | ".xaml" -> BuildAction.Page
        | ".ttf" | ".png" | ".ico" | ".jpg" | ".jpeg"| ".bmp" | ".gif"
        | ".wav" | ".mid" | ".midi"| ".wma" | ".mp3" | ".ogg" | ".rma"
        | ".avi" | ".mp4" | ".divx"| ".wmv"  //TODO: and other media types
            -> BuildAction.Resource
        | _ -> BuildAction.Content
@CumpsD

This comment has been minimized.

Contributor

CumpsD commented Jul 30, 2018

PR coming.... Verified the proposal, fixes the issue and solves the regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment