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

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

Closed
DamianEdwards opened this issue Jun 6, 2016 · 17 comments
Closed
Assignees
Labels
Bug
Milestone

Comments

@DamianEdwards
Copy link

@DamianEdwards 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
Copy link
Contributor

@davidfowl 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
Copy link

@yishaigalatzer 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
Copy link
Contributor

@davidfowl 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
Copy link

@yishaigalatzer yishaigalatzer commented Jun 6, 2016

@davidfowl agreed. We are taking a look right now

@joelverhagen
Copy link
Member

@joelverhagen 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
Copy link

@yishaigalatzer 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
Copy link
Contributor

@davidfowl 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
Copy link

@yishaigalatzer 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?
@davidfowl
Copy link
Contributor

@davidfowl 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
Copy link
Contributor

@TheRealPiotrP TheRealPiotrP commented Jun 10, 2016

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

@clairernovotny
Copy link
Member

@clairernovotny clairernovotny 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
Copy link
Contributor

@TheRealPiotrP TheRealPiotrP commented Jun 10, 2016

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

@clairernovotny
Copy link
Member

@clairernovotny clairernovotny 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.

clairernovotny referenced this issue in Humanizr/Humanizer Jul 4, 2016
@jorgeyanesdiez
Copy link

@jorgeyanesdiez jorgeyanesdiez commented Sep 14, 2016

This still fails in v1.0.0 final, sadfaes.

@brthor brthor assigned TheRealPiotrP and unassigned brthor Oct 21, 2016
@karelz
Copy link
Member

@karelz karelz commented Jan 9, 2017

@binoypatel hit the problem: https://github.com/dotnet/corefx/issues/14998#issuecomment-271415006 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
Copy link
Contributor

@TheRealPiotrP 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
Copy link
Contributor

@TheRealPiotrP TheRealPiotrP commented Jan 20, 2017

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

@msftgits msftgits transferred this issue from dotnet/cli Jan 31, 2020
@msftgits msftgits added this to the 1.0.0-rtm milestone Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
10 participants
You can’t perform that action at this time.