Skip to content
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

Windows: Installer to ask which user profiles need installation #1289

Closed
23floors opened this issue Nov 15, 2019 · 19 comments
Closed

Windows: Installer to ask which user profiles need installation #1289

23floors opened this issue Nov 15, 2019 · 19 comments
Labels
Windows Windows related issues

Comments

@23floors
Copy link

Is your feature request related to a problem? Please describe.

Hi Surge Team!

Thanks for creating this wonderful software.

Recently installed it on a very vanilla Windows 10 x64 machine running Reaper 5.985.

Presently, installer deposits files to C:/user/foo-adminstrator/appdata/local/Surge

In our case, Surge is being run under a non-administrator account. We simply run the installer and provide the admin password, when the installer requests it.

Now, since we're installing from that non-admin account, we expected the installer to place these files in C:/user/foo-user/appdata/local/Surge.

Took an account switch, folder copy, and ownership change in order for us to get the software running nicely.

Describe the solution you'd like

It may be a smoother end-user experience, if the installer were to offer the choice of installing to a specific user's profile (OR default to all active profiles OR explicitly state the final installation path on the last window of the installer).

2 cents.

Once again, thank you!! :)

@baconpaul
Copy link
Collaborator

Hi! Thanks for the kind words

The installer is set up to install to the localappdata of the user in question

Source: ..\resources\data\*; DestDir: {localappdata}\Surge; Components: Data; Flags: recursesubdirs; Excludes: "*.git";

and surge looks there. Is %LOCALAPPDATA% not correctly set up in your install environment?

To be honest, the core team aren't exactly windows experts so happy for advice or contributions on more appropriate locations if you have expertise with installers!

@baconpaul baconpaul added this to the Currently Unscheduled milestone Nov 28, 2019
@baconpaul baconpaul added the Windows Windows related issues label Nov 28, 2019
@SA-Flipperwaldt
Copy link

Hi,

I was just looking to report the same installer behavior for version 1.6.4.1. The problem is that when installing when logged in as a regular user, Windows will ask you for credentials of an administrator account. Once these credentials are entered, the installer will proceed the same as if you were initially logged in as the administrator and started the install. %LOCALAPPDATA% will then be the administrator's local appdata folder. This is correct behavior as far as Windows is concerned, and should be the same across anything from Vista until the current version of Windows 10.

It must be said that the Surge plugin installs and superficially works correctly under these circumstances, but it just can't find the presets and wave files and lets you know this with a barrage of pop up error messages unless you login as the administrator.

Now I'm no authority on what is best practice here. There are two things I see other installers do. One is to put the presets in a subfolder of where the dlls are. This seems to work for other vsts I have installed and indeed seems to be what Surge did when it was still under VemberAudio management (a "surgedata" folder where surge.dll is). The other, alternatively and more broadly, is to only ask for elevation once the installer is ready to start writing files to protected folders instead of right at the start. This presumably gives the installer time to collect information on what user actually started the installer and to put the presets in the right appdata folder before elevating. I wish I could tell you how this is done on a technical level, but I don't know. I've just seen it done and should work fine here.

I hope this has been helpful in some way and thanks for the effort put in already.

@baconpaul
Copy link
Collaborator

Hi!

Thanks!

Right now I don't even have a windows VM up and running. Your comments are super helpful and also relate to the portable mode conversation in #16, from way back when surge was first open sourced (The 1.6.x vintage has always had the behavior with localappdata you describe).

What we really need is: A windows developer to help with things like this! I was happy to write test and improve the mac installers; and similarly linux devs were a great help with the linux build. But we are all just sort of in the blind on how the windows install stuff works. I can pattern match to add new things (like I did for the FX bank) but I have no real idea how to do and test what you describe.

So: Do you happen to be friends with a windows dev who would like to do some compact, well defined, but really high impact open source software development, to close this issue and #16? I would really welcome such a person joining the team, even if just for a short while to deal with these installer questions.

Thank you so much for your comments. They are useful!

@SA-Flipperwaldt
Copy link

Don't know anyone personally who qualifies, but I'll ask around!

Looking at the discussion about portability, I'll agree that the problem originates in the same place. Which is imo strictly sticking with the MS guidelines where it's correct, but perhaps not right (for the niche of plugins). Leaving that where it is, asking during installation where the asset folder should be, should work for everyone though and also mostly circumvent our problem here. At least I think an installer elevated to administrator can just write to another regular user's folders as long as it knows where they are. This seems to be the case if I elevate an alternative file manager anyway (File Explorer itself is a special case that can't be used to prove this either way).

After skimming some webforums, the solution to only asking for privileges once they are needed seems pretty involved. It would take starting the process unelevated, collecting data, then to have the process kill itself and relaunch asking for elevation when required, presumably while passing on information from one instance to the other some way.

More interesting was the suggestion not to use the {localappdata} constant at all, but instead to run a hidden cmd window and have it echo back the value for the environment variable %LOCALAPPDATA%, which, so is claimed, should resolve to the folder for the currently logged in user, even if this cmd window was triggered from an elevated process. I see no reason why this shouldn't work for other enviroment variables as well, if needed.

Ref: https://stackoverflow.com/questions/40613608/inno-setup-using-localappdata-for-logged-in-user - where it is used to write directly to the correct registry hyve. I'm not sure if there are more convenient ways for the installer to read out what the command prompt gives in response, but if nothing else you can basically "ECHO %LOCALAPPDATA% > filename.tmp" and read it out from there. Ref2: https://stackoverflow.com/questions/1136770/how-to-get-an-output-of-an-execed-program-in-inno-setup - People are doing similar such here.

@baconpaul
Copy link
Collaborator

