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

Help Roslyn-Project-System understand why infinite restore loop may happen with bad package. #4542

Closed
rrelyea opened this Issue Feb 9, 2017 · 13 comments

Comments

Projects
None yet
5 participants
@rrelyea
Contributor

rrelyea commented Feb 9, 2017

@natidea asked for help from us in dotnet/project-system#1457
This is not blocking RTM, so tracking an issue in 4.0.1.

@emgarten

This comment has been minimized.

Show comment
Hide comment
@emgarten

emgarten Mar 9, 2017

Collaborator

I was able to repro this on VS 2017 RTM after a customer reported it

Steps

  1. New NET Core console app
  2. Install LLVMSharp 3.8.0

Auto restore will run infinitely. Closing and re-opening the solution leads to the same problem.

The only workaround is to turn off automatic package restore under the VS options.

Collaborator

emgarten commented Mar 9, 2017

I was able to repro this on VS 2017 RTM after a customer reported it

Steps

  1. New NET Core console app
  2. Install LLVMSharp 3.8.0

Auto restore will run infinitely. Closing and re-opening the solution leads to the same problem.

The only workaround is to turn off automatic package restore under the VS options.

@emgarten emgarten modified the milestones: 4.1, Future-0 Mar 9, 2017

@emgarten

This comment has been minimized.

Show comment
Hide comment
@emgarten

emgarten Mar 9, 2017

Collaborator

This should be considered for 4.1

Collaborator

emgarten commented Mar 9, 2017

This should be considered for 4.1

@jainaashish

This comment has been minimized.

Show comment
Hide comment
@jainaashish

jainaashish Mar 9, 2017

Contributor

I don't think NuGet can do much about it, if Project is being nominated in a infinite loop then it should be fixed at project system end. Even though it's a bad behavior but still NuGet shouldn't work around this.

Also, little more details about this issue, LLVMSharp 3.8.0 actually has some build target which tamper the project system which makes it keep nominating this again and again. Package authors has already removed these build targets with their latest prerelease versions so hopefully this bad package will be corrected soon. But until then Project system should be more resilient to this kind of behavior.

Contributor

jainaashish commented Mar 9, 2017

I don't think NuGet can do much about it, if Project is being nominated in a infinite loop then it should be fixed at project system end. Even though it's a bad behavior but still NuGet shouldn't work around this.

Also, little more details about this issue, LLVMSharp 3.8.0 actually has some build target which tamper the project system which makes it keep nominating this again and again. Package authors has already removed these build targets with their latest prerelease versions so hopefully this bad package will be corrected soon. But until then Project system should be more resilient to this kind of behavior.

@emgarten

This comment has been minimized.

Show comment
Hide comment
@emgarten

emgarten Mar 9, 2017

Collaborator

@jainaashish Agreed on the project system being smarter, but isn't there a noop mechanism in place? Why isn't it working? And should we also keep track so that we can give up even if the last restore failed but the next request has the same inputs?

Collaborator

emgarten commented Mar 9, 2017

@jainaashish Agreed on the project system being smarter, but isn't there a noop mechanism in place? Why isn't it working? And should we also keep track so that we can give up even if the last restore failed but the next request has the same inputs?

@natidea

This comment has been minimized.

Show comment
Hide comment
@natidea

natidea Mar 10, 2017

The project system has a noop check:

            if (!updates.Any(u => u.Value.ProjectChanges.Any(c => c.Value.Difference.AnyChanges)))
            {
                return null;
            }

However this check is failing because one of the properties we listen for is changing. I took a look under a debugger, and I see:

image

which leads to a nomination. Then the next time the noop check fails, I see:

image

So something is modifying the package references. This also means each new restore will be called with different inputs.

natidea commented Mar 10, 2017

The project system has a noop check:

            if (!updates.Any(u => u.Value.ProjectChanges.Any(c => c.Value.Difference.AnyChanges)))
            {
                return null;
            }

