Packages install path can again be edited from preferences window #619

igor84 · 2024-01-25T15:26:39Z

No description provided.

Akeit0 · 2024-01-25T17:02:39Z

Thank you! You did a good job!
But isn't it somewhat unnatural that selecting the /Package folder generates NuGetPackages folder under it?
That's the only thing that bothered me.

It would be better to be more explicit or display a note in Preferences.

igor84 · 2024-01-25T17:04:40Z

Yes, you are right. Tomorrow I will remove that part and display an error message somewhere in the window.

Akeit0 · 2024-01-25T17:12:27Z

Do you plan to add something to explicitly generate a NuGetPackages folder?

igor84 · 2024-01-25T19:01:17Z

I was thinking to just display a message like "If you want install under Packages folder create a subfolder in it and select that one" if user just selects the Packages folder itself. I would not create NuGetPackages folder myself.

JoC0de · 2024-02-09T22:14:08Z

I think the final goal should be support a Folder structure like:

├── Packages
│   ├── nuget-packages
│   │   ├── package.json
│   │   ├── NuGet.config
│   │   ├── packages.config
│   │   ├── InstalledPackages
│   │   │   ├── <packageId>.<packageVersion>
│   │   │   ├── ...
├── Assets

There are multiple requests about how to get all the NuGetForUnity related files out of the Assets directory.
As far as I understand how unity works this should be possible as:

Unity automatically imports a package if it is placed in the Packages folder, see unity documentation Embedded package concept and Local Package Install
The folder inside the Packages folder need to be the same as the name of the embedded package and conform with the package naming convention (so it need to be lower case) see Creating a new embedded package
The embedded package package.json need to be named the same as the folder so assets inside the folder have a fixed path. If unity imports a Asset from a package it most of the time uses the package name and not the real file-path see Accessing package assets
Unity should import all assets from the embedded package so all functionalities like auto reloading if the packages.config file was changed should be possible.
The name of the nuget-packages folder need to be 'fixed' so NuGetForUnity and the CLI can find the NuGet.config file.

Eventually we can even ask for generating a .gitignore that excludes the InstalledPackages folder to help setting up the environment. Probably we can find a better name for the nuget-packages folder so it is associated to NuGetForUnity.

This PR more or less is also a step into the direction or? What do you think about this? Do you see any potential issues?

JoC0de · 2024-02-09T22:26:38Z

src/NuGetForUnity/Editor/Configuration/NugetConfigFile.cs

+        ///     Gets or sets the incomplete path that is saved.  The path is expanded and made public via the property above.
+        /// </summary>
+        [NotNull]
+        public string RelativeRepositoryPath


Suggested change

public string RelativeRepositoryPath

internal string ConfiguredRepositoryPath

The important part is that this ist the path that is stored inside the NuGet.config, it can be absolute / relative / contain environment variables. Eventually there is a even better name.

I think the original name "savedRepositoryPath" is not so bad.

Akeit0 · 2024-02-10T06:42:43Z

I think the final goal should be support a Folder structure like:

├── Packages
│   ├── nuget-packages
│   │   ├── package.json
│   │   ├── NuGet.config
│   │   ├── packages.config
│   │   ├── InstalledPackages
│   │   │   ├── <packageId>.<packageVersion>
│   │   │   ├── ...
├── Assets

This is what I wanted to do before my PR!

The folder inside the Packages folder need to be the same as the name of the embedded package and conform with the package naming convention (so it need to be lower case) see Creating a new embedded package

The embedded package package.json need to be named the same as the folder so assets inside the folder have a fixed path. If unity imports a Asset from a package it most of the time uses the package name and not the real file-path see Accessing package assets

I didn't know them.😮

The name of the nuget-packages folder need to be 'fixed' so NuGetForUnity and the CLI can find the NuGet.config file.

So it seemes that the folder and package.json.name should be something like com.github-glitchenzo.nugetforunity-packages, and its naming should not be changed in the future.

igor84 · 2024-02-12T13:35:18Z

Ok. I should change this but I don't think I can do it before my trip (I will be off from 14th to 28th February) so hopefully when I get back.

What do we want to present in Preferences? Maybe something like this:

Nuget Placement: <DropDown with options: "Custom within Assets" and "In Packages folder">

If Custom within Assets is selected we display what we show currently: "Packages Install path" and "Packages config path) and if the other option is selected we move everything under the folder structure you proposed. This way we keep backward compatibility and allow the new option.

Akeit0 · 2024-03-05T00:40:08Z

