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

Localized satellite packages are incorrectly marked as incompatible by the CLI #3396

Closed
DamianEdwards opened this Issue Jun 6, 2016 · 17 comments

Comments

Projects
None yet
10 participants
@DamianEdwards
Collaborator

DamianEdwards commented Jun 6, 2016

Steps to reproduce

Create a console application and add a reference to "Humanizer": "2.0.1", then run dotnet restore

Expected behavior

Restore completes successfully.

Actual behavior

CLI states that the satellite packages aren't compatible with netcoreapp1.0
This seems to be due to those packages only contained resources, and no runtime, ref or lib assets, and the CLI currently doesn't account for this (see https://github.com/dotnet/cli/blob/rel/1.0.0/src/Microsoft.DotNet.ProjectModel/Resolution/PackageDependencyProvider.cs#L35-L38)

Environment data

dotnet --info output:

.NET Command Line Tools (1.0.0-preview1-002702)

Product Information:
 Version:     1.0.0-preview1-002702
 Commit Sha:  6cde21225e

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.10586
 OS Platform: Windows
 RID:         win10-x64
@davidfowl

This comment has been minimized.

Show comment
Hide comment
@davidfowl

davidfowl Jun 6, 2016

Collaborator

/cc @yishaigalatzer @rrelyea @emgarten Can we expose the nuget compatibility check via an API that the CLI can use?

Collaborator

davidfowl commented Jun 6, 2016

/cc @yishaigalatzer @rrelyea @emgarten Can we expose the nuget compatibility check via an API that the CLI can use?

@yishaigalatzer

This comment has been minimized.

Show comment
Hide comment
@yishaigalatzer

yishaigalatzer Jun 6, 2016

We don't have actual public APIs on the NuGet packages (as in nothing we can commit to be fully stable), but we can definitely share how to run the check. @joelverhagen can you drop a sample here?

yishaigalatzer commented Jun 6, 2016

We don't have actual public APIs on the NuGet packages (as in nothing we can commit to be fully stable), but we can definitely share how to run the check. @joelverhagen can you drop a sample here?

@davidfowl

This comment has been minimized.

Show comment
Hide comment
@davidfowl

davidfowl Jun 6, 2016

Collaborator

We have a check but it's out of date. It would be great if we could call into something, even if it wasn't a good API. The cli and nuget ship in the same app so it's impossible to update nuget outside of the cli (as a cli user) so the risk of breaking is low

Collaborator

davidfowl commented Jun 6, 2016

We have a check but it's out of date. It would be great if we could call into something, even if it wasn't a good API. The cli and nuget ship in the same app so it's impossible to update nuget outside of the cli (as a cli user) so the risk of breaking is low

@yishaigalatzer

This comment has been minimized.

Show comment
Hide comment
@yishaigalatzer

yishaigalatzer Jun 6, 2016

@davidfowl agreed. We are taking a look right now

yishaigalatzer commented Jun 6, 2016

@davidfowl agreed. We are taking a look right now

@joelverhagen

This comment has been minimized.

Show comment
Hide comment
@joelverhagen

joelverhagen Jun 6, 2016

Member

I don't think we have a public API for this right now. The code certainly exists (CompatibilityChecker.cs), but it is marked as internal.

We should keep this issue open in dotnet/cli as well as expose a friendly API in NuGet: NuGet/Home#2911.

Member

joelverhagen commented Jun 6, 2016

I don't think we have a public API for this right now. The code certainly exists (CompatibilityChecker.cs), but it is marked as internal.

We should keep this issue open in dotnet/cli as well as expose a friendly API in NuGet: NuGet/Home#2911.

@yishaigalatzer

This comment has been minimized.

Show comment
Hide comment
@yishaigalatzer

yishaigalatzer Jun 6, 2016

@davidfowl now that I think about it, why does the CLI even do this check? If restore succeeds it means the packages are compatible. Can you elaborate a bit?