However this check is failing because one of the properties we listen for is changing. I took a look under a debugger, and I see:

image

which leads to a nomination. Then the next time the noop check fails, I see:

image

So something is modifying the package references. This also means each new restore will be called with different inputs.

@natidea

This comment has been minimized.

Show comment
Hide comment
@natidea

natidea Mar 10, 2017

@jainaashish what is the target in LLVMSharp 3.8.0 that is tampering with the project system?

natidea commented Mar 10, 2017

@jainaashish what is the target in LLVMSharp 3.8.0 that is tampering with the project system?

@natidea

This comment has been minimized.

Show comment
Hide comment
@natidea

natidea Mar 10, 2017

I also notice that the generated *.nuget.g.props file changes each time. Following the 'add' cycle, the following is added to props:

  <ImportGroup Condition=" '$(ExcludeRestorePackageImports)' != 'true' ">
    <Import Project="$(NuGetPackageRoot)microsoft.diasymreader.native\1.4.1\build\Microsoft.DiaSymReader.Native.props" Condition="Exists('$(NuGetPackageRoot)microsoft.diasymreader.native\1.4.1\build\Microsoft.DiaSymReader.Native.props')" />
  </ImportGroup>

This entry is removed following the 'remove' cycle. A similar thing happens to the generated targets file.

natidea commented Mar 10, 2017

I also notice that the generated *.nuget.g.props file changes each time. Following the 'add' cycle, the following is added to props:

  <ImportGroup Condition=" '$(ExcludeRestorePackageImports)' != 'true' ">
    <Import Project="$(NuGetPackageRoot)microsoft.diasymreader.native\1.4.1\build\Microsoft.DiaSymReader.Native.props" Condition="Exists('$(NuGetPackageRoot)microsoft.diasymreader.native\1.4.1\build\Microsoft.DiaSymReader.Native.props')" />
  </ImportGroup>

This entry is removed following the 'remove' cycle. A similar thing happens to the generated targets file.

@emgarten

This comment has been minimized.

Show comment
Hide comment
@emgarten

emgarten Mar 10, 2017

Collaborator

microsoft.diasymreader.native is a dependency of netcore app. Is the project nominating with 0 package references? NuGet may be clearing out everything when it runs the restore on 0 packages, and then putting them all back again when it nominates the 2 references again.

Collaborator

emgarten commented Mar 10, 2017

microsoft.diasymreader.native is a dependency of netcore app. Is the project nominating with 0 package references? NuGet may be clearing out everything when it runs the restore on 0 packages, and then putting them all back again when it nominates the 2 references again.

@natidea

This comment has been minimized.

Show comment
Hide comment
@natidea

natidea Mar 10, 2017

The first restore is the 'add' cycle. It nominates with 2 package references:
image
This also means the first time the props/targets are generated they contain the ImportGroup I mention above.

The second restore is the 'remove' cycle. It nominates with 0 package references. That is the problematic one. So we've got to figure out what is removing the package references. My guess is either something in the generated props/targets, or else something off with the CPS subscription.

natidea commented Mar 10, 2017

The first restore is the 'add' cycle. It nominates with 2 package references:
image
This also means the first time the props/targets are generated they contain the ImportGroup I mention above.

The second restore is the 'remove' cycle. It nominates with 0 package references. That is the problematic one. So we've got to figure out what is removing the package references. My guess is either something in the generated props/targets, or else something off with the CPS subscription.

@natidea

This comment has been minimized.

Show comment
Hide comment
@natidea

natidea Mar 10, 2017

I think I've found the offending target:

  <Target Name="PlatformCheck" BeforeTargets="ResolveAssemblyReferences"
    Condition=" ( ('$(Platform)' != 'x86') AND ('$(Platform)' != 'x64') AND ('$(OutputType)' != 'Library') )">
    <Error Text="LLVMSharp requires the platform target for the solution and/or project to be either x86 or x64 to PInvoke appropriately." />
  </Target>

