NuGet / Home

Repo for NuGet Client issues
Other
1.5k stars 252 forks source link

Can't override path to NuGet\Migrations #12712

Open keddad opened 1 year ago

keddad commented 1 year ago

NuGet Product Used

MSBuild.exe, NuGet SDK

Product Version

6.0.410

Worked before?

6.0.301

Impact

I'm unable to use this version

Repro Steps & Context

It appears that since PR #422933 to NuGet.Client, a special folder is created for processing migrations in nuget packages. This logic is handled by GetMigrationsDirectory method. On non-win platforms, location is determined by XDG_DATA_HOME variable, but under Windows, Environment.GetFolderPath method is used.

For Environment.SpecialFolder.LocalApplicationData, it appears to always return a "user_directory"\AppData\Local folder. (from what I can gather, it calls a Win32 method for that). After that, the folder is created there. Since migrations are used in build/restore tasks (on recent SDK versions), the folder is created there when building anything. However, when building in sanboxed enviroments, build tools might not be allowed to write to arbitrary locations, and lack of way to move "Migrations" folder elsewhere poses a problem.

It seems like this was not intended: in Migration1.cs file, NuGetEnvironment.GetFolderPath is used instead. Looking at it's implementation, it appears to allow folder override using an environment variable.

The fix might be as simple as replacing Environment.GetFolderPath with NuGetEnvironment.GetFolderPath in MigrationRunner.cs.

To reproduce the issue, just build anything with recent SDK (tested on 6.0.410, but looking at the release dates, the PR might be in release versions since 6.0.402) on Windows. The folder will be created, with no way to override it.

Verbose Logs

No response

kartheekp-ms commented 1 year ago

Thanks for filing this issue. We could change the implementation as shown below, but I am curious why the build tools are not allowed to the write to this location. If I am not wrong, when dotnet command is executed for the first time on an agent machine, it creates NuGet.Config file under %userprofile%\AppData\Roaming\NuGet directory on Windows platforms. When dotnet restore is executed by default the packages are unzipped into a global packages folder i.e., %userprofile%\.nuget\packages. The %localappdata% folder is also used by default on Windows to maintain v3-cache, plugins-cache along with migrations folder.

        internal static string GetMigrationsDirectory()
        {
            string appDataPath = NuGetEnvironment.GetFolderPath(NuGetEnvironment.SpecialFolder.LocalApplicationData);
            string migrationsDirectory = Path.Combine(appDataPath, "NuGet", "Migrations");

            Directory.CreateDirectory(migrationsDirectory);

            return migrationsDirectory;
        }
keddad commented 1 year ago

We are building C# applications in a sanboxed environment (using Bazel). When ran this way, build tools can only write arbitrary (like the mentoined folder) files to TEMP (otherwise, builds might not be reproducible). I'm not sure about other folders you mentioned, but I'm guessing that we override their location too.

It does break nuget cache, but we don't really care about it - we download all the nugets separately (which are cached by Bazel), and then just make them locally available to dotnet during build.

nkolev92 commented 1 year ago

I'm not sure about other folders you mentioned, but I'm guessing that we override their location too.

@keddad Can you please confirm that you're overriding these locations? Certain folders such as the plugin cache do not have a means to overwrite either, but you might not be hitting any issues with this if you're not using private Azure DevOps feeds.

keddad commented 1 year ago

I've checked, and on SDK we are currently using (6.0.301) none of the folders mentioned (v3-cache, plugins-cache, migrations, Migrations) are created, in any location. They are not even touched - if you record a build with procmon, no events with these strings in path exist.

On a newer SDK (6.0.410) the only folder I can see accessed is Migrations. Other folders are not used. (Interestingly, I can also see that AppData\Local\Microsoft\Windows\INetCache is opened, and it wasn't opened on older version, however, since nothing is written, it should be OK). Migrations folder is not even used - it is created (which breaks sandboxing), but if you disable sandbox and let it write wherever, it just leaves an empty file named 1 there.

I'd guess that those folders are not created because no Nugets are actually downloaded (with Azure DevOps feeds or without) - since we download and unpack nugets ourselves (with Bazel), we make them available using nuget.config (by adding fallbackPackageFolders entries), so no caches are created here.

erdembayar commented 1 year ago

none of the folders mentioned (v3-cache, plugins-cache, migrations, Migrations) are created, in any location.

One way of finding those folders would be dotnet nuget locals -l all, then you'll see something like below, then please check the content in them. image

I'd guess that those folders are not created because no Nugets are actually downloaded (with Azure DevOps feeds or without) - since we download and unpack nugets ourselves (with Bazel), we make them available using nuget.config (by adding fallbackPackageFolders entries), so no caches are created here.

It appears that you have a highly customized process. I'm not certain if the NuGet restore process could be seamlessly integrated into your CI/CD pipeline. I feel this request is very specific to one edge-case scenario. You may need to explore other options, such as checking if the sandbox already has an exception rule in place for situations like this.

keddad commented 1 year ago

I agree that we have quite a build system, however, I feel that no tool should just write random folders (especially since Migrations folder is not even used - as I mentioned, it just creates an empty file there). Considering that under Linux there is an easy way to override it's location, think it is reasonable to expect the same for Windows. We can whitelist a path in a sandbox, but that will make builds less reproducible, possibly causing heisenbugs.

nkolev92 commented 1 year ago

, I feel that no tool should just write random folders (especially since Migrations folder is not even used - as I mentioned, it just creates an empty file there).

It's used as a marker to decide whether migrations need to be run at the first run of the tooling.

Considering that under Linux there is an easy way to override it's location, think it is reasonable to expect the same for Windows. We can whitelist a path in a sandbox, but that will make builds less reproducible, possibly causing heisenbugs.

I don't think the situations are different for windows vs other OS. The means of discovering the directory on MacOS are through an env var. LOCALAPPDATA is an environment variable on Windows as well, it just happens to work that way and there's a particular API for it in .NET.

tldr; Our code does not provide a means to override that location on any OS. I think a fair ask here is to allow overriding the location altogether.

zivkan commented 1 year ago

The .NET CLI also writes a sentinel file on first use (a different file for every SDK version, unlike NuGet's migrations). But, they have an environment variable which will suppress it, which I'm guessing @keddad is using.

Anyway, given that the only purpose of NuGet's migrations are to migrate NuGet's persisted data when we change defaults across NuGet versions, I think it's also reasonable to have an environment variable to skip migrations. CI machines that get wiped every build, also everything that uses docker, will never have old state that needs migration.