Wow thank you. That is indeed involved.

As I mentioned over on the portability thread I haven’t done any serious windows dev for a decade - and then I was in a constrained environment anyway. This seems like something that “I would have a hard time with” and “a windows dev could do in a day” so I’m holding out hope. I appreciate the info you shared and also asking in your various communities if we can get any assistance!

@23floors
Copy link
Author

23floors commented Jan 17, 2020 via email

@baconpaul
Copy link
Collaborator

A great question, thank you for asking. Most of the code is portable and cross platform so there are just a few specific windows expertises we could use

Specifically things like building and configuring a windows installer, creating a portable surge (along with things like making hard links and setting paths), and just generally “making the windows stuff work like windows stuff” are what we really need. So a person who had packaged a windows app with innosetup is probably more useful immediately than a dsp c++ programmer (although we would welcome any and all help).

Another way to look at it. I use a mac. I develop on a mac. I can tell if the mac is “not mac-like” easily. I can also tell if surge works or not, and most of that code is portable. So as a result surge works fine on windows, but no one is there to help make surge “windows like” appropriately.

Innosetup experience and building and installing portable dll, especially in a music software environment, is really the proximate thing we need as a result.

Hope that helps! And again we welcome any and all contributors to the project. We’re pretty friendly grouP!

@ham-steamer
Copy link

The PrivilegesRequired=lowest flag in the [Setup] section would resolve this. Is there anything in the installation that necessarily requires elevated privileges?

@baconpaul
Copy link
Collaborator

Installing VST3 into the appropriate area requires elevated privs I think, right?

@baconpaul
Copy link
Collaborator

Yeah i am pretty sure that

Source: ..\target\vst3\Release\Surge.vst3; DestDir: {cf}\VST3; Components: VST3; Flags: ignoreversion

And write perms to common files means the vst3 needs to be elevated.

Can we elevate just one line item of an innosetup?

@baconpaul
Copy link
Collaborator

just googling it seems another solution would be

  1. Install with elevated privs in commonappdata rather than LocalAppData
  2. Modivy the surge c++ to check commonappdata before LocalAppData

This would mirror what we do on mac (where we check /Library/Application Support and ~/Library/Application Support both) but like I said, I don’t use windows, so I doin’t know if this idea is dumb.

if the idea is not dumb it’s a pretty easy change to make.

@baconpaul
Copy link
Collaborator

if (!SHGetKnownFolderPath(FOLDERID_LocalAppData, 0, nullptr, &localAppData))

That’s the code which would check for commo before local, just like mac and linux do directly above.

Comments from windows users on this approach?

@ham-steamer
Copy link

@baconpaul Seems logical. I wasn't aware that VST3 required installation into CommonFiles, which does require admin permission. Installing everything else in ProgramData makes sense.

@baconpaul
Copy link
Collaborator

OK cool. I’ll try and get a windows VM working before we ship 1.6.5 so I can see if I can change this; but to the earlier question of “what constitutes a windows dev” it would be someone who modifies the installers, modifies the source to check common before local, tests it in 64 and 32 bit in a few daws, pulls together the pull request and checks the nightly once it is merged. That’s a few hours of work for me even after I get the VM running, and again I’m not sure I would get it right. But if no one appears I can definitely look!

@baconpaul baconpaul modified the milestones: Currently Unscheduled, 1.6.5 Jan 24, 2020
baconpaul added a commit to baconpaul/surge that referenced this issue Jan 25, 2020
Move windows assets to ProgramData, failing back to LocalAppData.
Show paths in the About screen. Modify the installers.

Addresses surge-synthesizer#1289
baconpaul added a commit that referenced this issue Jan 25, 2020
Move windows assets to ProgramData, failing back to LocalAppData.
Show paths in the About screen. Modify the installers.

Addresses #1289
@baconpaul
Copy link
Collaborator

baconpaul commented Jan 25, 2020

Hello people on this issue.

I NEED YOUR HELP TESTING the new surge installer

Per this thread, and conversations with @mkruselj on our slack, I have changed the surge installer and runtime in two ways

  1. Rather than %LOCALAPPDATA% info goes into c:\ProgramData (using the appropriate variable)
  2. Surge reads from c:\programdata and then if that is empty %LOCALAPPDATA% after that

Additionally the Surge about screen now shows the paths used to load assets.

So how can you help

  1. Please grab the nightly installer from surge-synthesizer.github.io and install Surge
  2. Do the assets appear in your system in c:\programdata\surge properly?
  3. Does surge run
  4. Does surge about screen show the c:\programdata path for the data path?

It would be great if we could test this on 32 and 64 bit with both the vst2 and vst3. I have tested 64 bit VST3 and on my VM in Reaper it works as expected.

I plan on releasing Surge 1.6.5 in a couple of weeks and it will include this change, so testing from the broader community is really crucial. I appreciate you taking a bit of time to try it out and let me know if it works.

Thank you

@baconpaul
Copy link
Collaborator

Just giving this a bump to ask for some more testing? Seems windows users can install and run surge from the nightlies so I think it is good; I'll close this issue as resolved with the 1.6.5 release unless I hear otherwise from someone here. Thanks!

@mkruselj
Copy link
Collaborator

Seems to work fine over here, but I agree hearing a few more voices would be more reassuring.

@SA-Flipperwaldt
Copy link

Yeah, that all seems to work correctly for me as well for both the 32 and the 64 bit installer.

@baconpaul
Copy link
Collaborator

Wonderful; thank you for testing and helping! Appreciated.

Lemme close this now, since we have a few confirms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Windows Windows related issues
Projects
None yet
Development

No branches or pull requests

5 participants