Ok. I should change this but I don't think I can do it before my trip (I will be off from 14th to 28th February) so hopefully when I get back.

What do we want to present in Preferences? Maybe something like this:

Nuget Placement: <DropDown with options: "Custom within Assets" and "In Packages folder">

If Custom within Assets is selected we display what we show currently: "Packages Install path" and "Packages config path) and if the other option is selected we move everything under the folder structure you proposed. This way we keep backward compatibility and allow the new option.

I agree with you! Any progress?

igor84 · 2024-03-05T17:21:12Z

I had a number of issues to solve in moving files around but I think I got them all now.

Akeit0 · 2024-03-06T00:47:22Z

@igor84
It works great!

But I found 2 non-serious problems.

1. If you change the "Placement" to the "In Packages Folder" while something is installed, you will need to remove focus from the UnityEditor and then refocus on the UnityEditor for the changes to take effect.
1. If you change the "Placement" to the "Custom within Assets", you will see the warning "The packages.config is placed outside of Assets folder, this disables the functionality of automatically restoring packages if the file is changed on the disk.".

Akeit0 · 2024-03-06T02:38:57Z

I found followings can solve them.

private NugetPreferences()
    : base("Preferences/NuGet For Unity", SettingsScope.User)
{
    shouldShowPackagesConfigPathWarning = !UnityPathHelper.IsPathInAssets(ConfigurationManager.NugetConfigFile.PackagesConfigDirectoryPath);
    
+    if (ConfigurationManager.NugetConfigFile.Placement == NugetPlacement.InPackagesFolder)
+    {
+        AssetDatabase.Refresh();
+    }

var newPlacement = (NugetPlacement)EditorGUILayout.EnumPopup("Placement:", ConfigurationManager.NugetConfigFile.Placement);
if (newPlacement != ConfigurationManager.NugetConfigFile.Placement)
{
    var oldRepoPath = ConfigurationManager.NugetConfigFile.RepositoryPath;
    InstalledPackagesManager.UpdateInstalledPackages(); // Make sure it is initialized before we move files around
    ConfigurationManager.MoveConfig(newPlacement);
    var newRepoPath = ConfigurationManager.NugetConfigFile.RepositoryPath;

+    if (newPlacement == NugetPlacement.CustomWithinAssets)
+    {
+       shouldShowPackagesConfigPathWarning = !UnityPathHelper.IsPathInAssets(ConfigurationManager.NugetConfigFile.PackagesConfigDirectoryPath);
+    }

igor84 · 2024-03-06T10:58:51Z

You solution would execute AssetDatabase.Refresh every time preferences is opened once Placement is set to Packages folder. I found another solution but while experimenting I discovered that if you have a source package that doesn't have asmdef you will get errors like this from unity:

Script 'Packages/test-package/Source/Test.cs' will not be compiled because it exists outside the Assets folder and does not to belong to any assembly definition file.

How should we handle this? Do nothing and let user handle that error when he sees it, write a warning about it to the user in prefs window or should we actually detect such packages and refuse the move to Packages in such cases?

Akeit0 · 2024-03-06T12:43:23Z

Oh, this change has caused DLL migration to fail.
error CS0006: Metadata file 'Assets/Packages/~~.dll' could not be found

Akeit0 · 2024-03-06T12:52:10Z

src/NuGetForUnity/Editor/Ui/NugetPreferences.cs

@@ -567,7 +553,8 @@ public override void OnGUI([CanBeNull] string searchContext)

            if (needsAssetRefresh)
            {
-                AssetDatabase.Refresh();
+                // AssetDatabase.Refresh(); doesn't work when we move the files from Assets to Packages so we use this instead:


Doing both works well!
Erasing AssetDatabase.Refresh(); causes error.

Akeit0 · 2024-03-06T13:03:45Z

You solution would execute AssetDatabase.Refresh every time preferences is opened once Placement is set to Packages folder. I found another solution but while experimenting I discovered that if you have a source package that doesn't have asmdef you will get errors like this from unity:
Script 'Packages/test-package/Source/Test.cs' will not be compiled because it exists outside the Assets folder and does not to belong to any assembly definition file.
How should we handle this? Do nothing and let user handle that error when he sees it, write a warning about it to the user in prefs window or should we actually detect such packages and refuse the move to Packages in such cases?

I think it would be better to display the warning.
However, I did not know that NuGetForUnity sometimes handles uncompiled code.
If it is very rare, you may choose to ignore it.

igor84 · 2024-03-06T14:37:04Z

Yes. Almost all internal packages in my company are source packages, they are not precompiled dlls. But they do currently all have asmdefs although we added them last year, before they didn't have asmdefs.

JoC0de · 2024-03-06T19:15:56Z

Yes. Almost all internal packages in my company are source packages, they are not precompiled dlls. But they do currently all have asmdefs although we added them last year, before they didn't have asmdefs.

I think that NuGetForUnity is used mostly to import packages from nuget.org. And if you import a Source only package and you click on "move" and your package doesn't work anymore you directly know why. The only issue a user can fall in is if he switches to "install in Packages" and than he tries to install a "Source Code" package. So we at least need a warning or better a MessageBox when the user tries to install a "Source Code" package, without a asmdef and has enabled the "Install in Packages".

igor84 · 2024-03-06T20:43:15Z

I think they will get the same error after installing source package as they will get when moving it.

JoC0de · 2024-03-06T21:18:07Z

Yes but they wouldn't know how to solve them.

igor84 · 2024-03-07T14:44:07Z

I've been thinking about this. The solution is medium complexity but I feel there is a good chance this will not be a problem in practice. I would rather that we don't solve it unless we see it really is a problem. Like people opening issues regarding this.

What do you think?

JoC0de · 2024-03-07T17:55:51Z

Seems like a legit option.

Akeit0 · 2024-03-10T07:06:05Z

I have discovered that moving the .dll.meta file is the cause of the error that sometimes occurs when moving dll files.
If I delete the .dll.meta on moving the package path, the error does not occur, but the configuration does not remain.
I'll leave it to you to decide what to do, but it should be explicitly stated to users.

igor84 · 2024-03-11T07:33:59Z

I've been testing moving packages with NewtonsoftJson dll installed the whole time and I never got any error. Do you have some specific package you want me to try this with? Without reproducing the issue I wouldn't do anything.

Akeit0 · 2024-03-11T07:51:21Z

@igor84
I installed System.Runtime.CompilerServices.Unsafe.6.0.0.
My Unity version is 2022.3.17.
EditorApplication.ExecuteMenuItem("Assets/Refresh"); causes error on changing to 'In Packages Folder' with the dll and AssetDatabase.Refresh(); doesn't.

There's no error on version 2021.3.23.

igor84 · 2024-03-12T08:49:20Z

Unity also claims they fixed this issue in 2022.3.21 but I am still waiting to see if they can provide any kind of workaround for the versions where it doesn't work.

Akeit0 · 2024-03-12T09:26:45Z

Even if an error occurs, it can be fixed by restarting the editor or prevented by reinstalling the packages, so it may not be so bad that we have to wait for an official solution.

A possible solution would be to temporarily evacuate the meta file to another location or rename it and overwrite the .dll.meta setting that was generated after the dll was successfully loaded.

igor84 · 2024-03-12T12:06:40Z

That would still cause the secondary recompile. Since it should be fixed in newer Unity I would leave it like this and only if they provide me with some workaround would I add that.

igor84 · 2024-03-19T09:02:17Z

I got this response from Unity:

I looked at the code that resolved this issue in 2022.3.21f1. The applied fix impacted how logic works on our side. We changed how and in which order import is called. This is a semi-major change, so I believe there is no actual workaround. My only advice in this situation is to add a line to your tool documentation recommending an update to 2022.3.21f1 if this happens.

Considering this I would say this is ok to be merged in this state.

JoC0de

the rest looks nice 👍
probably you can run the code formatter

src/NuGetForUnity/Editor/Configuration/NugetPlacement.cs

src/NuGetForUnity/Editor/Helper/UnityPathHelper.cs

…cation'

Akeit0 · 2024-04-03T09:33:36Z

dotnet nugetforunity restore is not supported with this option now.
Will this be solved in the next version?

igor84 · 2024-04-03T10:16:38Z

It should work. How did you determine it is not working?

Akeit0 · 2024-04-03T10:48:50Z

When I run "dotnet nugetforunity restore" with the config file in the PackagesFolder, I get a message "No NuGet.config file found. Creating default at ~~~".

Akeit0 · 2024-04-03T10:58:06Z

#630 is not supported by Cli as well.
I am looking for the 4.0.3 release.

igor84 · 2024-04-03T11:14:40Z

This feature is not present in 4.0.2 which is latest on nuget.org. Does that mean you built nugetforunity yourself from the latest master? We are yet to release a new version with this and #630 feature.

Akeit0 · 2024-04-03T12:00:42Z

I apologize for my language, which was difficult to understand.
I installed with "dotnet tool install --global NuGetForUnity.Cli" and it did not work as expected, so I commented.

Packages install path can again be edited from preferences window

25143de

igor84 mentioned this pull request Jan 25, 2024

Option to install in /Packages/NuGetAssets #618

Closed

Improved handling of invalid folder selection

beb9644

igor84 requested review from JoC0de and popara96 February 2, 2024 14:03

popara96 approved these changes Feb 2, 2024

View reviewed changes

JoC0de reviewed Feb 9, 2024

View reviewed changes

Made a Packages path fixed

9bd10c7

Fixed moving packages out of Assets

edd6104

igor84 added 2 commits March 6, 2024 12:01

Fixed Asset Refreshing when moving packages outside of Assets

480ee54

Improved asset refresh after moving to Packages folder

6a6e4c7

Akeit0 reviewed Mar 6, 2024

View reviewed changes

JoC0de requested changes Mar 24, 2024

View reviewed changes

igor84 and others added 3 commits March 25, 2024 10:23

Renamed Placement to PackageInstallLocation and core reformat

b80aa89

run autoformatter

875d576

rename configuration parameter from 'Platcement' to 'packageInstallLo…

23532ed

…cation'

JoC0de approved these changes Mar 29, 2024

View reviewed changes

igor84 merged commit 4b67af9 into master Apr 1, 2024
8 checks passed

igor84 deleted the repo-path-editing branch April 1, 2024 12:47

	public string RelativeRepositoryPath
	internal string ConfiguredRepositoryPath

Packages install path can again be edited from preferences window #619

Packages install path can again be edited from preferences window #619

Conversation

igor84 commented Jan 25, 2024

Akeit0 commented Jan 25, 2024 • edited Loading

igor84 commented Jan 25, 2024

Akeit0 commented Jan 25, 2024 • edited Loading

igor84 commented Jan 25, 2024

JoC0de commented Feb 9, 2024 • edited Loading

JoC0de Feb 9, 2024

Choose a reason for hiding this comment

Akeit0 Feb 10, 2024

Choose a reason for hiding this comment

Akeit0 commented Feb 10, 2024

igor84 commented Feb 12, 2024

Akeit0 commented Mar 5, 2024

igor84 commented Mar 5, 2024

Akeit0 commented Mar 6, 2024 • edited Loading

Akeit0 commented Mar 6, 2024 • edited Loading

igor84 commented Mar 6, 2024 • edited Loading

Akeit0 commented Mar 6, 2024 • edited Loading

Akeit0 Mar 6, 2024

Choose a reason for hiding this comment

Akeit0 commented Mar 6, 2024

igor84 commented Mar 6, 2024

JoC0de commented Mar 6, 2024

igor84 commented Mar 6, 2024

JoC0de commented Mar 6, 2024

igor84 commented Mar 7, 2024

JoC0de commented Mar 7, 2024

Akeit0 commented Mar 10, 2024

igor84 commented Mar 11, 2024

Akeit0 commented Mar 11, 2024 • edited Loading

igor84 commented Mar 12, 2024

Akeit0 commented Mar 12, 2024 • edited Loading

igor84 commented Mar 12, 2024

igor84 commented Mar 19, 2024

JoC0de left a comment • edited Loading

Choose a reason for hiding this comment

Akeit0 commented Apr 3, 2024 • edited Loading

igor84 commented Apr 3, 2024

Akeit0 commented Apr 3, 2024

Akeit0 commented Apr 3, 2024 • edited Loading

igor84 commented Apr 3, 2024 • edited Loading

Akeit0 commented Apr 3, 2024

Akeit0 commented Jan 25, 2024 •

edited

Loading

Akeit0 commented Jan 25, 2024 •

edited

Loading

JoC0de commented Feb 9, 2024 •

edited

Loading

Akeit0 commented Mar 6, 2024 •

edited

Loading

Akeit0 commented Mar 6, 2024 •

edited

Loading

igor84 commented Mar 6, 2024 •

edited

Loading

Akeit0 commented Mar 6, 2024 •

edited

Loading

Akeit0 commented Mar 11, 2024 •

edited

Loading

Akeit0 commented Mar 12, 2024 •

edited

Loading

JoC0de left a comment •

edited

Loading

Akeit0 commented Apr 3, 2024 •

edited

Loading

Akeit0 commented Apr 3, 2024 •

edited

Loading

igor84 commented Apr 3, 2024 •

edited

Loading