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

Resolve dependencies using UnityNuGet scoped registry #9

Merged

Conversation

bdovaz
Copy link
Contributor

@bdovaz bdovaz commented Aug 15, 2023

To facilitate the installation and above all, to avoid collisions with assemblies, I add in this PR the changes with instructions included to adapt it to use this scoped registry.

By the way, I am mantainer of the mentioned repository: https://github.com/xoofx/unitynuget

@mayuki
Copy link
Member

mayuki commented Aug 16, 2023

Thank you for your pull request!

Let me explain our distribution policy including dependent libraries.

First of all, I knew there is a great project called UnityNuGet where you can get dependent packages. However, we decided not to adopt that approach in our project.

The first reason is the conflict of assemblies.
This is not a problem when originally configured with UnityNuGet, but it becomes a problem in cases where we have our own internal UPM server and managed assemblies are distributed within those packages. In such cases, it will be necessary to remove the dependency from one or the other, or to localize the package and delete the DLL.

The second reason is the responsibility for distribution and installation.
We prefer not to assume dependency on further 3rd party registries that are different from us. For instance, what if UnityNuGet has an outage?

In enterprise game development companies, for compliance reasons, they may scrutinize the libraries they are using.
In such cases, you might need to convince legal to use UnityNuGet in addition to GitHub and this repository/Organization.
Of course, this does not mean that we do not trust UnityNuGet. However, whether or not the user trusts it is different and cannot be forced.

Initially, we considered having users obtain the dependent libraries from NuGet.
However, we found that this was difficult, so we decided to redistribute using the most basic yet simple method of .unitypackage.

While UnityNuGet is convenient and provides a clean solution, it is realistically challenging to ask our customers to use it, from what we have observed with several developers.

Therefore, although we are sorry that you took the time to create a PR, we do not currently have any plans to accept methods that rely on unofficial UPMs, not just UnityNuGet.

@bdovaz
Copy link
Contributor Author

bdovaz commented Aug 16, 2023

@mayuki I understand the concerns but let me argue them and try to partly redirect the purpose of this PR with some adjustments.

Conflict resolution

I really missed the point of your explanation because I see that you agree with me that UnityNuGet is the best solution for this case.

Security concerns / Enterprise world

We develop a product which serves as a platform for XR content developers.

