canonical / dotnet-snap

The Canonical .NET snap with the .NET installer tool
GNU General Public License v3.0
1 stars 0 forks source link

Should snap installs automatically create symlinks? #13

Open nagilson opened 4 months ago

nagilson commented 4 months ago

Based on the history of https://github.com/dotnet/sdk/issues/10403.

When dotnet is installed via snap, the dotnet on PATH resolves to a realpath of /usr/bin/snap and not to a dotnet executable next to the sdk folder.

This breaks an assumption of the resolver that we can use realpath on dotnet on PATH to find the SDK. Thus, for a snap install to execute successfully, there are cases where manual intervention is required from the user to create a symbolic link to the actual .NET Path on their system, e.g.

ln -s /snap/dotnet-sdk/current/dotnet /usr/local/bin/dotnet.

Should we consider doing this automatically for snap installs?

mateusrodrigues commented 3 months ago

Hi @nagilson! Thank you for bringing this to our attention.

Currently, there are no plans to create the /usr/local/bin/dotnet -> /snap/dotnet-sdk/current/dotnet symlink automatically, especially because we'll be relying even more on the snap's own entry point when we release vNext of the .NET snap.

The new snap will include an installer tool that lets users pick and choose the runtimes and SDKs they want installed on their systems, effectively allowing them to have more than one major version installed at once (which is currently not possible with the existing dotnet-sdk snap), and that feature relies heavily on the snap's ability to have a custom entry-point.

Are there any alternatives that we can discuss, such as creating the /etc/dotnet/install_location{_x64} files that point to the real path of the dotnet executable? This is a classic snap, so doing that should not be a problem.

nagilson commented 3 months ago

Thanks for following up with my issue! :)

I think the alternative solution you propose is a better idea. Really, the symlink is more of a patch than solving the underlying issue, though both work well, I understand why you don't have plans to make that symlink automatically now. I'm not sure on the benefit of /etc/ -- in this case is it just a convention or are there aspects of this folder that make it for example get added to the path by default by snap, etc?

I don't have a ton of knowledge/authority in this space, but @richlander does and I'm curious to hear his take. cc @baronfel as well

mateusrodrigues commented 3 months ago

Well, if I understand correctly, the /etc/dotnet/install_location{_x64} files are already an established .NET convention on Linux/macOS (see here) and our .deb packages currently implement it.

Since that's a well-known location for getting the absolute path of the dotnet executable, the snaps can also follow the same convention.

For reference, I currently have the .NET .debs installed on my machine, this is what I see on both of these files:

$ cat /etc/dotnet/install_location
/usr/lib/dotnet
$ cat /etc/dotnet/install_location_x64 
/usr/lib/dotnet

Curious to hear the others takes as well 😃

leecow commented 2 months ago

Since that's a well-known location for getting the absolute path of the dotnet executable, the snaps can also follow the same convention.

@mateusrodrigues - are you suggesting that snaps also utilize /etc/dotnet/install_location to mark an installed snap's entrypoint?

mateusrodrigues commented 2 months ago

@leecow not the snap's entrypoint, but the actual location of the .NET installation. It would contain something like /var/snap/dotnet/common/dotnet, which is where we'll be placing the .NET files in the new snap.

Since the issue was raised because the snap's own executable on PATH does not link to the dotnet executable directly (see snippet below), /etc/dotnet/install_location would effectively be a place where you could find the path to the actual .NET location.

$ ls -l $(which dotnet)
lrwxrwxrwx 1 root root 13 Jun 14 09:46 /snap/bin/dotnet -> /usr/bin/snap
leecow commented 2 months ago

Seems like a good solution to me but would want @richlander, @baronfel or @marcpopMSFT to weigh in.

baronfel commented 2 months ago

Yes, that should be an acceptable solution. It would conflict with a .NET installation from a deb pkg but I don't think that's necessarily a bad thing - we generally don't recommend that users mix installation methods anyway.

richlander commented 2 months ago

There are two reasons why that file could be used:

I don't know much about snaps, but I'm wondering if they would break the second use case. It isn't clear to me that it would be a deal killer, but good to understand what the behavior will be.

mateusrodrigues commented 2 months ago

I don't know much about snaps, but I'm wondering if they would break the second use case.

@richlander IIUC the "dotnet directory hive" is the .NET root folder that contains the dotnet executable + the host, shared, sdk, and so on directories, correct?

If so, the fact that we're shipping with snaps shouldn't matter, because the behavior of /etc/dotnet/install_location is essentially the same as the deb pkg one. So, one can assume that:

Being more specific on the .NET snap case, /etc/dotnet/install_location would contain /var/snap/dotnet/common/dotnet, where:

Hope this makes sense :)

mateusrodrigues commented 2 months ago

Also, if we all agree with this approach, I can work on the PR for this change 😃

mateusrodrigues commented 2 months ago

So I have found the culprit in the GetDotnetExeDirectory function: https://github.com/dotnet/sdk/blob/a2de2d815a5fd9d51a56d686959589660f73b507/src/Resolvers/Microsoft.DotNet.NativeWrapper/EnvironmentProvider.cs#L76C17-L81C18

@baronfel @nagilson Ideally, I'd like to trigger this code path, so that I'm able to attach a debugger and work further on this function. I tried doing that through numerous dotnet CLI invocations, but none seem to go through GetDotnetExeDirectory, they always exit without ever hitting the break-point I set right at the beginning of it.

I'm a bit unfamiliar with the Resolver itself, so I would appreciate any pointers on how I could achieve that, or if there's a better way even :)

nagilson commented 1 month ago

@mateusrodrigues Thank you so much for your willingness to help investigate the issue. 😄 I apologize for our long delay to get back to you as we have been focused on a lot of other things atm. Great eye on spotting the GetDotnetExeDirectory function!

I was able to attach a debugger to the GetDotnetExeDirectory function. Here is one way to do it. Run dotnet --debug build --arch x86. (Other architecture values are OK too.) This will trigger a breakpoint in the architecture resolution logic, which then triggers RID resolution logic, which triggers GetDotnetExeDirectory. There are other ways to trigger it too, this is just a simpler one :)

I hope that answers your question? Btw, you need to also run the commands shown above after build and attach to the process ID as in my screenshot to get it to work. I show it in VS and using the windows script, but we have a shell script equivalent for linux. Hopefully this is helpful. image

mateusrodrigues commented 1 month ago

Hey @nagilson! Thank you so much, that was very helpful!

I noticed that the realpath logic sits inside the if statement on line 70 that is getting evaluated to false with the invocation dotnet --debug build --arch x86:

...
            if (string.IsNullOrEmpty(dotnetExe) || !Path.GetFileNameWithoutExtension(dotnetExe)
                    .Equals(Constants.DotNet, StringComparison.InvariantCultureIgnoreCase))
...

Going a little bit on a tangent here, but when is Path.GetFileNameWithoutExtension(dotnetExe) not going to be dotnet? Is this in the case when an application bundles the runtime and the invoked binary is not dotnet? If this is the (only) point of the if statement, is this scenario debuggable?

I was able to go into the if statement by inverting the logic temporarily anyway, so just asking out of curiosity 😄

nagilson commented 1 month ago

That's a good question, honestly I can't think of any scenario where that would be the case. That might just be a safe-guard. @JanKrivanek are you aware of a case where Path.GetFileNameWithoutExtension(dotnetExe) is not dotnet?

JanKrivanek commented 1 month ago

IIRC it was for selfcontained executables and sdk tests (code is hosted in runner).

nagilson commented 1 month ago

Thanks @JanKrivanek! That makes sense.