This target is emitting an error that is stopping the build early. (If I change to a Warning, the infinite loop goes away). I suspect this is caused by our move to using design-time builds to compute rule values. Here is my theory on what is happening:

  • A design time build occurs without props/targets, rules are computed, subscriptions are fired, and NuGet is nominated
  • After NuGet restores, it adds generated props/targets
  • The next time a design build occurs, it includes the props/targets which cause cause the build to terminate early with an error. But somehow rules are still computed, and subscriptions are fired, but this time the PackageReferences are not resolved, so it triggers a nomination to indicate that the package references were removed. This in turn updates the props/targets to remove the offending target.
  • And the cycle continues...

@davkean does this seem plausible given the way CPS handles properties?

natidea commented Mar 10, 2017

I think I've found the offending target:

  <Target Name="PlatformCheck" BeforeTargets="ResolveAssemblyReferences"
    Condition=" ( ('$(Platform)' != 'x86') AND ('$(Platform)' != 'x64') AND ('$(OutputType)' != 'Library') )">
    <Error Text="LLVMSharp requires the platform target for the solution and/or project to be either x86 or x64 to PInvoke appropriately." />
  </Target>

This target is emitting an error that is stopping the build early. (If I change to a Warning, the infinite loop goes away). I suspect this is caused by our move to using design-time builds to compute rule values. Here is my theory on what is happening:

  • A design time build occurs without props/targets, rules are computed, subscriptions are fired, and NuGet is nominated
  • After NuGet restores, it adds generated props/targets
  • The next time a design build occurs, it includes the props/targets which cause cause the build to terminate early with an error. But somehow rules are still computed, and subscriptions are fired, but this time the PackageReferences are not resolved, so it triggers a nomination to indicate that the package references were removed. This in turn updates the props/targets to remove the offending target.
  • And the cycle continues...

@davkean does this seem plausible given the way CPS handles properties?

@davkean

This comment has been minimized.

Show comment
Hide comment
@davkean

davkean Mar 10, 2017

Nice investigation.

Yeah - failed design-time builds will trigger the data flow to fire even if the target you are calling isn't executed. We had similar issues in the language service there and there we assume no data == failed design-time builds, and we don't push the data to Roslyn.

I'm not sure if you can do it in this case, as you might not be able to tell the difference between no data and no references. If you can, I suspect you should do the same thing. Really, I want CPS to handle this - and not having individual consumers having to understand that the design-time build failed.

davkean commented Mar 10, 2017

Nice investigation.

Yeah - failed design-time builds will trigger the data flow to fire even if the target you are calling isn't executed. We had similar issues in the language service there and there we assume no data == failed design-time builds, and we don't push the data to Roslyn.

I'm not sure if you can do it in this case, as you might not be able to tell the difference between no data and no references. If you can, I suspect you should do the same thing. Really, I want CPS to handle this - and not having individual consumers having to understand that the design-time build failed.

@rrelyea

This comment has been minimized.

Show comment
Hide comment
@rrelyea

rrelyea Mar 15, 2017

Contributor

@natidea - Sounds like we can close our issue to help you guys. Let us know if you need more help.

Contributor

rrelyea commented Mar 15, 2017

@natidea - Sounds like we can close our issue to help you guys. Let us know if you need more help.

@rrelyea

This comment has been minimized.

Show comment
Hide comment
@rrelyea

rrelyea Apr 3, 2017

Contributor

Looks like dotnet/project-system#1457 is currently scheduled for 15.3 milestone. So this issue still occurs, but appears to be waiting for a fix from Roslyn-Project-System.

Contributor

rrelyea commented Apr 3, 2017

Looks like dotnet/project-system#1457 is currently scheduled for 15.3 milestone. So this issue still occurs, but appears to be waiting for a fix from Roslyn-Project-System.

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