-
Notifications
You must be signed in to change notification settings - Fork 426
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
Execute the Windows installer as normal user #2628
Execute the Windows installer as normal user #2628
Conversation
Can you elaborate on this? If we still need .dll but we are not shipping its installer and thus not installing it ourselves, how does this work in practice? |
I just learned that the windows-latest image of GH actions includes VS 2022 with the component Microsoft.VisualStudio.Component.VC.Redist.14.Latest. And I found a script that copies the DLL from it. Python, VS Code and many Rust CLI apps ship this DLL. |
Sorry I still don’t understand how it works. If we are not shipping the .dll how does Livebook work without it? Are you saying users would have gotten it after installing, say, VSCode? But then it wouldn’t work without it, right? |
Are you saying we should ship .dll ourselves (as opposed to shipping the vc_redist.exe which installs it into location which requires admin)? If so, yes, that’d be great and help on that front would be very appreciated. |
Yes, that's what I mean. I will ask the Erlang people why don't they do it themselves. This issue leaves students and people in companies out of the BEAM world. If for some reason they don't want to do it, I will implement it for Livebook :) |
Right, thanks. So about the other changes. Switching to HKCU sounds good to me. About dropping Windows.Forms and .NET 4.6 with self-contained=false in favor of .NET 8.0 with self-contained=true and using third party plugins, what is the installer size difference? Could you list upsides/downsides of what you are proposing? In isolation from other issues (vc redist and HKCU) because I believe, and correct me if I’m wrong, it is completely orthogonal? |
The installer size is almost the same, thanks to trimming. I thought that Livebook.exe depends on vcredist. I didn't realize it was due to Erlang. Windows Forms can be used with .NET 8, but it currently does not support trimming. That is planned for .NET 9. So I removed it in order to use trimming. The third-party libraries are thin wrappers on top of the Windows API. One could just include the code. But now that I know that we only need vcruntime140.dll I reverted the changes to Livebook.exe. |
The PR looks good to me assuming OTP will ship with required .dll files. If you could start the conversation and/or PR and if accepted it would be released in OTP 27.1, I think we can wait. Otherwise, eg it is deamed to big of a change and so scheduled for OTP 28, we’d appreciate doing the change in Livebook. |
If you start issue/PR in OTP please cc me and/or mention it here so we can keep track! |
I opened an issue in the OTP repo. Subscribe and we'll see what they say :) |
Hey @mardukbp, thanks again for working on this. I tried the PR locally and noticed that Livebook was no longer showing up in "Settings / Apps / Installed Apps", i.e. we cannot delete it from that place anymore. Is it because to have that, we're writing to HKLM and we no longer have permission to do so when not running as an admin, i.e. it silently fails? I tested .livemd file association and livebook:// url scheme and it works well. Please revert removal of vc_redist.exe. We shouldn't wait for OTP to make it obsolete. I just tested this and even with |
Hi @wojtekmach, I just updated the installer so that uninstalling works. I also reverted the removal of vc_redist.exe. The Livebook installer would work for anybody who already has vcruntime140.dll. Hopefully that is a large percentage of users. And hopefully OTP will one day ship the DLL. Then Livebook will not need vc_redist.exe at all. |
Could you rebase with latest main? I’d expect your changes around node naming and similar are no longer necessary. |
1d2e8f3
to
6495187
Compare
Done |
I tested it with a user with no admin rights and Livebook does not work. The notebook cannot connect to Elixir. Something is missing in the new env.bat. I will have to investigate, unless you have an idea. |
@wojtekmach I got it working :) Since I am behind a VPN, following the Installation FAQ I created ".livebookdesktop.bat" in my User Profile folder with the contents:
My suggestion would be that the installer creates this file. Unless it can negatively affect some users. |
I believe those are the default in latest Livebook, so we should be good to go! |
6495187
to
dac251d
Compare
I built the installer and tested it on a Windows machine with no admin rights. Livebook is now enterprise-ready! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this and it works great.
There's a minor caveat that the vcredist installer shows up every single time:
My guess is we didn't see it previously because the installer was running as an admin and maybe then they don't bother asking.
I think I can easily fix this by only conditionally running the vcredist installer when needed (by checking appropriate winreg keys)
Thanks again!
Uffizzi Preview |
When we release it, we need to tell people that if they previously installed Livebook, they need to go to "Settings / Apps / Installed Apps" and manually uninstall it first. I'm a little bit worried people might run into weird issues where previously the installer was running as an admin and now as a user. I tested it though, installing nightly, uninstalling it, installing this branch, and everything works well. @josevalim @jonatanklosko @hugobarauna should we merge this or wait until after v0.15 is out? I'm keen on users ideally trying this out with nightlies and reporting any issues sooner than later. |
@wojtekmach could we perhaps use another directory, to avoid the admin conflict issue? In any case, feel free to merge this any time. |
You know what, I think this is probably fine and if people run into issues we'd tell them to uninstall and try again anyway. |
No comments on this one. Defer to José and Jonatan. That said, I see it's already merged. So, great! :) |
@wojtekmach Thanks for all the time invested in reviewing and testing this PR. As I have mentioned before, the definitive solution for vcredist is including that one DLL in the distribution. Since it is actually needed by Erlang, they should do it. I already opened an issue in their repo and argued that requiring admin rights for installing Erlang leaves people at enterprises and universities out of the BEAM ecosystem. I won't be at Code Beam Europe, but maybe you or @josevalim can talk to them about this :) |
Currently the Windows installer requires admin privileges, which are typically not available in an enterprise environment. This PR produces a Windows installer that runs without admin privileges.
Three of the four reasons why the installer requires admin rights have been addressed:
OTP (or rather erl.exe) requires vcruntime140.dll. This DLL can be added after extracting the OTP distribution.