/D=%ProgramData%\Notepad++ not working

mpheath

I see possibly the bug.

Commit #11013 added a fix.

; 64-bit patch for the NSIS attribute InstallDirRegKey (used in globalDef.nsh)
ReadRegStr $INSTDIR HKLM "Software\${APPNAME}" ""

This fix is copying the registry value directly to $INSTDIR. If $INSTDIR contains the value from the argument /D=..., then it probably gets overwritten without conditions.

I guess with NSIS, it should use a temporary variable and overwrite only if not an empty value.

So this may work:

; 64-bit patch for the NSIS attribute InstallDirRegKey (used in globalDef.nsh)
ReadRegStr $0 HKLM "Software\${APPNAME}" ""

${If} "$0" != ""
    StrCpy $InstDir "$0"
$(EndIf)

Location to post a bug report.

xomx

@mpheath

I bumped to this problem too.

The mentioned x64 patch is mine. I succumbed there to the temptation to do it as simple as possible. So I rejected my original solution to give this patch right at the top of the .onInit function. Your solution will work but I am putting in word for something like:

Function .onInit

!ifdef ARCH64 || ARCHARM64 ; x64 or ARM64
	${If} ${RunningX64}
		; disable registry redirection (enable access to 64-bit portion of registry)
		SetRegView 64
		
		; 64-bit patch for the NSIS attribute InstallDirRegKey (used in globalDef.nsh)
		ReadRegStr $INSTDIR HKLM "Software\${APPNAME}" ""
	${EndIf}
!endif

	${GetParameters} $R0

That way we simulate the used NSIS attribute InstallDirRegKey most closely and we will avoid another such difficulties. At least I hope so.

I could do the PR myself, if you want to. Otherwise please do it yourselves.

xomx

@xomx

sorry, fixed code:

Function .onInit

!ifdef ARCH64 || ARCHARM64 ; x64 or ARM64
	${If} ${RunningX64}
		; disable registry redirection (enable access to 64-bit portion of registry)
		SetRegView 64
		
		ReadRegStr $0 HKLM "Software\${APPNAME}" ""

		${If} "$0" != ""
			StrCpy $InstDir "$0"
		${EndIf}
	${EndIf}
!endif

	${GetParameters} $R0

mpheath

@xomx said in /D=%ProgramData%\Notepad++ not working:

That way we simulate the used NSIS attribute InstallDirRegKey most closely and we will avoid another such difficulties. At least I hope so.

I could do the PR myself, if you want to. Otherwise please do it yourselves.

You have some good ideas of improvement. If you can do the PR, that would be appreciated.

xomx

@mpheath
Ok, so I am going to do a PR for it, when I find time for testing.

And there seems to be already an issue for this.

xomx

So as always, the fix needs more, than I originally expected.
Could you please test both installers from:
http://redwoodal.sweb.cz/NotepadPlusPlus/NppTestInstaller_issue11072.7z

Current .onInit solution, explanatory remarks included:

Function .onInit

	; 64-bit patch for the NSIS attribute InstallDirRegKey (used in globalDef.nsh)
	; - this is needed because of the NSIS binary, generated for 64-bit Notepad++ installations, is still a 32-bit app (this can be changed in the future),
	;   so the InstallDirRegKey attribute checks for irrelevant RegKey (HKEY_LOCAL_MACHINE\SOFTWARE\WOW6432Node\Notepad++), see also this:
	;   https://nsis.sourceforge.io/Reference/SetRegView
	; - for the closest InstallDirRegKey simulation possible, this should be right at the top of .onInit
	;
	; Notes:
	;
	; - before .onInit is executed, $INSTDIR has been preset by the InstallDir attribute
	; - then if the value defined by a possible InstallDirRegKey attribute is valid, it overrides original InstallDir
	; - finally, if the user used "/D=..." at runtime (at installer cmdline), $INSTDIR has been changed to that parameter before .onInit
	;
	; - so we have to know here, if the "/D" was used, but there is a problem (citation from the NSIS documentation):
	;   "For parsing out the PARAMETER portion, see GetParameters. If /D= is specified on the command line (to override the install directory) it won't show up in $CMDLINE."
	;
	; - so one solution could be not to use the 'Installdir' & 'InstallDirRegKey' at all and then we can use here:
	;	${If} $INSTDIR != ""
	;		;/D was used for sure, so obey and do nothing
	;	${Else}
	;	...
	;
	; - another solution is to parse the original cmdline for the "/D=..." and then act accordingly (this is the chosen solution below)
	;
