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

.props files are broken #1233

Closed
inosik opened this Issue Nov 17, 2015 · 18 comments

Comments

Projects
None yet
3 participants
@inosik
Contributor

inosik commented Nov 17, 2015

Since v2.24.7 framework specific .props files (as in xunit.core, or our beloved xunit.runner.visualstudio) do not work anymore.

The problem is, that the framework specific part of the path (__paket__Package_Name_targets MSBuild property) isn't set, when the file gets imported.

@inosik

This comment has been minimized.

Show comment
Hide comment
@inosik

inosik Nov 17, 2015

Contributor

As I see now, the normal .props files are also broken, because we resolve the whole file name.

<Choose>
  <When Condition="$(TargetFrameworkIdentifier) == '.NETFramework' And $(TargetFrameworkVersion) == 'v4.5'">
    <PropertyGroup>
      <__paket__Package_Name_props>Package.Name</__paket__Package_Name_props>
    </PropertyGroup>
  </When>
</Choose>
Contributor

inosik commented Nov 17, 2015

As I see now, the normal .props files are also broken, because we resolve the whole file name.

<Choose>
  <When Condition="$(TargetFrameworkIdentifier) == '.NETFramework' And $(TargetFrameworkVersion) == 'v4.5'">
    <PropertyGroup>
      <__paket__Package_Name_props>Package.Name</__paket__Package_Name_props>
    </PropertyGroup>
  </When>
</Choose>

@inosik inosik changed the title from Framework specific .props files are broken to .props files are broken Nov 17, 2015

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 17, 2015

Member

I can reproduce.

Member

forki commented Nov 17, 2015

I can reproduce.

@forki forki added the bug label Nov 17, 2015

@forki forki closed this in db037f3 Nov 17, 2015

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 17, 2015

Member

could you please try latest alpha?

Member

forki commented Nov 17, 2015

could you please try latest alpha?

@inosik

This comment has been minimized.

Show comment
Hide comment
@inosik

inosik Nov 17, 2015

Contributor

Thanks, that solved the issue. But now we have #1219 again.

@theggelund Are there any issues at all if the .props aren't imported at the top of the file?

Contributor

inosik commented Nov 17, 2015

Thanks, that solved the issue. But now we have #1219 again.

@theggelund Are there any issues at all if the .props aren't imported at the top of the file?

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 17, 2015

Member

We have still #1219 covered for props that are global. There is an
integration test
On Nov 17, 2015 2:02 PM, "Ilja Nosik" notifications@github.com wrote:

Thanks, that solved the issue. But now we have #1219
#1219 again.

@theggelund https://github.com/theggelund Are there any issues at all
if the .props aren't imported at the top of the file?


Reply to this email directly or view it on GitHub
#1233 (comment).

Member

forki commented Nov 17, 2015

We have still #1219 covered for props that are global. There is an
integration test
On Nov 17, 2015 2:02 PM, "Ilja Nosik" notifications@github.com wrote:

Thanks, that solved the issue. But now we have #1219
#1219 again.

@theggelund https://github.com/theggelund Are there any issues at all
if the .props aren't imported at the top of the file?


Reply to this email directly or view it on GitHub
#1233 (comment).

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 17, 2015

Member

we can't put this up to the top since the properties for the case block are not defined at the top.

Member

forki commented Nov 17, 2015

we can't put this up to the top since the properties for the case block are not defined at the top.

@theggelund

This comment has been minimized.

Show comment
Hide comment
@theggelund

theggelund Nov 17, 2015

Ordering is important. Using conditional checks for variable not set is used to support amongst other features, multi-target builds

Accordning to nuget, props should be put at the top and targets should be put at the bottom.
Ordering is important, and by using conditionals that check if variable is set and giving a default value, multi-target solutions can be supported in visual studio

theggelund commented Nov 17, 2015

Ordering is important. Using conditional checks for variable not set is used to support amongst other features, multi-target builds

Accordning to nuget, props should be put at the top and targets should be put at the bottom.
Ordering is important, and by using conditionals that check if variable is set and giving a default value, multi-target solutions can be supported in visual studio

@inosik

This comment has been minimized.

Show comment
Hide comment
@inosik

inosik Mar 10, 2016

Contributor

@forki Sorry to bother you with this again, but we have to reopen this issue. Framework specific .props files get imported at the top of the project again.

Contributor

inosik commented Mar 10, 2016

@forki Sorry to bother you with this again, but we have to reopen this issue. Framework specific .props files get imported at the top of the project again.

@forki forki reopened this Mar 10, 2016

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Mar 10, 2016

Member

do we already have an integration test for this? (and did I break it ?)

Can you name a public nuget package which has this issue?

Member

forki commented Mar 10, 2016

do we already have an integration test for this? (and did I break it ?)

Can you name a public nuget package which has this issue?

@inosik

This comment has been minimized.

Show comment
Hide comment
@inosik

inosik Mar 10, 2016

Contributor

The infamous xunit.runner.visualstudio strikes back again. I prepared this repro sample.

IIRC we have an integration test for the opposite case: .props files which reside directly in build/, not in build/{framework}/.

Contributor

inosik commented Mar 10, 2016

The infamous xunit.runner.visualstudio strikes back again. I prepared this repro sample.

IIRC we have an integration test for the opposite case: .props files which reside directly in build/, not in build/{framework}/.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Mar 10, 2016

Member

I will take a look

Member

forki commented Mar 10, 2016

I will take a look

forki added a commit that referenced this issue Mar 10, 2016

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Mar 10, 2016

Member

xUnitTests.expected.csproj is that what you expect it to be? In 28116e0 the test suggests that this is actually the result that paket gives you.

Member

forki commented Mar 10, 2016

xUnitTests.expected.csproj is that what you expect it to be? In 28116e0 the test suggests that this is actually the result that paket gives you.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Mar 10, 2016

Member

so what did I do wrong in the test?

Member

forki commented Mar 10, 2016

so what did I do wrong in the test?

@inosik

This comment has been minimized.

Show comment
Hide comment
@inosik

inosik Mar 10, 2016

Contributor

Sorry, somehow I edited the expected result back again to the wrong one. This is how I'd expect it to be, the <Import /> should come either at the end of the file, or right after the <Choose />.

Contributor

inosik commented Mar 10, 2016

Sorry, somehow I edited the expected result back again to the wrong one. This is how I'd expect it to be, the <Import /> should come either at the end of the file, or right after the <Choose />.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Mar 10, 2016

Member

I don't understand. Why should it go to the end?

Member

forki commented Mar 10, 2016

I don't understand. Why should it go to the end?

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Mar 10, 2016

Member

ah I see. the property is not defined

Member

forki commented Mar 10, 2016

ah I see. the property is not defined

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Mar 10, 2016

Member

so this was changed in #1487 and I'm not completely sure what the correct way is

Member

forki commented Mar 10, 2016

so this was changed in #1487 and I'm not completely sure what the correct way is

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Mar 10, 2016

Member

so I guess this one is "fixed", but please help with #1487

Member

forki commented Mar 10, 2016

so I guess this one is "fixed", but please help with #1487

@forki forki closed this Mar 10, 2016

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