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

[WIP] 32 and 64 bit installer #476

Merged
merged 13 commits into from Sep 21, 2018

Conversation

Lucarda
Copy link
Contributor

@Lucarda Lucarda commented Sep 18, 2018

This allows to have the 32bit and 64bit Pd installed on the same system without clashing.

It does:

  • Individual start menu entries.
  • Individual desktop icons.
  • Individual default installation paths.

Tested and both installations can coexist with no trouble but more work on src/s_file.c and tcl/pd_guiprefs.tcl needs to be done so that each Pd has its own 'windows registry' settings.

Copy link
Contributor

@umlaeute umlaeute left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems to be an awful lot of duplicate code. i think would be much better to have a single script (both build-nsi.sh and pd.nsi) that take parameters to be instrumented for 32bit or 64bit. then small wrapper scripts to call the generic ones with the proper parameters.

@umlaeute
Copy link
Contributor

umlaeute commented Sep 18, 2018

also i don't understand why you changed the install dir to Pd-32bit and Pd-64bit.
shouldn't the location (%ProgramFiles% vs %ProgramFiles(x86)%) be enough to separate them (don't try to optimize for the obscure case, where somebody wants to install both 32bit and 64bit versions side-by-side into the same directory).

@Lucarda
Copy link
Contributor Author

Lucarda commented Sep 18, 2018

@danomatika and @umlaeute

These two files needs Pure-Data-32 / Pure-Data-64 ifs.

I can't handle these C / Tcl.

Help needed!

:)

@Lucarda
Copy link
Contributor Author

Lucarda commented Sep 18, 2018

also i don't understand why you changed the install dir to Pd-32bit and Pd-64bit

This can be changed but it was on purpose to make it totally clear.

@umlaeute
Copy link
Contributor

These two files needs Pure-Data-32 / Pure-Data-64 ifs.
https://github.com/pure-data/pure-data/blob/master/src/s_file.c#L284
https://github.com/pure-data/pure-data/blob/master/tcl/pd_guiprefs.tcl#L153
I can't handle these C / Tcl.

on the C-side, the fix is trivial.
the question is more, how to do that properly (for all platforms, not just Win).

in the Tcl/Tk side, i'm not entirely sure, whether we should change it at all.
there is no proper way, to detect (from the GUI-side) which architecture the Pd-core is running (at this stage in the startup phase).
a better idea would be to be able to set the registry base key (or base path or whatever is used on the other systems) by a single configuration file that resides in the tcl/ folder (and which could be generated by the installer - either online or offline)

also i don't understand why you changed the install dir to Pd-32bit and Pd-64bit

This can be changed but it was on purpose to make it totally clear.

why?
i mean, it's probably fine to add such info to the start-menu entry, but there's little point in adding it to the installation path (there's so much information we might want to make totally clear; think C:\Archivos de programa (x86)\Pd-0.49-1 for Windows (64bit) doubleprecision by Miller Puckette - a bit like MaxMSP but BSD3 licensed\ ;-) more seriously, it adds redundancy which can easily be made self-contradictory (in the given example: why is a 64bit binary installed in the 32bit wow path?))

@Spacechild1
Copy link
Contributor

shouldn't the location (%ProgramFiles% vs %ProgramFiles(x86)%) be enough to separate them

I rather agree with @umlaeute. although some programs do this (e.g. REAPER (x64)), I don't think it's necessary.

@Lucarda
Copy link
Contributor Author

Lucarda commented Sep 18, 2018

shouldn't the location (%ProgramFiles% vs %ProgramFiles(x86)%) be enough to separate them

Ok, no trouble at all with that.

:)

@Lucarda
Copy link
Contributor Author

Lucarda commented Sep 18, 2018

in the Tcl/Tk side, i'm not entirely sure, whether we should change it at all.
there is no proper way, to detect (from the GUI-side) which architecture the Pd-core is running (at this stage in the startup phase).
a better idea would be to be able to set the registry base key (or base path or whatever is used on the other systems) by a single configuration file that resides in the tcl/ folder (and which could be generated by the installer - either online or offline)

This is Ok if possible with the installer but what about the Zip app?