!ifdef ARCH64 || ARCHARM64 ; x64 or ARM64
	${If} ${RunningX64}
		System::Call kernel32::GetCommandLine()t.r0 ; get the original cmdline (where a possible "/D=..." is not hidden for us by NSIS)
		${StrStr} $1 $0 "/D="
		${If} "$1" == ""
			; "/D=..." was NOT used for sure, so we can continue in this InstallDirRegKey x64 patch 
			SetRegView 64 ; disable registry redirection (enable access to 64-bit portion of registry)
			ReadRegStr $0 HKLM "Software\${APPNAME}" ""
			${If} "$0" != ""
				; a valid previous installation has been detected, so offer that as the $INSTDIR
				StrCpy $INSTDIR "$0"
			${EndIf}
			SetRegView 32 ; restore original state
		${EndIf}
	${EndIf}
!endif

mpheath

@xomx said in /D=%ProgramData%\Notepad++ not working:

Could you please test both installers from:
http://redwoodal.sweb.cz/NotepadPlusPlus/NppTestInstaller_issue11072.7z

I tried the x64 installer. SetRegView 32 causes Call writeInstallInfoInRegistry to write to the x86 registry. Consider what the previous installer does and do not restore the registry view or perhaps change once again the registry view just before Call writeInstallInfoInRegistry, near to the end of the script.

I do not see the reason to use System::Call kernel32::GetCommandLine()t.r0 when ${GetOptions} $R0 "/D" $R1 can be used to get the value from /D. So, I looked at an alternative to your code. I cloned the NPP repo and uploaded a branch with what I tested the x64 installer with. I have not tested x86 yet. Here is a compare between the master and installer branches.

My code ignores /D if already installed as why allow to install to a different location? To me, it would probably mess up things if someone tried multiple locations. So first read registry, then /D, else the default $ProgramFiles....

I usually use Inno Setup so NSIS is a bit of a new learning curve for me. It still boils down to logic so I think I got it right for this NSIS script.

xomx

@mpheath
Thanks for your code review.

@mpheath said in /D=%ProgramData%\Notepad++ not working:

I tried the x64 installer. SetRegView 32 causes Call writeInstallInfoInRegistry to write to the x86 registry.

Strange, for me my patched npp.8.3.Installer.x64.exe writes installation path correctly to the:
HKLM\SOFTWARE\Notepad++
instead to the 32-bit equivalent:
HKLM\SOFTWARE\Wow6432Node\Notepad++

And re-running of a x64 N++ installer after correctly picks up this as the default installation place, while the x86 one does not see that.

Did you really tried to run that and to check the Registry after? Or are you judging only from the code fragment I pasted here? Because in the whole nppSetup.nsi installation script has been left the original code calling the SetRegView 64 later for a running x64-instalator.

@mpheath said in /D=%ProgramData%\Notepad++ not working:

I do not see the reason to use System::Call kernel32::GetCommandLine()t.r0 when ${GetOptions} $R0 “/D” $R1 can be used to get the value from /D

As I stated in the patch-fragment Notes, one cannot detect the “/D” by usual way, as the NSIS hides that from the NSIS user - taken from:

“For parsing out the PARAMETER portion, see GetParameters. If /D= is specified on the command line (to override the install directory) it won’t show up in $CMDLINE.”

So your ${GetOptions} $R0 "/D" $R1 should not work (I can test that in the evening).

@mpheath said in /D=%ProgramData%\Notepad++ not working:

So, I looked at an alternative to your code

Your alternative is described in my Notes too:

	; - so one solution could be not to use the 'Installdir' & 'InstallDirRegKey' at all and then we can use here:
	;	${If} $INSTDIR != ""
	;		;/D was used for sure, so obey and do nothing
	;	${Else}
	;	...

But this is intended to be a simple patch, which can be simply deleted (preferably at one place only) when neccessary (e.g. when the NSIS N++ binary becomes also x64 instead of the current x86). So I prefer not touching the other files (globalDef.nsh).

@mpheath said in /D=%ProgramData%\Notepad++ not working:

My code ignores /D if already installed as why allow to install to a different location?

Yes, that was my intention. I would like to allow users whatewer they need to (maybe I should not, but who knows, what the users want to do…).

mpheath

@xomx said in /D=%ProgramData%\Notepad++ not working:

Strange, for me my patched npp.8.3.Installer.x64.exe writes installation path correctly to the:
HKLM\SOFTWARE\Notepad++
instead to the 32-bit equivalent:
HKLM\SOFTWARE\Wow6432Node\Notepad++

And re-running of a x64 N++ installer after correctly picks up this as the default installation place, while the x86 one does not see that.

Did you really tried to run that and to check the Registry after? Or are you judging only from the code fragment I pasted here? Because in the whole nppSetup.nsi installation script has been left the original code calling the SetRegView 64 later for a running x64-instalator.

I may have jumped on that one too quick, sorry. I had a entry in x86 registry and a fresh install reads it and it becomes the destination location in the UI of the x64 install. If this registry entry is added to the registry:

Windows Registry Editor Version 5.00

[HKEY_LOCAL_MACHINE\SOFTWARE\WOW6432Node\Notepad++]
@="c:\\x86"

