Packages install path can again be edited from preferences window by igor84 · Pull Request #619 · GlitchEnzo/NuGetForUnity (original) (raw)

#618 (comment)

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.

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

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

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.

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?

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

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

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.

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.

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?

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

@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.".

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);

}

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?

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

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

Choose a reason for hiding this comment

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

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

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.

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.

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".

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

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

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?

Seems like a legit option.

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.

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.

@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.

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.

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.

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.

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 JoC0de left a comment • Loading

Choose a reason for hiding this comment

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

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

igor84 deleted the repo-path-editing branch

April 1, 2024 12:47

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

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

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 ~~~".

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

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.

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.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})