-
Notifications
You must be signed in to change notification settings - Fork 249
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
StaticGraph Restore should clearly call out when a project is not KnownToBeMSBuildFormat
When Called via a Solution File
#10363
Comments
@jeffkl Responding to #10307 (comment)
What about a compromise? A warning if the |
@nkolev92 and the NuGet team will need to decide how to handle this issue. I'm technically not on the NuGet team, I contributed this feature from another team within Microsoft. Any information that can be provided is very helpful to a user. There are other warnings within NuGet that list possible reasons that something isn't working. This code path could log something that says:
|
Just to be clear this bug is subtle if you have a large solution (3,000+ Projects) and a portion of those do not have their restore ran due to this. This is why the heuristic needs to be |
I think adding additional logging to improve the diagnosability of static graph problems is a great idea. |
Yeah I am good with that. Is it too far of an ask to have the warning point to a page to report missing projects? Not sure if there is a precedence for that? It might help encourage ISV's to do "the right thing" or at least vote for the "escape hatch" dotnet/msbuild#5931 Again want to stress that the check needs to find the "one or two missing projects in a sea of hundreds", many times project extensions are small "add-ons" to large existing C# code bases (think of like an interop library), they would easily be lost in the shuffle. Is there any type of telemetry you can pick up to see how many times this gets hit? |
We can't really tell how many customers are hitting this at this point. Retitling to capture the logging improvements. |
KnownToBeMSBuildFormat
When Called via a Solution FileKnownToBeMSBuildFormat
When Called via a Solution File
@nkolev92 Can you work on the wording for this issue, what is intended? |
KnownToBeMSBuildFormat
When Called via a Solution FileKnownToBeMSBuildFormat
When Called via a Solution File
The goals here should be:
|
See related: dotnet/runtime#45755. |
Static graph currently crashes with circular dependencies in play and doesn't log this piece (which non static graph restore does):
|
@ViktorHofer Or are you saying that during a static graph based build, a project fails with an exception and that doesn't get logged? |
Right, the static graph nuget restore console app is crashing and doesn't log anything. Uncertain but I think the build even continues afterwards. |
That definitely sounds like an issue. Please open a separate issue. Not enough data to tell if the problem is with msbuild or static graph restore |
Static graph restore shouldn't be running that target. I'm unfamiliar with static graph beyond restore though, so I might be missing some context. |
Right, that target runs only without static graph restore which logs the circular dependency. With static graph restore enabled, restore just silently fails and nothing is logged. |
That part is definitely an issue. Root cause unknown. |
The PR associated with this issue has been merged. MSBuild static graph restore displays the following additional log messages at default verbosity i.e.
|
I think is where the actual issue is:
https://github.com/NuGet/NuGet.Client/blob/223189bbc21e8259e6771d363e69d37d9b10e693/src/NuGet.Core/NuGet.Build.Tasks.Console/MSBuildStaticGraphRestore.cs#L508
In addition there is also another bug in here (if you want I can file another bug report):
@jeffkl, what happens if the project doesn't report itself as
SolutionProjectType.KnownToBeMSBuildFormat
?We had to have our third party submit this PR dotnet/msbuild#5698 (Thanks @madkat) to get this vendor's project extension in the "blessed list" but not all people who extend the project system are going to be as on top of it to actually file a bug, much less a pull request.
This has wide ranging consequences if you intend to make this the default as proposed by #9803 and more discussion at NuGet/docs.microsoft.com-nuget#1941. Note that this is the SAME class of bug as noted here dotnet/msbuild#5159 for full blown /graph that @cdmihai should be aware of as well.
Right or wrong tons of ISV's have extended the project system in unexpected ways, there needs to be a better way to resolve this, the gains are super worth it (@madkat please feel free to share the gains you've seen on our code base and others if you can), and we are super grateful that you guys are pushing this forward, but there are growing pains for sure.
Originally posted by @aolszowka in #10307 (comment)
@jeffkl Found an old Visual Studio box that doesn't have the pulled PR; compare the following (same project/solution I probably messed up the xxxx in the sanitation between the two so ignore that):
Visual Studio 2019 MSBuild 16.7.4:
Visual Studio 2019 MSBuild 16.8.2 (this has the patch applied to allow this third party type to be recongized as a
KnownToBeMSBuildFormat
type:As I described above, if you don't register yourself as
KnownToBeMSBuildFormat
you get lost when it attempts to perform a Graph based restore.For our particular usecase with our third party we're OK, how do other ISV's know to do this?
Originally posted by @aolszowka in #10307 (comment)
The text was updated successfully, but these errors were encountered: