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

nsis: installer updates #2067

Closed
wants to merge 5 commits into from
Closed

Conversation

Lucarda
Copy link
Contributor

@Lucarda Lucarda commented Jul 27, 2023

This PR (targeted to develop) attempts to have a less prone to errors installer and also includes some troubleshooting tools.

  • safer behavior:

    • the (un)installer refuses to do anything if it finds a currently running Pd(64).

    • check if there's write permission on target dir before attempting to do anything else.

  • troubleshoot tools:

    • tool to clear Pd(64) preferences. (only runs at user scope).

    • tool to set which Pd(64) installation is used to open .pd files (scope can be user or allusers).

    • also these two tools are included in the uninstaller but here the '.pd' files repair is automatic (you don't have to choose which app).

  • user scope:

    • admin users can choose to install or run troubleshot tools at user or allusers scope.
  • alternate install

    • user can choose to do an alternate install if there is already a default one. it prompts for a short string (like Pd-0.54-0-(alt) to be used to name the: install folder, start menu folder, desktop shortcut and uninstall name.

Tested to work correctly on WinXp(i386) and Win11(amd64).

Test artifacts: https://nc.nubegris.com.ar/index.php/s/E7Pyg3M9ZA7Hygo

@umlaeute
Copy link
Contributor

nice.

troubleshoot tools:

i wonder: could we integrate these tools into the (un)installer ?

IIRC, windows often provides this Modify/Uninstall button in the Software Center, and I think the "clear properties" might be an excellent use-case.

(I'm not entirely sure whether this would work for file associations as well, without letting the user jump through too many hoops...)

@Lucarda
Copy link
Contributor Author

Lucarda commented Jul 28, 2023

i wonder: could we integrate these tools into the (un)installer ?

indeed that is useful. I'll work on it.

@Lucarda Lucarda marked this pull request as draft July 28, 2023 05:43
safer behavior:

  - the (un)installer refuses to do anything if it finds a currently running Pd(64).

  - check if there's write permission on target dir before attempting to do anything else.

troubleshoot tools:

  - tool to clear Pd(64) preferences. (only runs at user scope).

  - tool to set which Pd(64) installation is used to open .pd files (scope can be user or allusers).

  - also these two tools are included in the uninstaller but here the '.pd' files repair is automatic (you don't have to choose which app).

user scope:

  - admin users can choose to install or run troubleshot tools at user or allusers scope.

alternate install:

  - user can choose to do an alternate install if there is already a default one. it prompts for a short string (like Pd-0.54-0-(alt) to be used to name the install folder, start menu folder, desktop shortcut and uninstall name.

Tested to work correctly on WinXp(i386) and Win11(amd64).
@Lucarda
Copy link
Contributor Author

Lucarda commented Aug 1, 2023

(I'm not entirely sure whether this would work for file associations as well, without letting the user jump through too many hoops...)

it turned out that it's the simplest and useful one. it had been implemented in the uninstaller along with the 'clear prefs'.

things I'm not totally sure

  • wording

  • do we need the tools in the installer? i think they are ok there so people are advertised of them.

the installers in the link above had been updated.

@Lucarda Lucarda marked this pull request as ready for review August 1, 2023 07:41
@sebshader
Copy link
Contributor

sebshader commented Aug 1, 2023

I always run into the issue that I have to change the paths manually in pd.nsi (to exclude '/tmp/' prefix) or it can't find the resources (which end up in c:/msys64/tmp/tmpblahblah). Is there a better way around this? Should that be a separate issue?

@Lucarda
Copy link
Contributor Author

Lucarda commented Aug 1, 2023

yes this is odd. iirc during this days i built on linux without the /tmp/ prefix and it worked. i'll test this again tomorrow. if there are no problems i'll remove them.

@sebshader
Copy link
Contributor

sebshader commented Aug 1, 2023

well according to the message at the top of the file, the 'tmp' has to be there for miller's build environment using wine? Just seems like there should be some option or setting, and I'm wondering how other people use the scripts to make an installer on a real windows machine. Hard to imagine everyone having to manually edit the pd.nsi file every time.. (or maybe everyone just uses wine?)

And if everyone does accomplish it by manually editing pd.nsi every time, that should probably be mentioned in the README or something so people don't just see an error and wonder why it isn't working.

@umlaeute
Copy link
Contributor

umlaeute commented Aug 1, 2023

Is there a better way around this? Should that be a separate issue?

how about using the -o/--outdir flag:

$ ./build-nsi.sh -h
Helper script to build a proper Windows installer out of a
Pd build tree.

Usage:
./build-nsi.sh [ OPTIONS ] <path_to_pd_build> [<version> [<wish-exec-name> [<arch>]]]

 Options:

  -o,--outdir DIR      Output directory (where the generated installer binary
                       can be found after a succesfull run)

  -h,--help            Print this help

 Arguments:

  <path_to_pd_build>   input directory (containing 'bin/pd.exe')
                       such as created by 'make app'
  <version>            Pd-version to create installer for (e.g. '0.32-8')
                       DEFAULT: autodetect from <path_to_pd_build>
  <wish-exec-name>     name of the Tcl/Tk interpreter executable
                       e.g., 'wish85.exe'
                       DEFAULT: use a "bin/wish*.exe" in <path_to_pd_build>
  <ARCH>               architecture of the Pd executable.
                       valid values are '32' for x86 and '64' for x86_64
                       DEFAULT: autodetect from "bin/pd.exe" in <path_to_pd_build>

@sebshader
Copy link
Contributor

sebshader commented Aug 1, 2023

doesn't work, I think the issue is that the script can't find things in "WORKDIR", and OUTDIR is different. I'm wondering if other people use it on windows without editing pd.nsi.
./build-nsi.sh -o c:/Users/sebfu/codins/pd/pdnext/msw c:/Users/sebfu/codins/pd/pdnext/msw/pd-0.54-0-next 0.54-0 ->

!include: could not find: "/tmp/tmp.EWn40Bymzk/license_page.nsh"
Error in script "C:/msys64/tmp/tmp.EWn40Bymzk/pd.nsi" on line 142 -- aborting creation process

@umlaeute
Copy link
Contributor

umlaeute commented Aug 1, 2023

@sebshader thanks for the additional info. i think this warrants indeed a separate issue.

@umlaeute
Copy link
Contributor

umlaeute commented Aug 1, 2023

in any case, please check whether 06d6632 in this PR fixes the issue.

@sebshader
Copy link
Contributor

sebshader commented Aug 1, 2023

unfortunately not, now it's just: !include: could not find: "C:/msys64/tmp/tmp.WvWpL8Vj1h/license_page.nsh" instead.
actually I had already tried manually changing the paths in the build-nsi.sh to that and it's odd it doesn't work because that file does exist there.

@sebshader
Copy link
Contributor

sebshader commented Aug 2, 2023

so trying to use the installer, I'm getting the "Refusing to continue, save and quit pd" message using the files on this branch.
(This is after restarting my computer to make sure no pd is running)

@sebshader
Copy link
Contributor

sebshader commented Aug 2, 2023

I'm pretty inexperienced with windows, but I'm not sure if the 'qprocess' exists..?
edit: it isn't in C:\Windows\sysnative when running C:\WINDOWS\SysWOW64\cmd.exe
(windows 10)

@Lucarda
Copy link
Contributor Author

Lucarda commented Aug 3, 2023

32-bit NSIS

this should be documented somewhere but the thing is that we have to compile with a 32-bit nsis. It has to be done like that because the package that comes with Debian and Fedora are 32bit nsis. Also we need to support 32bit Pd on 32bit machines :)

So get

pacman -S mingw32/mingw-w64-i686-nsis

then open the MINGW32 shell and compile.

the Refusing to continue, save and quit pd should now work correctly (only if Pd is running).

About qprocess

start a normal CMD window. Now you are at the 64bit command prompt.

type qprocess: you'll get all running processes.

now switch to the 32-bit command prompt

C:\WINDOWS\SysWOW64\cmd.exe

you'll get

Microsoft Windows [Version 10.0.22000.2176]
(c) Microsoft Corporation. All rights reserved.

type qprocess. you'll get

'qprocess' is not recognized as an internal or external command,
operable program or batch file.

so to access the 64bit programs you must do:

cd %windir%\sysnative

qprocess

Note that on a 64bit prompt you can't cd %windir%\sysnative

@sebshader
Copy link
Contributor

sebshader commented Aug 3, 2023

About qprocess

start a normal CMD window. Now you are at the 64bit command prompt.

type qprocess: you'll get all running processes.

running this gives:

'qprocess' is not recognized as an internal or external command,
operable program or batch file.

this is the C:\Windows\System32\cmd.exe
edit: same for qprocess.exe

as I said above, when I use the 32-bit cmdand cd into %windir%/sysnative the result is the same

edit: tried with the 32 bit nsis version and it also failed.
afaict qprocess is only available on windows server

@Lucarda
Copy link
Contributor Author

Lucarda commented Aug 3, 2023

Oh, oh oh,

thanks for the report. Scratching my head. I'm on win11 and works for me. Will test on win10 in 2'.

@Lucarda
Copy link
Contributor Author

Lucarda commented Aug 3, 2023

qprocess works for me on Win10 (enterprise).

Which version do you use?

@sebshader
Copy link
Contributor

Edition Windows 10 Home
Version 22H2
Installed on ‎5/‎20/‎2023
OS build 19045.3208
Experience Windows Feature Experience Pack 1000.19041.1000.0

@Lucarda
Copy link
Contributor Author

Lucarda commented Aug 3, 2023

I took from here that this should work but seems there is missing info

@sebshader
Copy link
Contributor

I think we might have to use tasklist instead

@Lucarda
Copy link
Contributor Author

Lucarda commented Aug 3, 2023

here is says that is not for all editions

@sebshader
Copy link
Contributor

sebshader commented Aug 3, 2023

well I am on home addition and have tasklist but no qprocess..
it seems more ubiquitous than qprocess at least

@sebshader
Copy link
Contributor

sebshader commented Aug 3, 2023

https://superuser.com/questions/1440493/how-to-get-query-command-on-windows-10

Actually all query.exe does is delegate to a bunch of other executables: quser.exe, qappsrv.exe, qprocess.exe, qwinsta.exe. For query.exe to work, you need those EXEs, none of which are included in Win10 Home.

thanks for leaving the documentation to a random SuperUser response, microsoft

@Lucarda
Copy link
Contributor Author

Lucarda commented Aug 3, 2023

will work on it. Thanks for the valuable test. have to test if tasklist works on xp

@umlaeute
Copy link
Contributor

umlaeute commented Aug 3, 2023

i thought a better way would be to use DDE as Pd is already using this on Windows (to open documents with an already running instance).

however, it also seems that the current implementation uses the full path to the pd-gui.tcl file into the key, which we do not know during the installer.

pure-data/tcl/pd-gui.tcl

Lines 720 to 723 in c0a7c16

package require dde ;# 1.4 or later needed for full unicode support
set topic "Pure_Data_DDE_Open ${::pdgui::scriptname}"
# if no DDE service is running, start one and claim the name
if { [dde services TclEval $topic] == {} } {

probably a running Pd(-GUI) should also register a more generic key which can be checked to see if there is any Pd(-GUI) running, and possibly foreground it (or quit it).

also, I do not really know how easy it is to interact with DDE from NSIS (there is a very dated nsisDDE plugin at http://wiz0u.free.fr/prog/nsisDDE/, but I think we should probably avoid it)

@Lucarda Lucarda marked this pull request as draft August 3, 2023 09:58
@Lucarda
Copy link
Contributor Author

Lucarda commented Aug 3, 2023

@sebshader can you test b0e8ab4 ?

one good thing about tasklist is that it works even without %windir%\sysnative so the installer can be done with 64bit nsis.

@umlaeute yes, let's try to avoid using nsis plugins.

@sebshader
Copy link
Contributor

@Lucarda yep works fine here now

@Lucarda Lucarda marked this pull request as ready for review August 4, 2023 10:45
@umlaeute
Copy link
Contributor

umlaeute commented Aug 8, 2023

@Lucarda should we collect the feedback about the installer here? or on the mailinglist? (or can you setup a poll on your nextcloud instance?)

@Lucarda
Copy link
Contributor Author

Lucarda commented Aug 8, 2023

should we collect the feedback about the installer here? or on the mailinglist?

I think the mailinglist is Ok (more people getting the call).

If we find troubles with tasklist reported there, may be, we should resort to https://nsis.sourceforge.io/NsProcess_plugin. or live without detection?.

@umlaeute
Copy link
Contributor

umlaeute commented Aug 8, 2023

Following the call on the mailinglist, I'm going to collect the results here:

OS cpu state tester notes
WinXP (pro) i386 🎉 @Lucarda
Win7 (home) amd64 🎉 @Lucarda
Win8.1 (pro) amd64 🎉 @Lucarda
Win10 (home) amd64 🎉 @Lucarda
Win10 (enterprise) amd64 🎉 @Lucarda
Win11 (pro) amd64 🎉 @Lucarda
Win11 (Business) 10.0.22621 Build 22621 amd64 🎉 @ben-wes non-admin
Win11 (Business) 10.0.22621 Build 22621 i386 🎉 @ben-wes non-admin

@ben-wes
Copy link
Contributor

ben-wes commented Aug 8, 2023

feel free to remove this comment again in order to not bury the summarizing table above ...

since it's not mentioned there: it works perfectly well including the warning for a running Pd instance on:

  • Microsoft Windows 11 Business 10.0.22621 Build 22621
  • 11th Gen Intel(R) Core(TM) i7-1165G7

(installed at user scope - since no admin rights available here)

@umlaeute
Copy link
Contributor

umlaeute commented Aug 8, 2023

@ben-wes thanks. since your "11th Gen Intel(R) Core(TM) i7-1165G7" is capable of both x86_32 (i386) and x86_64 (amd64): did you test the 32bit or the 64bit installer?

@ben-wes
Copy link
Contributor

ben-wes commented Aug 8, 2023

@umlaeute : good point. sorry i didn't clarify. had only tested Pd-0.54-x.windows-installer.exe. but now i tested both with parallel install and running instances of the same version, other version, previous 0.53.2 32bit admin install (installed while i still had those privileges) and no running instance. everything works well here! unfortunately can't test the admin scope anymore though.

EDIT: to clarify after @Lucarda comment below: by these two versions, i refer to Pd-0.54-x-i386.windows-installer.exe and Pd-0.54-x.windows-installer.exe

@Lucarda
Copy link
Contributor Author

Lucarda commented Aug 8, 2023

@umlaeute to not get in confusions: both of the installers are 32bit nsis.

@umlaeute
Copy link
Contributor

umlaeute commented Aug 8, 2023

@Lucarda i'm not at all confused.
i think the architecture of the installer-binary itself is not really important (mostly, because there is only one), unlike the architecture of the pd.exe (for which there is a choice).

so for now, i would avoid talking about the architecture of the installer as it doesn't give us any additional information, and - as you say - will just confuse people.

@Lucarda
Copy link
Contributor Author

Lucarda commented Aug 8, 2023

@umlaeute warning!

the installer-binary itself is not really important

it is. we get 32bit Windows programs from from 32bit nsis. see above: we had to do tricks (cd %windir%\sysnative to use qprocess from a 32bit nsis on 64bit Windows.

installer-binary itself is not really important (mostly, because there is only one)

there is no "only one" on msys2. in the docs we say you can build nsis installers with 64bit and 32bit nsis packages for msys2. I choose to build only on 32bit 'nsis` as this one is the only arch for on Linux package managers (and Miller uses that).

EDIT: While working on the initial qprocess detection it took me 2 days to figure out why it worked for me building on Windows and not if building with Linux. It turned i was building with nsis64 on Windows. See #2067 (comment)

@umlaeute
Copy link
Contributor

umlaeute commented Aug 8, 2023

Sure, but that's an implementation detail. Esp with your call for feedback in mind, there is no mention/hint/... of a "64bit installer" (nsis64), but there is a "64bit (Pd) installer".

The context where I used "64bit installer" was when talking to a user who took the question about "CPU" literally and reported their intel model, while the question was really "which of the two installers from the download folder did you use?"

@Lucarda
Copy link
Contributor Author

Lucarda commented Aug 14, 2023

tested to work fine on Win7 (home) amd64

@Lucarda
Copy link
Contributor Author

Lucarda commented Oct 23, 2023

closing as the commits are already on the master branch.

@Lucarda Lucarda closed this Oct 23, 2023
@umlaeute
Copy link
Contributor

@Lucarda now that the branch has been merged, you could delete it from your repository :-)
https://github.com/lucarda/pure-data/branches

@Lucarda Lucarda deleted the nsis-safer branch October 23, 2023 16:07
@Lucarda
Copy link
Contributor Author

Lucarda commented Oct 23, 2023

done

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

Successfully merging this pull request may close these issues.

None yet

4 participants