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

Use new serializer library to parse solution files #6219

Merged
merged 7 commits into from
Jan 24, 2025

Conversation

baronfel
Copy link
Contributor

@baronfel baronfel commented Jan 10, 2025

Because the library is async-only, this required a bit of bubbling-up of async signatures and CancellationToken-passing through the List and Why commands.

Bug

Fixes: NuGet/Home#14034
Contributes to dotnet/sdk#40913

Description

Minimal work to

  • add the new library
  • Use it when loading solutions
  • Expand the set of extensions that are recognized as 'solution-like'
  • Flow through viral async-ness through the why and package list commands.
  • Update existing tests due to internal API changes
  • Add new tests showing successful usage of list package with the slnx format

Note that this does not remove all usage of the now-deprecated SolutionFile MSBuild API in NuGet (the only location of which I could find was GetProjectGraphEntryPoints - that seemed a bit above my knowledge level (though conceptually should also be simple).

PR Checklist

  • Meaningful title, helpful description and a linked NuGet/Home issue
  • Added tests
  • Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Community PRs created by someone not in the NuGet team label Jan 10, 2025
@baronfel baronfel marked this pull request as ready for review January 10, 2025 22:35
@baronfel baronfel requested a review from a team as a code owner January 10, 2025 22:35
Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Some minor feedback.
We need a change for source build as well.

Directory.Packages.props Show resolved Hide resolved
@baronfel
Copy link
Contributor Author

@zivkan do you have any idea what's up with the tests on the single failing leg?

@zivkan
Copy link
Member

zivkan commented Jan 11, 2025

I was going to write that there are a (large?) number of Nuget's tests that are just a little bit flakey, but when I looked at the test's history, this specific test doesn't have any other failures in the test history. Plus, the stack trace shows the error is coming from shows it's coming from Interop.Sys.GetCwd, which almost certainly means the Environment.CurrentDirectory API (or something equivalent), which doesn't seem like it should be possible to fail. So, I'm tempted to say this is some kind of flakiness in the .NET runtime. But I also can't prove that the test directory wasn't somehow deleted before the current directory API was called.

Anyway, I clicked the button to retry failed tests, so it'll probably pass on retry.

@zivkan
Copy link
Member

zivkan commented Jan 11, 2025

Well, it failed again on the same test with the same error, which increases the chance that it might somehow be related to something in this PR, but I'm struggling to imagine what it could possibly be ☹️

@baronfel
Copy link
Contributor Author

I was going to write that there are a (large?) number of Nuget's tests that are just a little bit flakey, but when I looked at the test's history, this specific test doesn't have any other failures in the test history. Plus, the stack trace shows the error is coming from shows it's coming from Interop.Sys.GetCwd, which almost certainly means the Environment.CurrentDirectory API (or something equivalent), which doesn't seem like it should be possible to fail. So, I'm tempted to say this is some kind of flakiness in the .NET runtime. But I also can't prove that the test directory wasn't somehow deleted before the current directory API was called.

Anyway, I clicked the button to retry failed tests, so it'll probably pass on retry.

Hypothesis: More async-stuff is happening with this PR, so if base-platform level syscalls require thread synchronization it's possible that might need to be adhered to? Maybe sprinkling .ConfigureAwait(false) everywhere there's new async code in this PR?

@zivkan
Copy link
Member

zivkan commented Jan 13, 2025

Hypothesis: More async-stuff is happening with this PR, so if base-platform level syscalls require thread synchronization it's possible that might need to be adhered to? Maybe sprinkling .ConfigureAwait(false) everywhere there's new async code in this PR?

dotnet nuget locals is a console app command, just like all other dotnet cli commands, which doesn't use a SynchronizationContext. Therefore, I'd expect all async work to be happening on ThreadPool.Default already. .ConfigureAwait(false) should therefore be a no-op, given my understanding of how async works on .NET.

Personally, I think it's more likely that some method that was made async is not being awaited, allowing some method to exit before the async work happens. This makes a good case for always renaming methods that are made async to append the Async suffix, because if there are any places where the call is not modified, you'll get a compiler error, making it very clear you need to update the method name being called, in addition to adding the await keyword.

However, I don't see any files modified that dotnet nuget locals might be using. So, maybe my advice should be ignored because I have no idea what's going on 🤣

The fact that the test also fails only on Mac, not on Linux or Windows makes it even more mysterious. I keep writing questions like "have you tried running the test locally", but unless you have a Mac, it doesn't really matter. It's a fresh day today, but I didn't get any new ideas on how to diagnose or fix the failing test.

@jeffkl jeffkl self-assigned this Jan 14, 2025
@microsoft-github-policy-service microsoft-github-policy-service bot added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Jan 22, 2025
@jeffkl
Copy link
Contributor

jeffkl commented Jan 23, 2025

@baronfel we had to merge a commit to fix CI, when you have time please rebase your branch and push, or let me know if you want me to do it on your behalf.

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Jan 23, 2025
@baronfel
Copy link
Contributor Author

Thanks for the poke @jeffkl - I've rebased it now.

@baronfel
Copy link
Contributor Author

@jeffkl it looks like the windows tests failed on a new test I added. Progress! The tests triggered hang dump generation, but I don't think the dumps were collected:

  Results File: D:\a\_work\_temp\cloudtest_a160b5fcc000000_2025-01-23_23_32_51.trx
  
  Attachments:
    D:\a\_work\_temp\fb9a4a04-24c0-4ece-9f18-d3b953f1b4da\testhost_4224_20250123T233936_hangdump.dmp
    D:\a\_work\_temp\fb9a4a04-24c0-4ece-9f18-d3b953f1b4da\Sequence_c3fe1a81c93a4f98adce715806e0e90c.xml

Is it possible for those to be collected?

@jeffkl
Copy link
Contributor

jeffkl commented Jan 24, 2025

@baronfel
Copy link
Contributor Author

@jeffkl perfect, thank you for the pointer!

@baronfel
Copy link
Contributor Author

As expected, it's the new test (specifically the slnx test case) that's failing here:

<Test Name="NuGet.XPlat.FuncTest.ListPackageTests.CanListPackagesForProjectsInSolutions" DisplayName="NuGet.XPlat.FuncTest.ListPackageTests.CanListPackagesForProjectsInSolutions(useSlnx: True)" Source="D:\a\_work\1\s\test\NuGet.Core.FuncTests\NuGet.XPlat.FuncTest\bin\Debug\net8.0\NuGet.XPlat.FuncTest.dll" Completed="False" />

All other cases are completing.

@baronfel
Copy link
Contributor Author

@jeffkl !!! It's green, quick merge it!

@nkolev92
Copy link
Member

@jeffkl !!! It's green, quick merge it!

How did you end up fixing the test failure?

I don't see anything right now.
Did it just go away?

@baronfel
Copy link
Contributor Author

Yeah, a re-run cleared it up.

@baronfel
Copy link
Contributor Author

This is too late for 9.0.200 GA at this point, but is this the kind of thing that would be amenable to a servicing backport? The rest of the SDK will have great support for slnx in 9.0.200 and I'm concerned about users having a pit of failure with package management as they onboard to slnx.

@nkolev92
Copy link
Member

It's not quite servicing since it has not GA yet, so "technically" just a QB insertion.

I'd suggest reaching out to @aortiz-msft about that.

@jeffkl jeffkl merged commit 20096ab into NuGet:dev Jan 24, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community PRs created by someone not in the NuGet team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for new slnx solution format
5 participants