You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucenenet.apache.org by "paulirwin (via GitHub)" <gi...@apache.org> on 2025/09/05 02:32:36 UTC
[PR] Cache NuGet Packages (GitHub Actions) [lucenenet]
paulirwin opened a new pull request, #1172:
URL: https://github.com/apache/lucenenet/pull/1172
- [X] You've read the [Contributor Guide](https://github.com/apache/lucenenet/blob/main/CONTRIBUTING.md) and [Code of Conduct](https://www.apache.org/foundation/policies/conduct.html).
- [ ] You've included unit or integration tests for your change, where applicable.
- [ ] You've included inline docs for your change, where applicable.
- [ ] There's an open issue for the PR that you are making. If you'd like to propose a change, please [open an issue](https://github.com/apache/lucenenet/issues/new/choose) to discuss the change or find an existing issue.
Adds caching of NuGet packages to GitHub Actions.
Fixes #857
## Description
This is a re-creation of PR #866, and will experiment with using the updated caching options available in GitHub Actions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cache NuGet Packages (GitHub Actions) [lucenenet]
Posted by "NightOwl888 (via GitHub)" <gi...@apache.org>.
NightOwl888 commented on PR #1172:
URL: https://github.com/apache/lucenenet/pull/1172#issuecomment-3261813642
FYI - There is an INFRA-approved cache option called Stash that allows more than 10GB and doesn't require any special permissions to set up. It is meant to be a drop-in replacement for actions/cache.
https://github.com/apache/infrastructure-actions/blob/main/stash/README.md
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cache NuGet Packages (GitHub Actions) [lucenenet]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on PR #1172:
URL: https://github.com/apache/lucenenet/pull/1172#issuecomment-3258867131
> I am a little lost here. What about the latest IKVM requires Mono?
See: https://github.com/ikvmnet/ikvm-maven/issues/49#issuecomment-2224524388
I confirmed that when upgrading the ikvm-maven package that it tries to execute mono (at least on macOS) and fails if that is not installed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cache NuGet Packages (GitHub Actions) [lucenenet]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on PR #1172:
URL: https://github.com/apache/lucenenet/pull/1172#issuecomment-3263349544
> I propose that we change this PR to use **restore caching** only, because that will certainly have a big impact. From that point, we can decide whether it will be worth the effort to enable incremental build caching (separate PR). My best guess is that it will because we have so many parallel builds that share dependencies.
>
> I would be happy to fix the PR if you agree.
Agreed, and please proceed!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cache NuGet Packages (GitHub Actions) [lucenenet]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on PR #1172:
URL: https://github.com/apache/lucenenet/pull/1172#issuecomment-3256944097
I found a blocker to adopting the packages.lock.json files approach, at least for now.
We currently have to conditionally exclude building the net472 target on macOS (my daily driver OS, for example), because the version of ikvm-maven we use has a bug building for it. This bug has been resolved upstream, but there are two issues with updating it currently:
1. The fix requires having mono available on the system in order to build the net472 target. While not insurmountable, this is an annoying limitation and will make it more difficult for people to contribute.
2. The ikvm-maven updates target a newer version of IKVM, and apparently we can't upgrade because of [a size limitation on Azure DevOps build agents](https://github.com/apache/lucenenet/blob/f38de47d8ddac674065f04034e44c2122dd89620/src/Lucene.Net.Analysis.OpenNLP/Lucene.Net.Analysis.OpenNLP.csproj#L57-L58).
And the reason why this is a problem is that if you conditionally exclude a target framework on a platform, then the lock files get regenerated on build/restore without that target included.
While I think we need to solve the IKVM-upgrade problem (hopefully with a solution that does not involve Mono), I don't think "the juice is worth the squeeze" for this optimization at this time. I'm going to close this PR for now, but we can always resume this work later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cache NuGet Packages (GitHub Actions) [lucenenet]
Posted by "NightOwl888 (via GitHub)" <gi...@apache.org>.
NightOwl888 commented on PR #1172:
URL: https://github.com/apache/lucenenet/pull/1172#issuecomment-3264255345
In addition to setting up the NuGet restore caching functionality, this PR also addresses the following:
1. Shared cache key between the PR tests and sonar scan
2. Added `BUILD_FOR_ALL_TEST_TARGET_FRAMEWORKS` environment variable to the cache key, since our default setup only builds tests for `net9.0` by default, which is a subset of NuGet packages
3. Fixed a bug with build of `Lucene.Net.Expressions` on the `sonar.yml` workflow. The `Antlr4BuildTasks` package has a MSBuild file that depends on the ANTLR4 `.jar` file, and when it fails to find it, an [error will be thrown](https://github.com/apache/lucenenet/actions/runs/17530407782). A suitable `.jar` file is in the `Antlr4.CodeGenerator` package, but adding the `build` assets causes other build failures. So, this adds a reference that sets `PrivateAssets=all` and `ExcludeAssets=all`, and adds a bit of MSBuild logic to set the `Antlr4ToolPath` explicitly.
4. Upgrades all actions to the latest version.
5. Adds the `LiquidTestReports.Markdown` report to the GitHub Actions summary.
Do note that unlike Azure DevOps, GitHub Actions provides a [GUI to manage the cache contents](https://github.com/apache/lucenenet/actions/caches). So, there is an escape hatch if something with the caching goes wrong.
If there is a better solution to the `Lucene.Net.Expressions` build error, let me know. I couldn't find any way to disable the MSBuild scripts that were included so they wouldn't search for a `.jar` file or try to run code generation automatically.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cache NuGet Packages (GitHub Actions) [lucenenet]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on PR #1172:
URL: https://github.com/apache/lucenenet/pull/1172#issuecomment-3258245793
> I think a way around that may be to simply add the $(TargetFramework) and OS to the cache key.
That's not really the problem (or solution), sorry, I should have been clearer. If the lock files have all of our target frameworks in source control, it would have no problem restoring the packages on macOS or Linux build agents, where the cache would be used, so there's no need to differentiate that by OS AFAICT (at least not for this specific problem). The problem I was referring to above is that for _contributors building on macOS or Linux_, the lock files are regenerated on our dev machines each time we build since it doesn't have that target. That makes it at best highly annoying to avoid having to commit those files. I suppose most of the "at worst" scenarios are protected by CI builds... so perhaps this isn't a "blocker" but more of an "extremely annoying impediment to contribution."
Because of that, I'll reopen this and leave it as draft, and see if anyone has any other ideas. We can also revisit this in the future if we are able to upgrade IKVM without using Mono, accept Mono as an annoying contributor requirement (and go through the work of adding it to our CI pipelines), or (fingers crossed) drop .NET Framework support (at least for Analysis.OpenNLP) in the future.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cache NuGet Packages (GitHub Actions) [lucenenet]
Posted by "NightOwl888 (via GitHub)" <gi...@apache.org>.
NightOwl888 commented on PR #1172:
URL: https://github.com/apache/lucenenet/pull/1172#issuecomment-3258788841
> That's not really the problem (or solution), sorry, I should have been clearer. If the lock files have all of our target frameworks in source control, it would have no problem restoring the packages on macOS or Linux build agents, where the cache would be used, so there's no need to differentiate that by OS AFAICT (at least not for this specific problem). The problem I was referring to above is that for _contributors building on macOS or Linux_, the lock files are regenerated on our dev machines each time we build since it doesn't have that target. That makes it at best highly annoying to avoid having to commit those files. I suppose most of the "at worst" scenarios are protected by CI builds... so perhaps this isn't a "blocker" but more of an "extremely annoying impediment to contribution."
Right. It adds more weight to my argument that the cache key should not be based on a generated lock file, but from the original MSBuild files that specify package versions. If we do it that way
1. Adding parameters to the cache key such as OS or TFM are no big deal
2. We don't have 2 different things specifying package versions that can go out of sync
3. Contributor workflows aren't affected at all
> We can also revisit this in the future if we are able to upgrade IKVM without using Mono, accept Mono as an annoying contributor requirement (and go through the work of adding it to our CI pipelines), or (fingers crossed) drop .NET Framework support (at least for Analysis.OpenNLP) in the future.
I am a little lost here. What about the latest IKVM requires Mono?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cache NuGet Packages (GitHub Actions) [lucenenet]
Posted by "NightOwl888 (via GitHub)" <gi...@apache.org>.
NightOwl888 commented on PR #1172:
URL: https://github.com/apache/lucenenet/pull/1172#issuecomment-3263790846
The config is too complex to go through everything, but here is a representative of the difference between a cache hit and cache miss comparing apples to apples (same project, same os, same platform, same target framework). In this case, we shaved nearly 2 minutes off of the runtime.
### Cache Miss
<img width="714" height="669" alt="image" src="https://github.com/user-attachments/assets/3794ac88-7668-48a0-bfcd-e0b0673a8609" />
### Cache Hit
<img width="713" height="654" alt="image" src="https://github.com/user-attachments/assets/14713699-d8ba-4c82-9dd3-ac7d29ea7da9" />
As you can see above, the run time for build is ~35s, so we may be able to shave another 15-20s if we enable incremental build caching, although I am not sure whether that will offset the extra amount of time downloading the cache will take. Not sure it is worth the complexity. I went to find the worst case I could think of (OpenNLP) and it still only takes about 1 minute to build. The below is also a cache miss, so the runtime is still fairly short (due to the simple tests).
We gain a lot by building each project on a separate job here on GitHub Actions. However, on Azure DevOps it will probably be worth it to do both types of caching, since the build takes a significant amount of time when the solution is built all at once.
<img width="712" height="668" alt="image" src="https://github.com/user-attachments/assets/ac932c32-67bf-40c3-8270-6314ed673c18" />
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cache NuGet Packages (GitHub Actions) [lucenenet]
Posted by "NightOwl888 (via GitHub)" <gi...@apache.org>.
NightOwl888 commented on PR #1172:
URL: https://github.com/apache/lucenenet/pull/1172#issuecomment-3257596222
I think a way around that may be to simply add the `$(TargetFramework)` and OS to the cache key.
But I agree this is almost not worth the trouble. On a small project, the package restore is about half of the build time, but we have so many other build tasks going on that the time it saves on the build isn't really worth the headaches it introduces.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cache NuGet Packages (GitHub Actions) [lucenenet]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on PR #1172:
URL: https://github.com/apache/lucenenet/pull/1172#issuecomment-3268642635
GitHub won't let me approve my own PR, even though it includes your changes... so, consider this message approval 😆
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cache NuGet Packages (GitHub Actions) [lucenenet]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin merged PR #1172:
URL: https://github.com/apache/lucenenet/pull/1172
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cache NuGet Packages (GitHub Actions) [lucenenet]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin closed pull request #1172: Cache NuGet Packages (GitHub Actions)
URL: https://github.com/apache/lucenenet/pull/1172
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cache NuGet Packages (GitHub Actions) [lucenenet]
Posted by "NightOwl888 (via GitHub)" <gi...@apache.org>.
NightOwl888 commented on PR #1172:
URL: https://github.com/apache/lucenenet/pull/1172#issuecomment-3262117992
The `hashFiles()` method is OS-dependent. Or at least it is if `.gitattributes` is setup to checkout line endings differently per OS (and I think we have to based on some experiments I have tried).
https://github.com/actions/runner/issues/1246
It is simpler to include OS in the cache key than to go the `.gitattributes`/`.editorconfig` route of normalizing line endings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cache NuGet Packages (GitHub Actions) [lucenenet]
Posted by "NightOwl888 (via GitHub)" <gi...@apache.org>.
NightOwl888 commented on PR #1172:
URL: https://github.com/apache/lucenenet/pull/1172#issuecomment-3263289030
I have worked out how to successfully cache NuGet packages.
But after some research, I discovered that there are 2 caching goals that are typically employed (sometimes both at the same time):
### Restore caching
Restore caching backs up and restores the local NuGet cache to the global GitHub Actions cache. This is the most impactful cache option for small to medium sized projects, since the time it takes to build is far dwarfed by the time it takes to restore.
#### Cache key
The cache key only needs to be based on a hash of every project's configuration of `<PackageRefernce>` elements and `<PackageVersion>` elements, and the current OS.
```bash
# '**/*.*proj' includes .csproj, .vbproj, .fsproj, msbuildproj, etc.
# '**/*.props' includes Directory.Packages.props, Directory.Build.props, and Dependencies.props
# '**/*.targets' includes Directory.Build.targets
# '**/*.sln' and '*.sln' ensure root solution files are included (minimatch glitch for file extension .sln)
# 'global.json' included for SDK version changes
key: nuget-v1-${{ runner.os }}-${{ hashFiles('**/*.*proj', '**/*.props', '**/*.targets', '**/*.sln', '*.sln', 'global.json') }}
```
#### Restore keys
There is also a setting for `restore-keys`. This is meant for npm, pip, or other such package mangers where a partial hit is still valid. This approach does not usually work for NuGet caches, so the setting should be omitted.
### Incremental build caching
Incremental build caching backs up and restores the local `obj` folders (build output) to the global GitHub Actions cache. This may be impactful for large projects, particularly those with many build jobs and common dependencies, but typically for small projects with only a single build it isn't worth the effort. This strategy takes advantage of the fact that the GitHub Actions cache is **shared** between build agents.
#### Cache key
The cache key is typically based on a hash of every project's `packages.lock.json` files, project files, `global.json` file, the OS, target framework, and architecture (x64, arm64, etc).
```bash
key: build-${ runner.os }-${ matrix.tfm }-${ matrix.arch }-${ hashFiles('**/packages.lock.json', '**/*.*proj', 'global.json') }
```
For very large repositories, combining the project file hashes + `packages.lock.json` is the safest option.
#### Restore keys
In this case restore keys can make a difference when most of the projects don't change and only a few of them do. So, having a a fallback could improve performance a bit if there is a cache miss.
#### Lock files
The `packages.lock.json` files only make sense to have on the CI server for caching key purposes and there is a `dotnet restore --use-lock-file` command to create them on demand. There is no requirement to set the `RestorePackagesWithLockFile` setting or commit the lock files to the repository. So, either of these 2 options is non-intrusive:
1. Leave `RestorePackagesWithLockFile` unset and use `dotnet restore --use-lock-file` in CI
2. Set `RestorePackagesWithLockFile` and add `packages.lock.json` files to `.gitignore`
> Technically, the `RestorePackagesWithLockFile` setting must be set for `dotnet` commands run after `dotnet restore` to honor the lock files. However, for caching purposes this doesn' matter because all we care about is key consistency. In this scenario, the lock files are **cache metadata**, not for dependency pinning.
---------------------------
After looking at the configuration in this PR, it seems that it is mixing apples and oranges.
```yml
- name: Cache NuGet Packages
uses: actions/cache@v3
id: lucene-nuget-cache
with:
path: |
~/.nuget/packages
key: `${{ runner.os }}-nuget-`${{ hashFiles('**/packages.lock.json') }}
restore-keys: |
`${{ runner.os }}-nuget-lucene
- name: Restore dependencies
run: dotnet restore
```
There are a few different issues with this approach.
1. The path should not be hard coded to `~/.nuget/packages` since the canonical way to retrieve the path is the `NUGET_PACKAGES` environment variable, which is used by the `dotnet restore` and other commands.
2. Using the hash of '**/packages.lock.json' without including target framework and architecture in the cache key is problematic. But even if that were straightened out so the cache is correctly busted, there is no reason to store NuGet caches based off of more than just OS. This arrangement will create extra unnecessary duplicate caches. The bug here is that there is a mismatch between what is being cached and how it is being keyed.
4. The restore key `${{ runner.os }}-nuget-lucene` is effectively the same thing as having no key, because it will never match a prefix of the pattern `${{ runner.os }}-nuget-${{ hashFiles('**/packages.lock.json') }}`. For the fallback to work, it must be a prefix (or created like this elsewhere in GitHub Actions).
-------------------------
### Proposal
I propose that we change this PR to use **restore caching** only, because that will certainly have a big impact. From that point, we can decide whether it will be worth the effort to enable incremental build caching (separate PR). My best guess is that it will because we have so many parallel builds that share dependencies.
I would be happy to fix the PR if you agree.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org