I have taken the time to try and make the windows installer a bit more sold, as it didn't work well with my windows system (where I run as a low-privileged user, and manually approve any escalation of privileges).
The main changes involve changing from the 'local user' settings to the 'local system,' and requiring admin privileges to install. (of course any user will be able to run Bitcoin without being an admin still).
Of-course this will need more testing.
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/233b122177528f1e6758890b77ba93e0b114a3c6 for binaries and test log.
8 | + 9 | # General Symbol Definitions 10 | -!define REGKEY "SOFTWARE\$(^Name)" 11 | +!define REGKEY "SOFTWARE\${NAME}" 12 | !define VERSION 0.7.99 13 | +!define PRODUCT_VERSION 0.7.99.0
This change would create another place that needs to be edited by hand for official releases, I'm not sure if core devs ACK this one. Perhaps you could mention in https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.txt#L6 that there are then 2 lines, which need to be edited.
!define PRODUCT_VERSION "${VERSION}.0" ... would save us a little typing and another potential source of bugs.
Agreed. Updating now.
I like the introduction of that variable, which holds the value Bitcoin, I'm unsure about the switch from local user to local machine. What is the benefit here? I also think NOT requiring Admin privileges is far more user friendly, as perhaps some use the client on machines, where they simply have none.
Edit: Can you have a look at #993 while you are working on this :)?
84 | @@ -82,24 +85,25 @@ Section -Main SEC0000 85 | SectionEnd 86 | 87 | Section -post SEC0001 88 | - WriteRegStr HKCU "${REGKEY}" Path $INSTDIR 89 | + WriteRegStr HKLM "${REGKEY}" Path $INSTDIR 90 | SetOutPath $INSTDIR 91 | WriteUninstaller $INSTDIR\uninstall.exe 92 | !insertmacro MUI_STARTMENU_WRITE_BEGIN Application 93 | + SetShellVarContext all
What is this one doing?
It tells the uninstaller to uninstall the start menu items from the All Users location, not the (default) user local location.
Can you write up a test plan?
Something like:
Test installing on top of an old installation, and then uninstalling. EXPECT: all installed files and registry entries removed (but data directory: blockchain and wallet files: left intact).
Note for testers: Run regedit and look for registry keys {....???....} to verify.
Test installing a new version, then installing an old version on top. EXPECT: ... etc...
Test: install as non-privileged user EXPECT: ... etc ...
Test: install as admin, run as admin EXPECT: ... etc ...
Test: install as admin, login as another user EXPECT: ... ??? ...
@Diapolo Yes, having user local installs would be great. However to provide that functionally the nsi install scripts are much more complicated than what we have now.
Ideally the installer will only as for permissions as it needs them. (For example a user local install will not require any escalation of privileges). Unlike an all-user install, that would escalate to admin.
Unfortunately, making a nsi installer that provides that sort of functional is quite hard.
I'm not sure user-local installs are a real concern, considering that it'd be a pretty bad idea to use Bitcoin on a system where anyone else has administrative controls...
Is it possible for the build bot to create the nsi installers?
We have no build bot (Gitian), which handles this nsi stuff AFAIK.
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/44e2863c7d0d9cb3d50803d776cb97993b06860b for binaries and test log.
Ping me here, and I'll do a gitian build of your branch.
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/44e2863c7d0d9cb3d50803d776cb97993b06860b for binaries and test log.
Vista and after support %LOCALAPPDATA%, which is the non-roaming data store (on XP and prior NT-based OS, the corresponding local data is %USERPROFILE%\Local Settings\Application Data).
As an example of use, this is where Google Chrome installs it's program and user data. Google doesn't care about your admin's silly "install program" policies or writing software that is Microsoft compliant, they are more than happy to install to and execute from this user-writable data store.
As an aside, Bitcoin stores user data in the wrong location:
To reduce exposure to "evil system admin" and make Bitcoin not break the enterprise environment, we must consider that %APPDATA% is a ROAMING profile location - when a domain controller is configured for roaming profiles (log into a different computer and your files are there too) all program data stored in this tree is replicated to the remote profile store.
This creates two problems: -Replicating gigs of "roaming" blockchain and using up user's quota, -Copying the wallet all over the network to other computers that the domain user logs in to.
A wise Bitcoin would use the non-roaming directory if %APPDATA%\Bitcoin is not present, or even better, migrate if it is.
Rather than hardcoding the decision, can we ask -- as some installers do -- whether to install for (a) current user or (b) all users?
Only the Administrator will be able to make changes to the Bitcoin install files. (Install, Update, Remove) However all users will be able to use bitcoin, and have their own interdependent %APPDATA% user files (Wallet, Blockchain).
ACK changes conceptually. Let's get this tested, especially on multiple Windows versions.
Rebased.
9 | # General Symbol Definitions 10 | -!define REGKEY "SOFTWARE\$(^Name)" 11 | +!define REGKEY "SOFTWARE\${NAME}" 12 | !define VERSION 0.8.2 13 | +!define PRODUCT_VERSION "${VERSION}.0" 14 | !define COMPANY "Bitcoin project"
After looking into https://github.com/bitcoin/bitcoin/blob/master/src/qt/res/bitcoin-qt.rc I would vote for harmonizing some strings and replacing this with just Bitcoin as we also use it in bitcoin.cpp as QApplication::setOrganizationName("Bitcoin");.
10 | -!define REGKEY "SOFTWARE\$(^Name)" 11 | +!define REGKEY "SOFTWARE\${NAME}" 12 | !define VERSION 0.8.2 13 | +!define PRODUCT_VERSION "${VERSION}.0" 14 | !define COMPANY "Bitcoin project" 15 | !define URL http://www.bitcoin.org/
Can this be just bitcoin.org perhaps, as we are using QApplication::setOrganizationDomain("bitcoin.org"); in bitcoin.cpp?
65 | VIAddVersionKey CompanyName "${COMPANY}"
66 | VIAddVersionKey CompanyWebsite "${URL}"
67 | -VIAddVersionKey FileVersion "${VERSION}"
68 | +VIAddVersionKey FileVersion ${VERSION}
69 | VIAddVersionKey FileDescription ""
70 | VIAddVersionKey LegalCopyright ""
Should be changed to 2009-2013 The Bitcoin developers to match the source and even bitcoin-qt.exe metadata IMHO :).
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/c80efe132575f2ca65479d784f18f5c6e6d478ec for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
47 | @@ -45,21 +48,21 @@ Var StartMenuGroup 48 | !insertmacro MUI_LANGUAGE English 49 | 50 | # Installer attributes 51 | -OutFile bitcoin-0.8.2-win32-setup.exe 52 | -InstallDir $PROGRAMFILES\Bitcoin 53 | +OutFile ${NAME}-${VERSION}-win32-setup.exe
This changes the output filename to capitalized, but there is no corresponding capitalization expected by the gitian yml.
Needs a rebase and a test plan written and tested on at least two versions of Windows (oldest we support and newest).
Closing this pull as it has been inactive for a long time. I also don't really like requiring admin rights to install, this has always annoyed me for other software in the rare cases I had to use windows.
In any case: please ping me or open a new pull request with a rebased version if anyone still wants to get this merged in some form.