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

When setting framework restriction on nuget package with .props and .targets, elements in .csproj are moved to wrong location #1898

Closed
theggelund opened this Issue Aug 29, 2016 · 12 comments

Comments

Projects
None yet
2 participants
@theggelund

theggelund commented Aug 29, 2016

Description

Added framework: net452, net46, net461 to specification in paket.dependencies

From

nuget DIPS.MultiTarget ~> 1.0.0 alpha

To

nuget DIPS.MultiTarget ~> 1.0.0 alpha framework: net452, net46, net461

Expected behavior

Should not change location of imports. .props import should be at the top, .targets should be at the bottom.

Actual behavior

image

Known workarounds

Solution to the issue is to not add framework restriction to package but then I need to add framework restriction to all other packages instead of just using framework restriction at the top

@theggelund

This comment has been minimized.

Show comment
Hide comment

theggelund commented Aug 29, 2016

image

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Aug 29, 2016

Member

can you please add a zip with the repro?

Member

forki commented Aug 29, 2016

can you please add a zip with the repro?

@forki forki added the needs-repro label Aug 29, 2016

@theggelund

This comment has been minimized.

Show comment
Hide comment
@theggelund

theggelund commented Aug 29, 2016

framework_bug2.zip

reproduce.bat

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Aug 29, 2016

Member

In your sample the props are still above all references and the targets below all references.

Do you think we should move it just below the latst "PropertyGroup"?

Member

forki commented Aug 29, 2016

In your sample the props are still above all references and the targets below all references.

Do you think we should move it just below the latst "PropertyGroup"?

@theggelund

This comment has been minimized.

Show comment
Hide comment
@theggelund

theggelund Aug 29, 2016

.props should be before the first "PropertyGroup" and
.targets should be below the last "PropertyGroup"

So I think that for .targets it still works, it's the .props import that is moved from line 3 to line 99, and this fails

theggelund commented Aug 29, 2016

.props should be before the first "PropertyGroup" and
.targets should be below the last "PropertyGroup"

So I think that for .targets it still works, it's the .props import that is moved from line 3 to line 99, and this fails

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Aug 29, 2016

Member

no we can't move it before the propertygroups since the constants for the choose nodes are only defined after the propertygroupds. anyways I'm pushing a fix which moves it up. Please give it a try

Member

forki commented Aug 29, 2016

no we can't move it before the propertygroups since the constants for the choose nodes are only defined after the propertygroupds. anyways I'm pushing a fix which moves it up. Please give it a try

@forki forki closed this in 7c1591f Aug 29, 2016

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Aug 29, 2016

Member

so it's released. can you please give it a shot?

Member

forki commented Aug 29, 2016

so it's released. can you please give it a shot?

@theggelund

This comment has been minimized.

Show comment
Hide comment
@theggelund

theggelund Aug 29, 2016

Same result, The first import project element is moved.

Maybe we are looking at the issue the wrong way. What if the problem should be how to ignore framework restriction. My paket.dependencies has a lot of duplicated

nuget [packagename] framework: net452, net46, net461

what if I could write this at the top of package.dependencies as described in the documentation

framework: net452, net46, net461 

and then for my dependency that should not have any framework restriction i just ignore them

nuget Multitarget framework: ignore

theggelund commented Aug 29, 2016

Same result, The first import project element is moved.

Maybe we are looking at the issue the wrong way. What if the problem should be how to ignore framework restriction. My paket.dependencies has a lot of duplicated

nuget [packagename] framework: net452, net46, net461

what if I could write this at the top of package.dependencies as described in the documentation

framework: net452, net46, net461 

and then for my dependency that should not have any framework restriction i just ignore them

nuget Multitarget framework: ignore
@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Aug 29, 2016

Member

it's expected that it is moved below the propertygroups. question is why that is not good enough

Member

forki commented Aug 29, 2016

it's expected that it is moved below the propertygroups. question is why that is not good enough

@theggelund

This comment has been minimized.

Show comment
Hide comment
@theggelund

theggelund Aug 30, 2016

Why is it expected to be moved below the first PropertyGroup?

Nuget adds the import project at the top before the first PropertyGroup

This is the first PropertyGroup in all csproj files. MSBuild reads csproj from the top to the bottom, so if you check the TargetFrameworkProfiler or DefineConstants, they have a condition attribute saying that if already defined, ignore this. To replace these values from the imported project, it has to come before the first PropertyGroup.

<PropertyGroup>
    <Configuration Condition="'$(Configuration)' == ''">Debug</Configuration>
    <ProjectGuid>{AA7FD029-8F87-4804-B9EA-0B8549ACEAB4}</ProjectGuid>
    <OutputType>Library</OutputType>
    ....
    <TargetFrameworkProfile Condition="'$(TargetFrameworkVersion)' == 'v4.0'">Client</TargetFrameworkProfile>
    <DefineConstants Condition="'$(DefineConstants)' == ''">NET40;NET45;NET451;NET452;NET46</DefineConstants>
  </PropertyGroup>