You already did the magic detection with Deken, I guess this detection is late?

Idea: Now that we have pref file could we not use at all the 'windows registry' exept for file association?

@umlaeute
Copy link
Contributor

umlaeute commented Sep 18, 2018 via email

@Lucarda
Copy link
Contributor Author

Lucarda commented Sep 18, 2018

  • one registry root for the 32bit installation
  • another registry root for the 64bit installation
  • yet another registry root for all ZIP "installations".

How you do the "registry root for all ZIP "installations"?
The installer is made from the zip version.
what did you meant with:

by a single configuration file that resides in the tcl/ folder (and which could be generated by the installer - either online or offline)

?

@umlaeute
Copy link
Contributor

How you do the "registry root for all ZIP "installations"?
The installer is made from the zip version.

use the default registry root for ZIP "installations".
add an override of the default for NSI-installations (while building the NSI installer).

what did you meant with:

by a single configuration file that resides in the tcl/ folder (and which could be generated by the installer - either online or offline)

"put a single file containing the registry root into the tcl/-folder. this file could be generated by the installer-script; or missing."
this sounds an awful lot like what I wrote in my incomprehensible style, so I guess it is not much better. which part should i clarify?

@Lucarda
Copy link
Contributor Author

Lucarda commented Sep 18, 2018

"put a single file containing the registry root into the tcl/-folder. this file could be generated by the installer-script; or missing."
this sounds an awful lot like what I wrote in my incomprehensible style, so I guess it is not much better. which part should i clarify?

I think I got it. Some txt with just:

"HKEY_CURRENT_USER\\Software\\Pure-Data-32"

or

"HKEY_CURRENT_USER\\Software\\Pure-Data-64"

?

I like it. :)

@Lucarda
Copy link
Contributor Author

Lucarda commented Sep 18, 2018

I think the Txt thing resolves everything.

:)

@umlaeute
Copy link
Contributor

yep, something like this. i'd like to hear the opinion of others on this though (@danomatika :-))

@Lucarda
Copy link
Contributor Author

Lucarda commented Sep 18, 2018

Ok, I'll try to make a nicer nsi script

:).

@Lucarda
Copy link
Contributor Author

Lucarda commented Sep 19, 2018

@umlaeute I made the changes that you requested, can you review?

The thing now goes with makensis -DARCHI=32 pd.nsi

@Lucarda
Copy link
Contributor Author

Lucarda commented Sep 19, 2018

Something went wrong when trying to do some little fix.

I'll do another PR because I cant fix this one any more.

But this one works with ./build-nsi.sh <path-to-pd-dir> <pd-version-number> <arch (32 or 64)>

@umlaeute
Copy link
Contributor

??

anyhow, while doing a new PR please also get rid of the ...\Pd-${ARCHI}bit\ directory.

@umlaeute
Copy link
Contributor

here's another point via ...\Pd-${ARCHI}bit\ is a bad idea (apart from my personal preferences):

pd-lib-builder will stop automatically finding the Pd sources for your architecture.

so changing the installation-path is a regression.

msw/pd.nsi Outdated
;
; removed as of 0.49 to prevent 32/64 installer clashing. No negative side effects were observed.
;
; !define PRODUCT_DIR_REGKEY "Software\Microsoft\Windows\CurrentVersion\App Paths\pd.exe"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reduzent what was the original reason to add the PRODUCT_DIR_REGKEY

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most likely it there was no particular reason, but it got there because it was in the original template that I edited

@Lucarda
Copy link
Contributor Author

Lucarda commented Sep 19, 2018

get rid of the ...\Pd-${ARCHI}bit\ directory.

there's no such dir.
Is the 'start menu' / 'desktons icons' thing.

msw/pd.nsi Outdated
; removed as of 0.49 to prevent 32/64 installer clashing. No negative side effects were observed.
;
;
; InstallDirRegKey HKLM "${PRODUCT_DIR_REGKEY}" ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking up the NSIS Documentation, I think the usage of this is to remember the installation directory across re-installs. As such, it seems clear that their are no immediate side-effects visible when doing a single installation. The change will only become apparent in consecutive use of the installer.
As such, I guess it should stay in (probably with a explanatory comment why it should stay)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that is present you get the wrong default installation dir for the parallel 2nd installation something like: c:\program files\pd\bin. I tested it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess, that's because the InstallDirRegKey must be different for 32bit and 64bit installations

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to play with these 4 that were removed:

