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

feat: Cache NuGet global-packages folder #303

Merged
merged 35 commits into from
May 29, 2023
Merged

feat: Cache NuGet global-packages folder #303

merged 35 commits into from
May 29, 2023

Conversation

nogic1008
Copy link
Contributor

@nogic1008 nogic1008 commented Aug 1, 2022

Description

This PR adds the feature to cache NuGet global-packages folder. (inspired by actions/setup-node)

Usage

env:
  NUGET_PACKAGES: ${{ github.workspace }}/.nuget/packages # optional, but recommended
steps:
  - uses: actions/checkout@v3
  - uses: actions/setup-dotnet@v2
    with:
      dotnet-version: '6.0.x'
      cache: true # default: false

This feature needs to NuGet lock file (**/packages.lock.json).

By default, global-packages folder path is %userprofile%\.nuget\packages (Windows) or ~/.nuget/packages (Mac/Linux).
Depending on host runners, this folder may already contain huge files. (like Xamarin)
Therefore, if there is no problem, we recommend that you specify the NUGET_PACKAGES environment variable to create your own cache folder.

See also: https://docs.microsoft.com/nuget/consume-packages/managing-the-global-packages-and-cache-folders

Tasks

  • Source code
  • Build assets
  • Unit tests
  • End-to-end tests
  • Documentation

Related issue

None

Check list

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

@nogic1008 nogic1008 marked this pull request as ready for review October 18, 2022 04:33
@nogic1008
Copy link
Contributor Author

nogic1008 commented Oct 18, 2022

To maintainer
Please review the license for these packages.

@IvanZosimov IvanZosimov self-requested a review November 17, 2022 08:26
@IvanZosimov IvanZosimov self-assigned this Nov 17, 2022
@IvanZosimov
Copy link
Contributor

IvanZosimov commented Nov 23, 2022

Hi. @nogic1008, thanks a lot for that huge PR! Could you clarify a little the list of caching directories? In the PR description, you wrote that you are going to cache the global-packages directory. But in the code, I see that you call dotnet nuget locals all --list --force-english-output which shows all 4 cache-related directories as stated here: https://learn.microsoft.com/en-us/nuget/consume-packages/managing-the-global-packages-and-cache-folders Do we want to cache all of them?

@IvanZosimov
Copy link
Contributor

Hi, @nogic1008 just a gentle ping 📟

Copy link

@rohthegreat rohthegreat left a comment

Choose a reason for hiding this comment

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

I Approve.

README.md Show resolved Hide resolved
@panticmilos
Copy link
Contributor

hi @nogic1008, could you sync with the main branch?

@panticmilos
Copy link
Contributor

hi @nogic1008, just a gentle ping :)

@nogic1008 nogic1008 requested a review from a team as a code owner March 29, 2023 15:13
@nogic1008
Copy link
Contributor Author

Hi. @nogic1008, thanks a lot for that huge PR! Could you clarify a little the list of caching directories? In the PR description, you wrote that you are going to cache the global-packages directory. But in the code, I see that you call dotnet nuget locals all --list --force-english-output which shows all 4 cache-related directories as stated here: https://learn.microsoft.com/en-us/nuget/consume-packages/managing-the-global-packages-and-cache-folders Do we want to cache all of them?

@IvanZosimov
As you say, getNuGetFolderPath() returns 4 paths but this features only use global-cache path.
It is left for future enhancements. (ex. cache other folders)

@IvanZosimov
Copy link
Contributor

Hi, @nogic1008 👋 Thank you for the update! Could you, please, sync your PR with the main branch? There are some new changes related to the e2e tests, check this PR to learn more.

@nogic1008
Copy link
Contributor Author

@IvanZosimov
I have rebased to the latest main branch, it's ready for review.
I don't want to rebase too many times because there are big diffs in this PR.

action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/cache-utils.ts Outdated Show resolved Hide resolved
src/cache-utils.ts Outdated Show resolved Hide resolved
src/cache-utils.ts Outdated Show resolved Hide resolved
src/cache-utils.ts Outdated Show resolved Hide resolved
src/cache-utils.ts Outdated Show resolved Hide resolved
@IvanZosimov
Copy link
Contributor

IvanZosimov commented Apr 26, 2023

Hi, @nogic1008 👋 Thanks for the update, I left several comments, please take a look. Also, may I ask you to add e2e tests?

@nogic1008
Copy link
Contributor Author

nogic1008 commented Apr 27, 2023

@IvanZosimov
Thank you for your review. I have resolved some points.

  • add end-to-end tests for cache feature
    • I added on previous commit, but I missed it when resolving conflict.
  • fix README
  • fix getNuGetFolderPath
    • change Regex parttern to support .NET Core 2.1
  • fix isCacheFeatureAvailable
    • call core.warning() instead of throw Error
    • extern isGHES
  • change main script path to setup/
    • To resolve it, we need to change unit test for installer.ts. It is difficult for me because it is not "unit" test but calls install script directly.
  • Support cache-dependency-path
    • Please give me some time because I need to implement it anew.

@IvanZosimov
Copy link
Contributor

Thanks for the update @nogic1008 ❤️! Regarding the unit tests for the installer.ts, I will help you. When you change the logic, I'll fix the tests by myself. Don't worry at all!

src/setup-dotnet.ts Outdated Show resolved Hide resolved
@IvanZosimov
Copy link
Contributor

Hi, @nogic1008 👋 Could you, please, resolve merge conflicts? As I see we got all necessary approvals, as soon as you resolve the conflicts we can make final tests and then we are ready to go 🚀

@nogic1008
Copy link
Contributor Author

@IvanZosimov
Resolved merge conflict. Ready to go.

@IvanZosimov
Copy link
Contributor

Hi, @nogic1008 👋 During testing, I found a problem, my test project failed to restore dependencies using dotnet restore --locked-mode with error NU1403. As we suggest dotnet restore --locked-mode command to customers, I think it's important to give them a small note that in case of error NU1403 while restoring a project's dependencies, they can add this setting in the .csproj file and the problem will be solved. Could you share your opinion about this point? If you agree, could you add this info as a > **Note** to the README.md file ?

P.S. There are also some merge conficts, sorry for that. May I ask you to solve them one more time ? 😇 

src/cache-restore.ts Outdated Show resolved Hide resolved
@nogic1008
Copy link
Contributor Author

@IvanZosimov
Resolved merge conflicts.
And add guide on README to resolve NU1403 error.
Please review this section.

@IvanZosimov IvanZosimov merged commit 3447fd6 into actions:main May 29, 2023
@IvanZosimov
Copy link
Contributor

Hi, @nogic1008 👋 The PR is merged 🚀 Thanks a lot for your willingness to make our action better! ❤️

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.

10 participants