If I just could ignore framework restriction on this particular package and let all other packages have it, then this would not be an issue. If I don't add any framework restrictions on this package, the Import element is placed at the correct location

theggelund commented Aug 30, 2016

Why is it expected to be moved below the first PropertyGroup?

Nuget adds the import project at the top before the first PropertyGroup

This is the first PropertyGroup in all csproj files. MSBuild reads csproj from the top to the bottom, so if you check the TargetFrameworkProfiler or DefineConstants, they have a condition attribute saying that if already defined, ignore this. To replace these values from the imported project, it has to come before the first PropertyGroup.

<PropertyGroup>
    <Configuration Condition="'$(Configuration)' == ''">Debug</Configuration>
    <ProjectGuid>{AA7FD029-8F87-4804-B9EA-0B8549ACEAB4}</ProjectGuid>
    <OutputType>Library</OutputType>
    ....
    <TargetFrameworkProfile Condition="'$(TargetFrameworkVersion)' == 'v4.0'">Client</TargetFrameworkProfile>
    <DefineConstants Condition="'$(DefineConstants)' == ''">NET40;NET45;NET451;NET452;NET46</DefineConstants>
  </PropertyGroup>

If I just could ignore framework restriction on this particular package and let all other packages have it, then this would not be an issue. If I don't add any framework restrictions on this package, the Import element is placed at the correct location

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Aug 30, 2016

Member

Because the choose nodes depend on the target framework which is defined in
the property group nodes.

Am 30.08.2016 7:48 vorm. schrieb "Thomas Heggelund" <
notifications@github.com>:

Why is it expected to be moved below the first PropertyGroup?

Nuget adds the import project at the top before the first PropertyGroup
https://docs.nuget.org/create/creating-and-publishing-a-package#import-msbuild-targets-and-props-files-into-project

This is the first PropertyGroup in all csproj files. MSBuild reads csproj
from the top to the bottom, so if you check the TargetFrameworkProfiler or
DefineConstants, they have a condition attribute saying that if already
defined, ignore this. To replace these values from the imported project, it
has to come before the first PropertyGroup.

Debug {AA7FD029-8F87-4804-B9EA-0B8549ACEAB4} Library .... Client NET40;NET45;NET451;NET452;NET46

If I just could ignore framework restriction on this particular package
and let all other packages have it, then this would not be an issue. If I
don't add any framework restrictions on this package, the Import element is
placed at the correct location


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#1898 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNPPPs5Sxw7hNTH7GUAWnGaA7YpaZks5qk8Q1gaJpZM4JvT-C
.

Member

forki commented Aug 30, 2016

Because the choose nodes depend on the target framework which is defined in
the property group nodes.

Am 30.08.2016 7:48 vorm. schrieb "Thomas Heggelund" <
notifications@github.com>:

Why is it expected to be moved below the first PropertyGroup?

Nuget adds the import project at the top before the first PropertyGroup
https://docs.nuget.org/create/creating-and-publishing-a-package#import-msbuild-targets-and-props-files-into-project

This is the first PropertyGroup in all csproj files. MSBuild reads csproj
from the top to the bottom, so if you check the TargetFrameworkProfiler or
DefineConstants, they have a condition attribute saying that if already
defined, ignore this. To replace these values from the imported project, it
has to come before the first PropertyGroup.

Debug {AA7FD029-8F87-4804-B9EA-0B8549ACEAB4} Library .... Client NET40;NET45;NET451;NET452;NET46

If I just could ignore framework restriction on this particular package
and let all other packages have it, then this would not be an issue. If I
don't add any framework restrictions on this package, the Import element is
placed at the correct location


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#1898 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNPPPs5Sxw7hNTH7GUAWnGaA7YpaZks5qk8Q1gaJpZM4JvT-C
.

forki added a commit that referenced this issue Aug 30, 2016

@theggelund

This comment has been minimized.

Show comment
Hide comment
@theggelund

theggelund Aug 30, 2016

A chicken and egg issue then. How would someone target multiple frameworks if you cannot change the TargetFramework?

My solution to the problem is to add framework restriction to all dependencies but the multitarget dependency. What if I could do as I suggested earlier, to set framework restriction at the top, and then, for this particular package I set it to none/ignore/or some ignore constant. That way, I do not have to duplicate the framework restriction on all dependencies

theggelund commented Aug 30, 2016

A chicken and egg issue then. How would someone target multiple frameworks if you cannot change the TargetFramework?

My solution to the problem is to add framework restriction to all dependencies but the multitarget dependency. What if I could do as I suggested earlier, to set framework restriction at the top, and then, for this particular package I set it to none/ignore/or some ignore constant. That way, I do not have to duplicate the framework restriction on all dependencies

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