Delete lock files on non-Windows platforms. #4123
Conversation
ghost
added
the
|
When I run |
There was a problem hiding this comment.
Firstly, in the future please don't delete the PR template. We have the template because we want people to use it, and we have tools that parse it automating parts of our end to end process (like release notes) that would otherwise be more manual.
Secondly, unfortunately it doesn't work. Steps to reproduce
- Either use a Linux VM or WSL2.
git clone https://github.com/NuGet/NuGet.Client- Edit
src/NuGet.Core/NuGet.Common/ConcurrencyUtilities.csto useDeleteOnCloseinstead ofNonefor Mac/Linux. Also comment out all usage ofPerFileLock. (reason: otherwise the filesystem lock is only used in multi-process synchornisation, making it much more difficult to test) - cd
/test/NuGet.Core.Tests/NuGet.Common.Tests dotnet build- run
dotnet test --no-builda few times
The tests fail. On my machine, it fails more common than it passes, but because it's a timing issue, the tests do still pass sometimes.
tmds
force-pushed
the
deleteonclose
branch
2 times, most recently
from
957d427 to
57276d8
Compare
|
@zivkan I'll keep the template next time. |
There was a problem hiding this comment.
You can edit the first comment in this PR and copy the template from here
Good news is that my testing (with only a single process) didn't show any performance regression. Testing methodology using OrchardCore, as it has a large number of projects and packages:
## Test with PR's changes
cd NuGet.Client
git checkout deleteonclose
dotnet build -c Release src/NuGet.Core/NuGet.Build.Tasks
cd ../OrchardCore
# repeat numerous times
git clean -xdf ; dotnet nuget locals all --clear
time dotnet restore -p:NuGetRestoreTargets=/path/to/NuGet.Client/artifacts/NuGet.Build.Tasks/bin/Release/netcoreapp5.0/NuGet.targets
## Test baseline
cd ../NuGet.Client
git checkout HEAD~1
dotnet build -c Release src/NuGet.Core/NuGet.Build.Tasks
cd ../OrchardCore
# repeat numerous times
git clean -xdf ; dotnet nuget locals all --clear
time dotnet restore -p:NuGetRestoreTargets=/path/to/NuGet.Client/artifacts/NuGet.Build.Tasks/bin/Release/netcoreapp5.0/NuGet.targets
On my PC, in WSL2, restore times were the same, before and after.
tmds
changed the title
|
@zivkan I've just realized that this change will break NuGet restores that run in parallel with but do not have this fix. 😞 |
|
We can start using the mutex now, but don't delete the file on Linux/Mac unless an environment variable like |
|
CI is stuck. |
I think that for this repository the CI needs to be manually run by a repo maintainer 🤷 |
|
I'm looking at improving what the framework does for |
|
I'm waiting to see how dotnet/runtime#55327 progresses. If that PR gets merged, then this PR doesn't need the global lock or the |
|
@tmds congratulations on getting dotnet/runtime#55327 merged! I think this means we don't need to use the global mutex in this PR, it can be a simple on/off switch. What do you think? |
Yes, I'll look into this next week. |
|
CI is taking long. Is it stuck? |
|
It just doesn't run automatically. |
There was a problem hiding this comment.
CI is taking long. Is it stuck?
As @nkolev92 mentioned, it doesn't run automatically. For what it's worth, it's documented in our contribuition guidelines document: https://github.com/NuGet/NuGet.Client/blob/dev/CONTRIBUTING.md#contributing-step-by-step
PRs from forks do not trigger CI automatically. Someone in the team needs to apply the "Approved for CI", which will build only the current commit. If changes are pushed to the branch, the "Approved for CI" label needs to be removed and re-applied.
test/NuGet.Core.Tests/NuGet.Synchronization.Test/SynchronizationTest.cs
Outdated
Show resolved
Hide resolved
test/NuGet.Core.Tests/NuGet.Synchronization.Test/SynchronizationTest.cs
Outdated
Show resolved
Hide resolved
test/NuGet.Core.Tests/NuGet.Synchronization.Test/SynchronizationTest.cs
Outdated
Show resolved
Hide resolved
|
@nkolev92 @zivkan I tried to address your feedback. Unfortunately I cannot build or test this on my machine. The When I try to manually run the test project, it requires an EOL .NET version: |
|
@zivkan I've removed the changes to the test. ptal. |
|
Change looks fine to me. We should ensure that the tooling will be running on the latest runtime version before merging right? |
This PR isn't introducing a change in behavior unless When .NET 6.0 goes EOL we can start unconditionally deleting the lock files. |
|
Coincidentally we discussed this PR briefly yesterday. At the time we thought that adding a Right now, I don't have any better ideas than to merge as-is, and if customers shoot themselves in the foot and then complain, we just tell them to stop using the environment variable. @NuGet/nuget-client again, I plan on merging within a few days unless someone else has feedback they want to provide. |
Bug
Fixes: NuGet/Home#10679.
Regression? No
Description
PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
No new tests added. Functional behavior is covered by existing tests.