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

[Csproj] Add support for conditional PackageReferences #258

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jpankalainen
Copy link

Adds support for conditionals in csproj PackageReference entries.

@jspelletier
Copy link
Collaborator

checking this

@jspelletier jspelletier self-assigned this Mar 31, 2023
@jspelletier
Copy link
Collaborator

Thanks for this intesting proposal.

I discussed this with a colleague as I don't know well the csproj generator.

While your proposal works, we thought it might be better to extend the existing ItemGroup support for supporting new types of conditions(Platform + Configuration)

There is right now support for automatic ItemGroup section conditions for dotnet framework version as you can see in the DotNetMultiFrameworksHelloWorld sample.

This can generate PackageReference references with framework version conditions when multiple framework versions are specified as target fragments. As you can see, there is
no conditions here, the generation of conditions is driven by what happens in Configure methods.

Example(DotNetMultiFrameworksHelloWorld):
    if (target.GetFragment<DotNetFramework>().IsDotNetStandard())
    {
        conf.ReferencesByNuGetPackage.Add("System.Text.Encoding.CodePages", "4.5.0");
        conf.ReferencesByNuGetPackage.Add("Newtonsoft.Json", "13.0.3");
    }

would generate:

  <ItemGroup Condition="'$(TargetFramework)'=='netstandard2.0'">
    <PackageReference Include="Newtonsoft.Json" Version="13.0.3" />
    <PackageReference Include="System.Text.Encoding.CodePages" Version="4.5.0" />
  </ItemGroup>

I synced your branch and I modified the sample to use the nuget used in your tests to add more cases.


    if (target.GetFragment<DotNetFramework>().IsDotNetFramework())
    {
        conf.ReferencesByName.Add("System");
    }

    if (target.GetFragment<DotNetFramework>().IsDotNetStandard())
    {
        conf.ReferencesByNuGetPackage.Add("System.Text.Encoding.CodePages", "4.5.0");
        conf.ReferencesByNuGetPackage.Add("NUnit-debug", "3.4.1", condition: "$(Configuration)='Debug'");
        // Note: This can be confusing for people used to sharpmake configs as this code is executed in a debug config but it won't be
        // use in debug because of the condition
        conf.ReferencesByNuGetPackage.Add("NUnit", "3.4.1", condition: "$(Configuration)='Release'");
    }

    conf.ReferencesByNuGetPackage.Add("PackageAlwaysPresent", "1.0.0");

    
    generates:
  <ItemGroup Condition="'$(TargetFramework)'=='net461' OR '$(TargetFramework)'=='net472'">
    <Reference Include="System">
      <Private>False</Private>
    </Reference>
  </ItemGroup>
  <ItemGroup>
    <PackageReference Include="PackageAlwaysPresent" Version="1.0.0" />
  </ItemGroup>
  <ItemGroup Condition="'$(TargetFramework)'=='netstandard2.0'">
    <PackageReference Include="NUnit" Version="3.4.1" Condition="$(Configuration)='Release'" />
    <PackageReference Include="NUnit-debug" Version="3.4.1" Condition="$(Configuration)='Debug'" />
    <PackageReference Include="System.Text.Encoding.CodePages" Version="4.5.0" />
  </ItemGroup>

    
    Not bad but we think we should do something a bit different as we will explain below.

What we think we should do is we shouldn't specify the condition as parameter and modify Sharpmake so that it is able to
magically generate the condition for Configurations and Platforms automatically.
In fact, this is a bit similar to the PropertyGroup section. This would better align with how we do things in Sharpmake elsewhere.

Example(DotNetMultiFrameworksHelloWorld):

    if (target.GetFragment<DotNetFramework>().IsDotNetFramework())
    {
        conf.ReferencesByName.Add("System");
    }

    if (target.GetFragment<DotNetFramework>().IsDotNetStandard())
    {
        conf.ReferencesByNuGetPackage.Add("System.Text.Encoding.CodePages", "4.5.0");
        if (target.GetOptimization() == Optimization.Debug)
        {
            conf.ReferencesByNuGetPackage.Add("NUnit-debug", "3.4.1");
        }
        else
        {
            conf.ReferencesByNuGetPackage.Add("NUnit", "3.4.1");
        }
    }

    conf.ReferencesByNuGetPackage.Add("PackageAlwaysPresent", "1.0.0");

    
    This should generate:
    <ItemGroup Condition="'$(TargetFramework)'=='net461' OR '$(TargetFramework)'=='net472'">
      <Reference Include="System">
        <Private>False</Private>
      </Reference>
    </ItemGroup>
    <ItemGroup>
      <PackageReference Include="PackageAlwaysPresent" Version="1.0.0" />
    </ItemGroup>    
    <ItemGroup Condition="'$(TargetFramework)'=='netstandard2.0'">
        <PackageReference Include="System.Text.Encoding.CodePages" Version="4.5.0" />
    </ItemGroup>
    <ItemGroup Condition="'$(TargetFramework)'=='netstandard2.0' AND "$(Configuration)='Release'" ">
        <PackageReference Include="NUnit" Version="3.4.1" />
    </ItemGroup>
    <ItemGroup Condition="'$(TargetFramework)'=='netstandard2.0' AND "$(Configuration)='Debug'" ">
        <PackageReference Include="NUnit-debug" Version="3.4.1" />
    </ItemGroup>

To implement this, we would have to rename TargetFrameworksCondition to something like TargetsConditions and modify it to take into account the Configuration and platform associated with the item
that is added to the collection. I think this will handled automatically but the code shouldn't generate useless conditions, similar to the example just above.

For example:

  • if all configurations are using PackageAlwaysPresent, there should be no condition.
  • If only a single platform is used in a project, there is no need for a platform condition. As we can see above, there is no platform condition as there is only AnyCPU

@jspelletier jspelletier changed the base branch from dev to main April 6, 2023 12:50
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

Successfully merging this pull request may close these issues.

2 participants