yishaigalatzer commented Jun 6, 2016

@davidfowl now that I think about it, why does the CLI even do this check? If restore succeeds it means the packages are compatible. Can you elaborate a bit?

@davidfowl

This comment has been minimized.

Show comment
Hide comment
@davidfowl

davidfowl Jun 6, 2016

Collaborator

Nuget does not persist all of the results of restore. We need the build and the IDE to show nice errors when a restore fails. Today this works in UWP because restore runs on build. In the cli, we try to infer diagnostics from the lock file

Collaborator

davidfowl commented Jun 6, 2016

Nuget does not persist all of the results of restore. We need the build and the IDE to show nice errors when a restore fails. Today this works in UWP because restore runs on build. In the cli, we try to infer diagnostics from the lock file

@yishaigalatzer

This comment has been minimized.

Show comment
Hide comment
@yishaigalatzer

yishaigalatzer Jun 6, 2016

  1. I still don't understand why we try to see if a package "fits" when restore succeeds?
  2. We need to work on consolidating the two systems so we don't need duplicate code paths to "figure things out". Perhaps the right thing is to capture all restore errors, which we do write to the output and surface them instead?

yishaigalatzer commented Jun 6, 2016

  1. I still don't understand why we try to see if a package "fits" when restore succeeds?
  2. We need to work on consolidating the two systems so we don't need duplicate code paths to "figure things out". Perhaps the right thing is to capture all restore errors, which we do write to the output and surface them instead?

@TheRealPiotrP TheRealPiotrP added this to the 1.0.0-preview2 milestone Jun 6, 2016

@TheRealPiotrP TheRealPiotrP added the bug label Jun 6, 2016

@davidfowl

This comment has been minimized.

Show comment
Hide comment
@davidfowl

davidfowl Jun 6, 2016

Collaborator

I still don't understand why we try to see if a package "fits" when restore succeeds?

Restore succeeds and always writes the lock file (regardless of failure). Build and the project model server want to show the same errors that restore showed. To do that, it tries to repeat the compatibility check to make sure things got resolved.

We need to work on consolidating the two systems so we don't need duplicate code paths to "figure things out". Perhaps the right thing is to capture all restore errors, which we do write to the output and surface them instead?

That's the entire purpose of this request. The errors need to be persisted in some format, preferably the lock file since that's what we use for everything else.

Collaborator

davidfowl commented Jun 6, 2016

I still don't understand why we try to see if a package "fits" when restore succeeds?

Restore succeeds and always writes the lock file (regardless of failure). Build and the project model server want to show the same errors that restore showed. To do that, it tries to repeat the compatibility check to make sure things got resolved.

We need to work on consolidating the two systems so we don't need duplicate code paths to "figure things out". Perhaps the right thing is to capture all restore errors, which we do write to the output and surface them instead?

That's the entire purpose of this request. The errors need to be persisted in some format, preferably the lock file since that's what we use for everything else.

@TheRealPiotrP

This comment has been minimized.

Show comment
Hide comment
@TheRealPiotrP

TheRealPiotrP Jun 10, 2016

Collaborator

@yishaigalatzer @rrelyea I see NuGet/Home#2911 is not scheduled yet. Do you expect this to make CLI preview2?

Collaborator

TheRealPiotrP commented Jun 10, 2016

@yishaigalatzer @rrelyea I see NuGet/Home#2911 is not scheduled yet. Do you expect this to make CLI preview2?

@onovotny

This comment has been minimized.

Show comment
Hide comment
@onovotny

onovotny Jun 10, 2016

Member

Note that this completely breaks Humanizer from working correctly for CLI projects (and any other libraries that use satellite languages).

Member

onovotny commented Jun 10, 2016

Note that this completely breaks Humanizer from working correctly for CLI projects (and any other libraries that use satellite languages).

@TheRealPiotrP

This comment has been minimized.