!define PRODUCT_DIR_REGKEY "Software\Microsoft\Windows\CurrentVersion\App Paths\pd.exe"

InstallDirRegKey HKLM "${PRODUCT_DIR_REGKEY}" ""

WriteRegStr HKLM "${PRODUCT_DIR_REGKEY}" "" "$INSTDIR\bin\pd.exe"

DeleteRegKey HKLM "${PRODUCT_DIR_REGKEY}"

may be:

InstallDirRegKey HKLM "${PRODUCT_DIR_REGKEY}" "${ARCHI}"

?

@umlaeute
Copy link
Contributor

Is the 'start menu' / 'desktons icons' thing.

ah indeed. static code analysis only gets you so far, if you don't know the eco-system well.
sorry for the noise

@pure-data pure-data deleted a comment from Lucarda Sep 19, 2018
@pure-data pure-data deleted a comment from Lucarda Sep 19, 2018
@pure-data pure-data deleted a comment from Lucarda Sep 19, 2018
@pure-data pure-data deleted a comment from Lucarda Sep 19, 2018
@Lucarda
Copy link
Contributor Author

Lucarda commented Sep 19, 2018

Ok, I have tested and the only known bug is that if you install first the 32 and then the 64, if 64 gets uninstalled the 32 loses the file association.

Not much a big problem as you can fix it with "open with" from the explorer.

@umlaeute can you review?

This only fixes the 32/64 installer. Work must be done on the C/tcl side so that the 32 app don't share the registry with the 64 app.

The installer is ready.

@Lucarda
Copy link
Contributor Author

Lucarda commented Sep 19, 2018

@millerpuckette

This allows to have 32 and 64 bit Pd installed on the same system.
It defaults installation to:

  • 64bit-OS 64bit-Pd: "%Program Files%\Pd"
  • 64bit-OS 32bit-Pd: "%Program Files (x86)%\Pd"
  • 32bit-OS 32bit-Pd: "%Program Files%\Pd"

It creates different:

  • Start-menu entries
  • Desktop icons

Some work must be done outside of the installer so that both apps don't share the same 'registry' but all that can be done with the installer's scripts is done in this PR.

usage:

./build-nsi.sh <path-to-pd-dir> <pd-version-number> <arch (32 or 64)>

:)

@danomatika
Copy link
Contributor

Looks good to me.

@umlaeute
Copy link
Contributor

i've updated the build-nsi.sh script to automatically detect the arch value if it is missing (please test :-))

"Pd-32bit" looks a bit...

LATER: think of getting rid of the ARCHI when it's not needed
(e.g. on Win/32bit we can only ever have 32bit applications)
@Lucarda
Copy link
Contributor Author

Lucarda commented Sep 21, 2018

fdf39d7 is running nicely.

@Lucarda
Copy link
Contributor Author

Lucarda commented Sep 21, 2018

Why did you erase the commits? they were almost going. :)

I got that part but I'm starting to fell asleep.

will hang out 1 hour more or less :)

@umlaeute
Copy link
Contributor

umlaeute commented Sep 21, 2018

i erased them because they are in the wrong PR (switching the file-association to Pd-GUI was not going to work without the changes from #481)

so i moved them all over to #481

@millerpuckette millerpuckette merged commit f7ff9b3 into pure-data:master Sep 21, 2018
@Lucarda
Copy link
Contributor Author

Lucarda commented Sep 21, 2018

Oops. Can you undo this merge.

Is was included on #481

@Lucarda
Copy link
Contributor Author

Lucarda commented Sep 21, 2018

Sorry for the noise master seems Ok.

@Lucarda Lucarda deleted the windows32and64installer branch September 21, 2018 19:56
@pure-data pure-data deleted a comment from Spacechild1 Dec 10, 2019
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

6 participants