and run the x64 installer, it reads it. This maybe some weird NSIS or Windows bug/behavior. If the x64 registry also has the value, then it considers that one as the destination location.

As I stated in the patch-fragment Notes, one cannot detect the “/D” by usual way, as the NSIS hides that from the NSIS user - taken from:

“For parsing out the PARAMETER portion, see GetParameters. If /D= is specified on the command line (to override the install directory) it won’t show up in $CMDLINE.”

So your ${GetOptions} $R0 "/D" $R1 should not work (I can test that in the evening).

Look at the top of the script. ${GetParameters} $R0 is being used as the help recommends GetParameters. $CMDLINE is not being used. ${GetOptions} $R0 "/D" $R1 is using the data from ${GetParameters} $R0.

Your alternative is described in my Notes too:
	; - so one solution could be not to use the 'Installdir' & 'InstallDirRegKey' at all and then we can use here:
	;	${If} $INSTDIR != ""
	;		;/D was used for sure, so obey and do nothing
	;	${Else}
	;	...
But this is intended to be a simple patch, which can be simply deleted (preferably at one place only) when neccessary (e.g. when the NSIS N++ binary becomes also x64 instead of the current x86). So I prefer not touching the other files (globalDef.nsh).

Not exactly, though perhaps in a minor way. The globalDef.nsh can remain the same, though the values for $InstallDir will be overwritten in nppSetup.nsi. I chose to remove InstallDir and InstallDirRegKey as it is dead/deprecated code, though if your PR, you can submit your choice.

xomx

@mpheath said in /D=%ProgramData%\Notepad++ not working:

I had a entry in x86 registry and a fresh install reads it and it becomes the destination location in the UI of the x64 install. …
… This maybe some weird NSIS or Windows bug/behavior.

Weird, maybe I should check for that behavior too. But I see that there is this code trying to detect a previous 32-bit installation and offering uninstall of the 32-bit 1st.

@mpheath said in /D=%ProgramData%\Notepad++ not working:

${GetParameters} $R0 is being used as the help recommends GetParameters. $CMDLINE is not being used. ${GetOptions} $R0 “/D” $R1 is using the data from ${GetParameters} $R0.

Yes, but maybe the GetParameters is inherently using $CMDLINE.
Anyway, I was right, it is not working. You can check that for yourself:

take the simplest NSIS example installed (“C:\Program Files (x86)\NSIS\Examples\example1.nsi”)
add there somewhere the following code:

!include "FileFunc.nsh"

Function .onInit
	MessageBox MB_OK $INSTDIR
	${GetParameters} $R0
	MessageBox MB_OK $R0
	${GetOptions} $R0 "/D" $R1
	MessageBox MB_OK $R1
	MessageBox MB_YESNO "This example will install. Continue?" IDYES NoAbort
		Abort
	NoAbort:
FunctionEnd

now try to run the compiled installer e.g. as:
example1.exe /S /D=C:\TEMP
you will see messageboxes with: 1) “C:\TEMP” … 2) “/S” … 3) “”

mpheath

@xomx

Using example1.nsi with the code above appended to the bottom of the example and compiled.

4 Messageboxes using Ctrl+C

---------------------------
Example1 Setup
---------------------------
D:\User\Desktop\Example1
---------------------------
OK   
---------------------------


---------------------------
Example1 Setup
---------------------------
/S /DC:\TEMP
---------------------------
OK   
---------------------------


---------------------------
Example1 Setup
---------------------------
C:\TEMP
---------------------------
OK   
---------------------------


---------------------------
Example1 Setup
---------------------------
This example will install. Continue?
---------------------------
Yes   No   
---------------------------

This was compiled with NSIS 3.08.

Expected due to InstallDir $DESKTOP\Example1. (That is where my desktop is.)
Expected due to all arguments passed.
Expected as being the value of /D.
Selected No to not install.

I do not see how you got C:\TEMP for the 1st messagebox if only InstallDir $DESKTOP\Example1 sets $INSTDIR before the message is shown. C:\TEMP is the value I got for the 3rd messagebox.

xomx

@mpheath said in /D=%ProgramData%\Notepad++ not working:

I do not see how you got C:\TEMP for the 1st messagebox

because of the /D=C:\TEMP was used at the cmdline…

Apparently you have this error at your cmdline:

/DC:\TEMP

It should be:

/D=C:\TEMP

Otherwise it is just another usual param and therefore it is visible for the GetParameters/GetOptions.

xomx

@mpheath said in /D=%ProgramData%\Notepad++ not working:

I do not see how you got C:\TEMP for the 1st messagebox

I described the sequence of the $INSTDIR assignment in my patch Notes too:

; - before .onInit is executed, $INSTDIR has been preset by the InstallDir attribute
; - then if the value defined by a possible InstallDirRegKey attribute is valid, it overrides original InstallDir
; - finally, if the user used "/D=..." at runtime (at installer cmdline), $INSTDIR has been changed to that parameter before .onInit