We solve our dependencies with UnityNuGet / OpenUPM (which does upstream https://openupm.com/nuget/) because this target of developers in our case has very little experience in development and that is why we opted for the simplest way which is the scoped registries and that everything is solved "magically" without any action to be performed by users other than the initial setup of the project by adding the scoped registries through the UI or in some cases not even that because we distribute a "template" project with everything preconfigured.

As you can understand, in our case it is crucial that all dependencies can be resolved through packages automatically because our users simply install our package and this in turn brings all the necessary dependencies either ours or third parties. And in successive updates they only have to update our package and nothing else, they don't have to deal with copying dlls by hand, etc... And if we have more advanced users who want to use other assets of OpenUPM, NuGet or Asset Store, the fact of solving everything as packages, facilitates this work enormously.

There are more special cases as you mentioned.

For these cases you can "defend" how this project (UnityNuGet) is open source, is on GitHub and is nothing "obscure" and is supported by a Unity employee (xoofx). Besides being a Docker container it is very easy to host it on your own without relying on Azure hosting by default (and then it would only be to change the url of the scoped registry in the manifest.json file). On the other hand, for cases where it is "not defensible" or external connections are not allowed, you can embed it, i.e. copy from Library/PackageCache to Packages and thus not depend on an external scoped registry.

Therefore, and if you agree, I propose the following modifications in this PR:

  1. Copy the packages from Library/PackageCache to Packages and thus not depend on the scoped registry but at the same time keep the dependencies section of the package.json file so that it can get to work directly with OpenUPM / UnityNuGet at the same time.
  2. Modify the script that exports the *.unitypackage file to do it with these new folders that are in Packages.

This way it would work as it works until now only that the assemblies instead of in Assets/Plugins will hang from Packages as embedded packages.

It also makes it easier for you to maintain this project because if you need to update the dependencies in the future you don't have to go to each NuGet page in the dependency tree and download them one by one. Just update the top-level packages (System.IO.Pipelines and System.Runtime.CompilerServices.Unsafe) and copy back from Library/PackageCache to Packages.

@bdovaz
Copy link
Contributor Author

bdovaz commented Aug 23, 2023

@mayuki what do you think of my previous message? It would allow to solve the problem this PR is trying to solve while not hurting the use cases you are concerned about.

@mayuki
Copy link
Member

mayuki commented Aug 25, 2023

Is it possible to use UnityNuGet to get the package dependencies and copy them when creating the .unitypackage (which will be extracted under Plugins)?

Also, if we use UnityNuGet to resolve dependencies, can we choose .NET Standard 2.1 as the target framework for the assembly?

@bdovaz
Copy link
Contributor Author

bdovaz commented Aug 25, 2023

  1. Yes, it is possible, it would be to copy the result of /Library/PackageCache to /Packages and use the content to generate the .unitypackage file, probably it is necessary to follow this procedure to generate the file: https://github.com/needle-tools/hybrid-packages

  2. Yes, UnityNuGet supports .NET Standard 2.0 / 2.1.

@mayuki
Copy link
Member

mayuki commented Aug 31, 2023

Sorry for the delay in response.

The idea of using UnityNuGet to resolve dependencies and packaging the result sounds good!
I would appreciate it if you could modify the PR to that approach if possible.

@bdovaz
Copy link
Contributor Author

bdovaz commented Sep 10, 2023

@mayuki the PR is ready.

I have updated the README.md indicating the 2 ways of installation: with UnityNuGet or without it. In the second case it works exactly the same as before, you have to import a *.unitypackage for YetAnotherHttpHandler dependencies and another one for Grpc.Net.Client, the difference is that now instead of importing in Assets/Plugins you import UPM packages in /Packages as I said.
2. I have updated the export script of *.unitypackage files using Hybrid Packages.

The nice thing is that the way I set it up makes it infinitely easier to lift the dependencies because you only have to edit the version in the manifest.json file for the gRPC ones and the packages.json file for the YetAnotherHttpHandler ones. You no longer have to manually download anything from the NuGet web site.

The only thing missing on your side after the merge is to release another release to be able to publish the 2 new *.unitypackage and change the links in the README.md file.

@mayuki
Copy link
Member

mayuki commented Sep 12, 2023

Thanks for the update! I will review it now.

@mayuki
Copy link
Member

mayuki commented Sep 12, 2023

It looks very nice and simple.👍

I found that the display name of the package from the unitypackage (built-in Package) looks like org.nuget.system.buffers.
Is it possible to set the displayName when UnityNuGet generates package.json?

I feel a little uncomfortable having the package IDs listed under Packages in the Package Manager and the Project View.

@bdovaz
Copy link
Contributor Author

bdovaz commented Sep 13, 2023

@mayuki If you clear the UPM cache on your computer and reimport the project from scratch, you will see that packages now have a display name.

Comment on lines 9 to 12
"dependencies": {
"org.nuget.system.io.pipelines": "7.0.0",
"org.nuget.system.runtime.compilerservices.unsafe": "4.5.3"
}
Copy link
Member

@mayuki mayuki Sep 20, 2023

Choose a reason for hiding this comment

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

If install the package by referring to git without using a scoped registry, it seems to cause an error due to these dependencies.

[Package Manager Window] Cannot perform upm operation: Unable to add package [https://github.com/bdovaz/YetAnotherHttpHandler.git?path=src/YetAnotherHttpHandler#feature/resolve-dependencies-using-unity-nuget]:
  Package com.cysharp.yetanotherhttphandler@https://github.com/bdovaz/YetAnotherHttpHandler.git?path=src/YetAnotherHttpHandler#feature/resolve-dependencies-using-unity-nuget has invalid dependencies or related test packages:
    org.nuget.system.io.pipelines (dependency): Package [[email protected]] cannot be found [NotFound].
UnityEditor.EditorApplication:Internal_CallUpdateFunctions ()

You may need to create a built-in package for dependency reference and change it to be referred from YetAnotherHttpHandler.Unity project.

Copy link
Contributor Author

@bdovaz bdovaz Sep 20, 2023

Choose a reason for hiding this comment

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

@mayuki but of course, this problem occurs if you have not imported before the *.unitypackage which is precisely the one that includes those packages so that then the error does not appear.

If you try to directly install the package without installing the dependencies this will happen.

Precisely for that reason in this PR apart from adding a second installation method (UnityNuGet) in Readme.md I have also inverted the order of the steps, that is to say, in the case of the installation without UnityNuGet I have expressly put that first you have to import the *.unitypackage file of the dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you. I understand the reason for the problem.

Our policy is to avoid creating dependencies on other packages.
This is because it becomes a problem if the dependency is already managed by another mechanism, such as internal UPM.

In our development environment, libraries are distributed under their own internal package names. For example, if System.Buffers has already been distributed/installed (e.g. com.example.system.buffers package), other dependent libraries (Grpc.Net.Client, System.Memory ...) can be moved from Packages to Plugins to ignore package dependencies, but YetAnotherHttpHandler will fail to resolve dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But what I don't understand is, this problem I didn't create it already existed.

I mean...

Actually, in the main branch it says to first import the package with Git and then import the unitypackage with the dependencies. Even if it allows you to install the Git package because the package.json has no dependencies, it will not compile because you have not imported the unitypackage file before.

That is, the order of the instructions should be reversed as I have done in this PR in the Readme.md.

And on the other hand, if a package really has dependencies why shouldn't they be put? I don't understand, that's how this UPM / NPM system works, it's the natural thing to do.

To work with OpenUPM / UnityNuGet it is necessary that the versioned package.json file has them, otherwise this PR loses all sense and all the time invested by me.

I'm really trying hard to make it work "as before" and through this OpenUPM / UnityNuGet flow that has become almost a standard among developers.

What do you suggest to keep this possible?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. My apologies for misunderstanding.

Given the use of UnityNuGet, it's unavoidable to list dependencies here.
As a result, I understand that if you install via git and without using UnityNuGet, it's necessary to install a .unitypackage that includes the org.nuget.* packages.

On the other hand, the problem I was having is when a dependency is already installed from elsewhere and the package cannot be installed by UnityNuGet or .unitypackage. Ignore the package when Unity Package Manager cannot resolve the dependency, I wish I could...

While I believe that directing to install from a .unitypackage will solve the problem in most use cases, could there be a solution to address the following case?

flowchart TB
    UnityProject-->YetAnotherHttpHandler
    UnityProject-->InternalLib

    YetAnotherHttpHandler --"org.nuget.system.buffers"--> System.IO.Pipelines
    YetAnotherHttpHandler --"org.nuget.system.runtime.compilerservices.unsafe"--> System.Runtime.CompilerServices.Unsafe

    subgraph InternalUPM
        InternalLib--"com.example.system.buffers"-->System.Buffers
    end
    subgraph .unitypackage
        System.IO.Pipelines --X--> System.Buffers
        System.IO.Pipelines --"org.nuget.system.memory"--> System.Memory
        System.IO.Pipelines --"org.nuget.system.threading.tasks.extensions"--> System.Threading.Tasks.Exensions
 
        System.Memory --X--> System.Buffers
        System.Memory --"org.nuget.system.numerics.vectors"--> System.Numerics.Vectors
        System.Memory --"org.nuget.system.runtime.compilerservices.unsafe"--> System.Runtime.CompilerServices.Unsafe
    end
Loading

Copy link
Member

Choose a reason for hiding this comment

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

The second package YetAnotherHttpHandler.Unity.Dependencies is a package solely for dependency resolution for users using UnityNuGet. It is not intended for use with OpenUPM.

The aim is to complete the installation of the package from the two URLs below after configuring UnityNuGet.

https://github.com/Cysharp/YetAnotherHttpHandler.git?path=src/YetAnotherHttpHandler
https://github.com/Cysharp/YetAnotherHttpHandler.git?path=src/YetAnotherHttpHandler.Dependencies

This proposal aims to keep an easy path for UnityNuGet users while there is a unique circumstance of not wanting to specifiy dependencies on YetAnotherHttpHandler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mayuki okay, and do you have an idea where to create it? Because that folder name you already have it reserved and it has a csproj in it.

https://github.com/Cysharp/YetAnotherHttpHandler/tree/main/src/YetAnotherHttpHandler.Unity.Dependencies

Copy link
Member

Choose a reason for hiding this comment

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

The current YetAnotherHttpHandler.Unity.Dependencies is a development helper tool project for copying DLLs.
I think it's okay to replace its contents because its role is ended by using UnityNuGet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mayuki seeing the YetAnotherHttpHandler.Unity.Dependencies.Sync project loses its sense no? Because it is already going to be solved from UnityNuGet.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's correct, it will become completely unnecessary.

@bdovaz
Copy link
Contributor Author

bdovaz commented Nov 3, 2023

@mayuki check my changes because I think everything we talked about is already resolved.

@bdovaz bdovaz requested a review from mayuki November 7, 2023 20:52
@mayuki mayuki merged commit 6cf6104 into Cysharp:main Nov 8, 2023
12 checks passed
@bdovaz bdovaz deleted the feature/resolve-dependencies-using-unity-nuget branch November 8, 2023 05:59
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.

2 participants