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

Embed NuGet files in binlog after restore #5042

Merged
merged 3 commits into from Feb 23, 2023

Conversation

jeffkl
Copy link
Contributor

@jeffkl jeffkl commented Feb 7, 2023

Bug

Fixes: NuGet/Home#12374

Regression? No Last working version:

Description

This is a follow-up of #5016 but now it includes the files from all projects that took part in restore.

This updates the Restore task to embed NuGet files in the binlog after Restore is complete. Even if restore fails, any files that exist will be embedded:

  • project.assets.json
  • ProjectFile.csproj.g.nuget.props
  • ProjectFile.csproj.g.nuget.targets
  • ProjectFile.csproj.dgspec.json

image

In this case the package name is wrong and the .dgspec is still in the binlog.

For static graph-based restore, the output paths are logged back to the RestoreTaskEx so it can embed the files in the binlog at the end of the actual build. I could not find a way to embed them in the nuget.binlog that contains evaluation...

Related to dotnet/sdk#16840

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception - It would be hard to verify the contents of the binlog at the moment.
    • OR
    • N/A
  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

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.

Looks pretty good!

One small suggestion about handling packages.config projects too.

If we're doing it now, might as well get it done fully :)

@jeffkl jeffkl force-pushed the dev-jeffkl-embed-nuget-files-in-binlog branch from 0936da1 to 93f8eb4 Compare February 10, 2023 21:25
@aortiz-msft aortiz-msft self-requested a review February 11, 2023 01:14
Copy link
Contributor

@aortiz-msft aortiz-msft left a comment

Choose a reason for hiding this comment

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

Could you please add the files to be included for each case as part of the PR description?

Copy link
Member

@zivkan zivkan left a comment

Choose a reason for hiding this comment

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

❤️ this feature!

nkolev92
nkolev92 previously approved these changes Feb 15, 2023
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. @zivkan had some feedback, but looks great other than that.

nkolev92
nkolev92 previously approved these changes Feb 21, 2023
zivkan
zivkan previously approved these changes Feb 22, 2023
@jeffkl jeffkl dismissed stale reviews from zivkan and nkolev92 via f8bd8a9 February 22, 2023 18:18
@jeffkl jeffkl force-pushed the dev-jeffkl-embed-nuget-files-in-binlog branch from 8206cce to f8bd8a9 Compare February 22, 2023 18:18
@jeffkl jeffkl enabled auto-merge (squash) February 22, 2023 18:18
@jeffkl jeffkl merged commit 0c92ce3 into dev Feb 23, 2023
@jeffkl jeffkl deleted the dev-jeffkl-embed-nuget-files-in-binlog branch February 23, 2023 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants