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

Allow disabling installing dependencies on restore #518

Merged
merged 9 commits into from
Jul 29, 2023

Conversation

tete1030
Copy link
Contributor

@tete1030 tete1030 commented Apr 25, 2023

NugetForUnity is constantly adding and installing a package, which was explicitly deleted from packages.config, when opening Unity Editor after initial clone. This package (System.Threading.Tasks.Extensions) is a dependency of another. It has duplicated definition of ValueTask class with netstandard. Deleting the package is thus necessary and has not caused any issue for me, so I have adopted this solution.

I think respecting instead of updating packages.config when restoring is a reasonable case. For this reason I added an option Lock packages on restore to disable installing dependencies of a restoring package.

I also added a timeout option for nuget mirror requests. We have used customized nuget proxy with longer http request than usual (almost 20s).

Result:
image

@JoC0de
Copy link
Collaborator

JoC0de commented Apr 25, 2023

Hi @tete1030 thanks for your contribution.
Can you please describe your issue with System.Threading.Tasks.Extensions as the option Lock packages on restore is only a workaround. Normally packages that are 'not required' should be handled by UnityPreImportedLibraryResolver so NuGetForUnity doesn't even try to install them. Do you have a example NuGet package that produces the error? What version of Unity do you use? What is your target framework (Api Compatibility Level)?

@tete1030
Copy link
Contributor Author

tete1030 commented Apr 25, 2023

I agree that it's a workaround for the System.Threading.Tasks.Extensions issue alone. But for this option itself it provides more. For example it creates stable and reproducible environments and same env with CLI restoring process.

You can try the fo-dicom 5.0.3 package. We are using Unity 2022.2.2f1 and the API compatibility level is .net standard 2.1.

To reproduce, first add some script with ValueTask class. The problem cannot be seen from Unity. It compiles well without any error. However when we open the project with VS2019 or VS2022 and try to attach to unity, the error appears in VS.

@tete1030
Copy link
Contributor Author

Do you need further information on this problem?

Copy link
Collaborator

@JoC0de JoC0de left a comment

Choose a reason for hiding this comment

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

I just was busy. So here my review. It would be nice if you also add the new feature to the documentation (Readme.md).

src/NuGetForUnity/Editor/NugetHelper.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/NugetHelper.cs Show resolved Hide resolved
src/NuGetForUnity/Editor/NugetHelper.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/NugetHelper.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/NugetConfigFile.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/NugetConfigFile.cs Outdated Show resolved Hide resolved
@tete1030
Copy link
Contributor Author

tete1030 commented May 4, 2023

Thanks for your review. I will update these and the doc. Regarding TimeSpan I have a small question to discuss. Please see the comment I replied

@RoyconSkylands
Copy link

Can I bump this?
I'm running into the same issue where a dependant package keeps comming back after I delete it and remove the packages.config reference

@JoC0de
Copy link
Collaborator

JoC0de commented Jul 27, 2023

@ConroyG OK so there is more demand. I will try to fix the last small thinks and merge the feature on the weekend.

@JoC0de JoC0de merged commit 7436d6d into GlitchEnzo:master Jul 29, 2023
8 checks passed
@tete1030
Copy link
Contributor Author

Sorry that I forgot to follow through this PR. Really appreciate the merge.

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.

3 participants