-
Notifications
You must be signed in to change notification settings - Fork 255
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
StaticGraphRestore Fails with NETSDK1004 in Some Scenarios #10307
Comments
Can you repro this on a single leaf project without any dependencies? Try running just restore and then static restore and post the logs from the runs? Do you set |
Did not appear so with the one project I tried
Can you clarify this request a bit more? I read this as doing the following:
Which restores all packages (For 98 projects in the solution) Then cleaning the output (
Which yields the following:
Restored nothing? Not sure if that is expected?
We don't explicitly; but this imports targets up the wazoo (including Xamarin) looking at the bin log:
Not sure who set this? (wasn't us) |
Weird, this suggests to me something went wrong and NuGet did not even start the restore. Can you do verbose logs? Nothing rings a bell right now.
There are defaults, and that's normal. That property basically tells you where the assets file should go. |
Sure is there something in particular I can give to you? Or some private way to share the *.binlog (which would probably be way more useful?) |
You can send it to nikolev [at] microsoft [dot] com Verbose logs & binlog from the restore not happening might help. |
Both have been provided as well as a binlog where staticgraph is not used (and it works without it) |
@nkolev92 any chance to look at those logs? |
Hey @aolszowka I was out all of last week, I haven't gotten a chance yet. I'll get to it this week. |
Hey @aolszowka Unfortunately the logs are not telling me enough where I can figure out what's going wrong. Here's a rundown on what I'd do to help us diagnose further.
With regular restore, you can see said dg spec by running You can try running on your sln. My idea would be to make static graph write an equivalent file, and then we can compare those 2 files. Alternatively, if you willing to debug, you can always follow: |
Running Fails with:
Attempting to define it as this: Yields an incredibly complex file that will take me a couple of minutes to sanitize for public consumption, is this expected? EDIT Here's the sanitized Gist: https://gist.github.com/aolszowka/efb0cd7620b4bee3df2117b2232705b8 |
Hey @aolszowka, yes the complex file is the one that's interesting. Basically comparing that file from traditional msbuild walk restore, should be equivalent to the one generated by static graph restore. Would you be willing to run a custom build static graph restore if I were to start a branch for that? |
@nkolev92 I'm ready and willing, lets do this! |
The easiest way to do this is to build the NuGet.Client locally. I have added an environment variable that'll help you generate the equivalent of Steps:
You should be able to generate both dg spec files and the difference between them will be a good start in helping us understand what goes wrong. |
@nkolev92 Not going to lie man fighting though this is a trip; is this really how it is on the inside? Like there isn't just an Azure DevOps Pipeline drop somewhere for this? for your branch? that you could just point me to? Unreal! Right off the bat trying to run After defining the environment variable as defined above I try running I hit the wall at 120mph with this gem:
Any way to disable this? Like and oldschool CAS policy overload that says YOLO on the signatures? Probably related to the multitude of errors I just sped past because you can't stop in bat country? |
I will get you a vsix that you can install. I went with the approach that didn't require you to install anything to your VS instance. Bad assumption by me, the drop should hopefully make it easier. |
Sounds great, let me know and I'll install it right away, I am not usually tied to my VS instances, I'll just snapshot and roll back if I'm that concerned. |
Here's a link to the vsix. To install, just double click or run the vsixinstaller.exe on the file. To revert back, follow https://github.com/NuGet/NuGet.Client/blob/dev/docs/debugging.md#uninstalling-a-custom-version-of-the-nuget-extension-from-visual-studio. |
@nkolev92 locked out :( I'll grab it as soon as the share link is fixed. |
Argghhh...I got bit by one drive again...try this |
The files have a single difference; there is NOTHING in the Restore section! |
and the rest of the For example, all of them say Nothing obvious...I'll dig more to see if I can understand how this might happen. |
I was referring to that question, why a project might be left out of restore when static graph based versus not. |
@jeffkl I no longer have a pre-16.8 Visual Studio Install handy, the only third party library we utilize that was not a |
@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
|
Yeah the discussion around One possible workaround, depending on the scenario is to move away from solution files all together and use Traversal projects. You restore these with static graph restore and they don't look at the project type in the same way as a solution file. |
@jeffkl What part of the discussion? I already opened up a bug for Using Traversal Projects is great, however we've got ~4,000 Solutions to Convert (Yeah, not great). Is there any tooling along the lines of |
My point is that if you're not in the blessed list of KnownToBeMSBuildFormat, you're going to hit more and more roadblocks even if we just make it work with static graph restore. So dotnet/msbuild#5931 is the correct path to take to get the best long term fix. I'm not sure if the NuGet team should work around it by removing the check, as not checking could be a breaking change where projects used to be left out of restore and now they would be included which could lead to errors. I'll leave that decision up to the NuGet team. In my opinion, it is not a bug that projects that are not KnownToBeMSBuild are left out of restore. This is the intended behavior. I do not know of an existing tool that would create a Traversal project out of a solution file, but if you have any knowledge of the MSBuild construction APIs, I would not expect it to be too difficult. |
Fair enough, we'll move discussion on that issue to the linked one (#10363). Back to this issue, these ARE supported types, who does this go to? |
I've read through this issue and just to confirm, we think the root cause is that this line's |
@jeffkl That is correct (at least what our debugging shows, seems like an Solution bug?) Why are we only affected by it in some scenarios? |
Is there any way for you to create a small repro of the issue with a simple solution file and maybe two projects? That would be extremely useful. If |
I can dig a bit more, but to be honest this solution has 98 projects (and a single leaf node did not repro it). It'd be great if this solution was smaller that I could throw it through Delta (https://www.st.cs.uni-saarland.de/dd/) to automate this. I'll give it a shot.
As much as this stinks this is where I am at with this bug, sprinkling a |
I think a workaround is a decent approach. NuGet and MSBuild ship at the same time though so fixing it in MSBuild will achieve the same outcome in the same amount of time. |
Thank you for the investigation to you both!
Can you manually try to edit out the 97 projects and see if it repros?
Where in particular would you recommend that? Overall, I do think we need some additional logging on a more verbose level to ensure we're not silently dropping projects. Removing the |
I would vote for it right here: https://github.com/NuGet/NuGet.Client/blob/223189bbc21e8259e6771d363e69d37d9b10e693/src/NuGet.Core/NuGet.Build.Tasks.Console/MSBuildStaticGraphRestore.cs#L508 Changed to: return
solutionFile
.ProjectsInOrder
.Where(i => i.ProjectType == SolutionProjectType.KnownToBeMSBuildFormat)
.Select(i => new ProjectGraphEntryPoint(Path.GetFullPath(i.AbsolutePath), globalProperties))
.ToList(); Note the addition of the I would be willing to test out a patch on this.
I had originally tried this above, but let me try a few others when I get access back to this project tomorrow morning.
That is fair, for us we are OK today (we got our PR accepted into MSBuild), I was just trying to help the next guy down the road, especially if you plan to turn on static graph by default. I want to see this feature succeed and not get stymied by unexpected users of the ecosystem. |
We appreciate all the help here. Regarding the patch idea, if it's a relative path, we probably need to know what to resolve it against right? |
I am not sure I understand the question? The folder structure looks something like this: src
I have dug on this, I am able to get this to repro with a single one of our projects, HOWEVER I still cannot get it to repro if I create a new project from scratch, here's what our SLN Looks like (minimized):
Some type of bug where going up a folder from the solution itself is mad? |
Woohoo I was finally able to repro the issue. The problem is that this line: Adds to a list containing paths like this:
Projects are then added to the dependency graph spec with the correct full path, but then this line looks for the projects by path: And the dictionary lookup fails because the paths are in two different formats. I'll discuss with some people and see what the right fix is. |
@jeffkl correct this was pointed out here #10307 (comment) and the comment proceeding it.... What is unknown is WHY that is coming back with a non-fully qualified path. Do you have an answer for that? |
Yes the real question is: Should MSBuild's SolutionFile API return a normalized full path when you call the AbsolutePath property? I'm asking around here to see what MSBuild/NuGet want to do... |
@nkolev92 I'll be fixing this in dotnet/msbuild#5950, can you please mark this issue as External? And remove the NeedsRepro tag. |
Closing as External per last comment. |
Details about Problem
NuGet product used: MSBuild /restore
OS version (i.e. win10 v1607 (14393.321)): Windows Server 1809 17763.1397
Worked before?: Yes, without StaticGraph
Detailed repro steps so we can see the same problem
Unclear on what exactly is happening here, but when using
/p:RestoreUseStaticGraphEvaluation=true
to get Static Graph we error with the following:Not using Static Graph (IE not defining the switch) works just fine.
We're motivated to help solve the issue, but we are constrained as to giving you code that repros (we don't have time to narrow it down blindly, please give guidance)
The text was updated successfully, but these errors were encountered: