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

Make nuget restore errors non fatal #3072

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Conversation

dupdob
Copy link
Member

@dupdob dupdob commented Oct 21, 2024

Slight change: make nuget failures non fatal (log error but continue), as it makes no sense for the general case, especially since selecting the proper msbuild and nuget combination is tricky on complex environment.

Having a proper handling of nuget version would be better, but not sure it is worth the needed effort

Fixes #3067.

@rouke-broersma
Copy link
Member

Nuget restore is not optional iirc because buildalyzer performs a clean which deletes the packages, I'm not sure this would be a valid scenario?

@dupdob
Copy link
Member Author

dupdob commented Oct 21, 2024

Well, in practice, Stryker only make a package restore attempt if the analysis is failed. Judging by the log the author of #3067 provided me, Nuget restore did not even happen in V4.0.6 for his/her project. It happens only with V4.1+ (because 1 project analysis fails for some reason). In any case, this results in aborting the whole mutation session, which is a shame.

Digging into Nuget issues, I discovered that 'unable to find MsBuild version xxx' was a recurrent issue with no clear solution nor workaround, so I am not sure we can fix this properly.

As an alternative, we could also retry a failed nuget session WITHOUT specifying the msbuild version.
But should we abort is this also fails?

@dupdob
Copy link
Member Author

dupdob commented Oct 21, 2024

remark: I did not notice there was a test explicitly testing for failure; my test runner (NCrunch) does not support yet the platform conditional attributes 😄 , so this test is not visible on my ide.
My bad

@rouke-broersma
Copy link
Member

Well, in practice, Stryker only make a package restore attempt if the analysis is failed. Judging by the log the author of #3067 provided me, Nuget restore did not even happen in V4.0.6 for his/her project. It happens only with V4.1+ (because 1 project analysis fails for some reason). In any case, this results in aborting the whole mutation session, which is a shame.

Digging into Nuget issues, I discovered that 'unable to find MsBuild version xxx' was a recurrent issue with no clear solution nor workaround, so I am not sure we can fix this properly.

As an alternative, we could also retry a failed nuget session WITHOUT specifying the msbuild version. But should we abort is this also fails?

Yes that makes sense. We added the nuget version because of other conflicts between nuget version and msbuild version, but if it fails regardless we might retry without a specific version.

If I remember correctly we used to even do restore -> analyze -> restore because analyze would always fail without restore, but the analzse would clear the nuget packages and then compilation would not work. Not sure if this is still what we do nor if it's still relevant, but iirc this is how we first made dotnet framework support functional.

But now that I think about it, the restore might only be mandatory for projects that use nuget.config + .packages folder instead of PackageReference and global package cache, so for modern (dotnet framework) sdk style projects it might no longer be required for analyze.

@dupdob
Copy link
Member Author

dupdob commented Oct 21, 2024

If I remember correctly we used to even do restore -> analyze -> restore because analyze would always fail without restore, but the analzse would clear the nuget packages and then compilation would not work. Not sure if this is still what we do nor if it's still relevant, but iirc this is how we first made dotnet framework support functional.

I remember it too.

But now that I think about it, the restore might only be mandatory for projects that use nuget.config + .packages folder instead of PackageReference and global package cache, so for modern (dotnet framework) sdk style projects it might no longer be required for analyze.

I made some attempts to discover when nuget was needed when I rewrote the project analysis phase (hoping to get rid of it altogether): failed to understand when and why it was needed. But it is still sometimes needed. 🤷‍♂️

I added a retry logic without msbuildversion.

Copy link

sonarcloud bot commented Oct 21, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MSBuild could not be found - issue with NuGet
2 participants