Show comment
Hide comment
@TheRealPiotrP

TheRealPiotrP Jun 10, 2016

Collaborator

@brthor can you coordinate with @joelverhagen on this one?

Collaborator

TheRealPiotrP commented Jun 10, 2016

@brthor can you coordinate with @joelverhagen on this one?

@onovotny onovotny referenced this issue Jul 3, 2016

Closed

Ship Humanizer 2.1 #567

5 of 5 tasks complete
@onovotny

This comment has been minimized.

Show comment
Hide comment
@onovotny

onovotny Jul 3, 2016

Member

Anything on this? When is preview 3 schedule for? This completely blocks Humanizer from working on xproj. It does appear to install on a csproj that's netstandard.

Member

onovotny commented Jul 3, 2016

Anything on this? When is preview 3 schedule for? This completely blocks Humanizer from working on xproj. It does appear to install on a csproj that's netstandard.

onovotny added a commit to Humanizr/Humanizer that referenced this issue Jul 4, 2016

@jorgeyanesdiez

This comment has been minimized.

Show comment
Hide comment
@jorgeyanesdiez

jorgeyanesdiez Sep 14, 2016

This still fails in v1.0.0 final, sadfaes.

jorgeyanesdiez commented Sep 14, 2016

This still fails in v1.0.0 final, sadfaes.

@karelz

This comment has been minimized.

Show comment
Hide comment
@karelz

karelz Jan 9, 2017

Member

@binoypatel hit the problem: dotnet/corefx#14998 (comment) and dotnet/standard#152 (comment)

My question is now should I move my production application to VS2017 or continue working with VS2015 until VS2017 RTMed?

@piotrpMSFT what would be your advice?

Member

karelz commented Jan 9, 2017

@binoypatel hit the problem: dotnet/corefx#14998 (comment) and dotnet/standard#152 (comment)

My question is now should I move my production application to VS2017 or continue working with VS2015 until VS2017 RTMed?

@piotrpMSFT what would be your advice?

@TheRealPiotrP

This comment has been minimized.

Show comment
Hide comment
@TheRealPiotrP

TheRealPiotrP Jan 20, 2017

Collaborator

Good news :)

I just tried this with CLI RC3 bits and successfully added Humanizer to a project. Caveats, of course, but solvable by Humanizer itself!

Details:
CLI 1.0.0-rc4-004536

  • dotnet new
  • manually add <PackageTargetFallback>$(PackageTargetFallback);portable-net45+win8+wp8+wpa81</PackageTargetFallback> to the csproj file. This is because Humanizer 2.0.1 is a PCL-style nupkg. I checked nuget.org and saw that 2.1.0 is the same, but this is something to solve in Humanizer packaging
  • 'dotnet restore'
  • 'dotnet build'

The above was a successful. I hope @onovotny can take a look and confirm!

Collaborator

TheRealPiotrP commented Jan 20, 2017

Good news :)

I just tried this with CLI RC3 bits and successfully added Humanizer to a project. Caveats, of course, but solvable by Humanizer itself!

Details:
CLI 1.0.0-rc4-004536

  • dotnet new
  • manually add <PackageTargetFallback>$(PackageTargetFallback);portable-net45+win8+wp8+wpa81</PackageTargetFallback> to the csproj file. This is because Humanizer 2.0.1 is a PCL-style nupkg. I checked nuget.org and saw that 2.1.0 is the same, but this is something to solve in Humanizer packaging
  • 'dotnet restore'
  • 'dotnet build'

The above was a successful. I hope @onovotny can take a look and confirm!

@TheRealPiotrP

This comment has been minimized.

Show comment
Hide comment
@TheRealPiotrP

TheRealPiotrP Jan 20, 2017

Collaborator

@karelz I would not be doing any new work atop project.json at this time...

Collaborator

TheRealPiotrP commented Jan 20, 2017

@karelz I would not be doing any new work atop project.json at this time...

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