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

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

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

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.

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

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