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

WIN32: Rewrite Inno setup installer script. #1324

Merged
merged 8 commits into from Sep 5, 2018

Conversation

Projects
None yet
4 participants
@lotharsm
Member

lotharsm commented Sep 1, 2018

This is a rewrite of the Inno setup installer script.

  • Information like AppVersion and Copyright is now directly fetched from the scummvm.exe. Now we don't have to touch this file in case the ScummVM version increases.

- Moved the documentation files in a seperate subfolder and create links to them in the Start Menu to avoid cluttering the Start Menu on Windows 8 and higher.

  • Added language entries to the installer for all languages supported by translations.scummvm.org.

  • Add option to create a ScummVM shortcut to the desktop.

- Remove shortcuts to the "Save Games (old default)" folder since current versions of ScummVM won't even install on Windows versions lower than Windows XP.

I am a bit unsure what to do with the scummvm.iss.in file since all this seems to do is to contain an additional placeholder for the version numbers. Since this script fetches it directly from the executable, the scummvm.iss.in file shouldn't be needed anymore?

@lotharsm lotharsm requested review from sev- and Kirben Sep 1, 2018

@lotharsm

This comment has been minimized.

Show comment
Hide comment
@lotharsm

lotharsm Sep 1, 2018

Member

I wonder if we should also move the default installation directory from {pf} to {userappdata} so we don't need elevated rights anymore.

Member

lotharsm commented Sep 1, 2018

I wonder if we should also move the default installation directory from {pf} to {userappdata} so we don't need elevated rights anymore.

@bluegr

This comment has been minimized.

Show comment
Hide comment
@bluegr

bluegr Sep 1, 2018

Member

Overall, excellent work!
The change to the default installation directory is a valid point, but how will the existing installations be affected? Will the previous uninstaller remove the old files, so that the new installer places them in the new folder?

Member

bluegr commented Sep 1, 2018

Overall, excellent work!
The change to the default installation directory is a valid point, but how will the existing installations be affected? Will the previous uninstaller remove the old files, so that the new installer places them in the new folder?

@lotharsm

This comment has been minimized.

Show comment
Hide comment
@lotharsm

lotharsm Sep 1, 2018

Member

Sadly not. Updating an existing folder without uninstalling the old version first will lead to a "duplicate" installation - the old one in Program Files, the new one in AppData. Since the shortcuts are updated by the new installer, this is a minor issue IMHO.

Member

lotharsm commented Sep 1, 2018

Sadly not. Updating an existing folder without uninstalling the old version first will lead to a "duplicate" installation - the old one in Program Files, the new one in AppData. Since the shortcuts are updated by the new installer, this is a minor issue IMHO.

@Kirben

This comment has been minimized.

Show comment
Hide comment
@Kirben

Kirben Sep 1, 2018

Member

The documentation files are on the Start Menu for easy access, we shouldn't be forcing users to use Windows Explorer for any access.

The migration tools are required for anyone who used old ScummVM version under Windows XP/Vista/7 too. The saved games location changed from '\Program Files\ScummVM' to userappdata in the distant past.

{userappdata} is only meant for application data, applications should never be installed in that area.

Otherwise the other changes seem fine.

Member

Kirben commented Sep 1, 2018

The documentation files are on the Start Menu for easy access, we shouldn't be forcing users to use Windows Explorer for any access.

The migration tools are required for anyone who used old ScummVM version under Windows XP/Vista/7 too. The saved games location changed from '\Program Files\ScummVM' to userappdata in the distant past.

{userappdata} is only meant for application data, applications should never be installed in that area.

Otherwise the other changes seem fine.

@lotharsm

This comment has been minimized.

Show comment
Hide comment
@lotharsm

lotharsm Sep 1, 2018

Member

Thanks for your comments!

The documentation files are on the Start Menu for easy access, we shouldn't be forcing users to use Windows Explorer for any access.

Even though this looks not really great on Windows 8 and higher?

The migration tools are required for anyone who used old ScummVM version under Windows XP/Vista/7 too. The saved games location changed from '\Program Files\ScummVM' to userappdata in the distant past.

Okay, but how relevant do you think is an upgrade path from such ancient versions?

{userappdata} is only meant for application data, applications should never be installed in that area.

Okay, sounds plausible. What about {userpf} which doesn't need elevated rights too?

Member

lotharsm commented Sep 1, 2018

Thanks for your comments!

The documentation files are on the Start Menu for easy access, we shouldn't be forcing users to use Windows Explorer for any access.

Even though this looks not really great on Windows 8 and higher?

The migration tools are required for anyone who used old ScummVM version under Windows XP/Vista/7 too. The saved games location changed from '\Program Files\ScummVM' to userappdata in the distant past.

Okay, but how relevant do you think is an upgrade path from such ancient versions?

{userappdata} is only meant for application data, applications should never be installed in that area.

Okay, sounds plausible. What about {userpf} which doesn't need elevated rights too?

@Kirben

This comment has been minimized.

Show comment
Hide comment
@Kirben

Kirben Sep 1, 2018

Member

The ScummVM documentation files on the Start Menu looked no worse on Windows 10 last I checked, or do you mean the touch style interface? the documentation files could be moved to Documents sub folder on the Start Menu.

It does seem unlikely for people to upgrade from an ScummVM version released over 6 years ago, but at least keep a link to current saved games location, to make it easier for back ups.

{userappdata} and {userpf} use similar paths, so the later is no better. ScummVM been installed to {pf] means it can be used by all users, and elevated rights exist so people can choose to block installers.

Member

Kirben commented Sep 1, 2018

The ScummVM documentation files on the Start Menu looked no worse on Windows 10 last I checked, or do you mean the touch style interface? the documentation files could be moved to Documents sub folder on the Start Menu.

It does seem unlikely for people to upgrade from an ScummVM version released over 6 years ago, but at least keep a link to current saved games location, to make it easier for back ups.

{userappdata} and {userpf} use similar paths, so the later is no better. ScummVM been installed to {pf] means it can be used by all users, and elevated rights exist so people can choose to block installers.

@lotharsm

This comment has been minimized.

Show comment
Hide comment
@lotharsm

lotharsm Sep 1, 2018

Member

The problem for me is that Windows 10 doesn't support sub-folders on the Start Menu anymore, so that's not an option unfortunately. Maybe it's just my personal preference, but having all the document shortcuts sitting directly in the Program group is not very nice...

It does seem unlikely for people to upgrade from an ScummVM version released over 6 years ago, but at least keep a link to current saved games location, to make it easier for back ups.

Okay, I'll do this then.

{userappdata} and {userpf} use similar paths, so the later is no better. ScummVM been installed to {pf] means it can be used by all users, and elevated rights exist so people can choose to block installers.

Okay, I'll leave this at {pf}.

Member

lotharsm commented Sep 1, 2018

The problem for me is that Windows 10 doesn't support sub-folders on the Start Menu anymore, so that's not an option unfortunately. Maybe it's just my personal preference, but having all the document shortcuts sitting directly in the Program group is not very nice...

It does seem unlikely for people to upgrade from an ScummVM version released over 6 years ago, but at least keep a link to current saved games location, to make it easier for back ups.

Okay, I'll do this then.

{userappdata} and {userpf} use similar paths, so the later is no better. ScummVM been installed to {pf] means it can be used by all users, and elevated rights exist so people can choose to block installers.

Okay, I'll leave this at {pf}.

@lotharsm

This comment has been minimized.

Show comment
Hide comment
@lotharsm

lotharsm Sep 2, 2018

Member

Kirben, I reverted some of my changes according to your comment. Is everything fine now in your opinion or do you have any more suggestions? Thanks for your help with this!

Member

lotharsm commented Sep 2, 2018

Kirben, I reverted some of my changes according to your comment. Is everything fine now in your opinion or do you have any more suggestions? Thanks for your help with this!

Show resolved Hide resolved dists/win32/ScummVM.iss
@Kirben

This comment has been minimized.

Show comment
Hide comment
@Kirben

Kirben Sep 3, 2018

Member

Seems fine to merge, other than that small change required.

The win32dist target in ports.mk will need 'scummvm-install.bmp' added too, but I can do that separately.

Member

Kirben commented Sep 3, 2018

Seems fine to merge, other than that small change required.

The win32dist target in ports.mk will need 'scummvm-install.bmp' added too, but I can do that separately.

@lotharsm

This comment has been minimized.

Show comment
Hide comment
@lotharsm

lotharsm Sep 3, 2018

Member

Kirben, do you know if the file ScummVM.iss.in is still required?

Member

lotharsm commented Sep 3, 2018

Kirben, do you know if the file ScummVM.iss.in is still required?

@Kirben

This comment has been minimized.

Show comment
Hide comment
@Kirben

Kirben Sep 3, 2018

Member

ScummVM.iss.in file isn't used by Inno Setup but the *.in files seems to be used by one of the build systems.

Member

Kirben commented Sep 3, 2018

ScummVM.iss.in file isn't used by Inno Setup but the *.in files seems to be used by one of the build systems.

@criezy

This comment has been minimized.

Show comment
Hide comment
@criezy

criezy Sep 3, 2018

Member

Now we don't have to touch this file in case the ScummVM version increases.

You are not supposed to touch directly the ScummVM.iss when increasing the ScummVM version. Instead it would automatically be updated alongside all the other files that contain the version by running update-version.pl.

The ScummVM.iss.in file (as well as other files with the .in suffix) is used by update-version.pl to generate the ScummVM.iss file by substituting the @VERSION@ with the actual version (it also substitutes @VER_MAJOR@, @VER_MINOR@, @VER_PATCH@ and @VER_EXTRA@).

If your updated ScummVM.iss no longer contains the version, then you should indeed remove the ScummVM.iss.in AND update devtools/update-version.pl so that it no longer tries to process this file. Our wiki (notably http://wiki.scummvm.org/index.php/HOWTO-Release) will also need to be updated to reflect the change.

Member

criezy commented Sep 3, 2018

Now we don't have to touch this file in case the ScummVM version increases.

You are not supposed to touch directly the ScummVM.iss when increasing the ScummVM version. Instead it would automatically be updated alongside all the other files that contain the version by running update-version.pl.

The ScummVM.iss.in file (as well as other files with the .in suffix) is used by update-version.pl to generate the ScummVM.iss file by substituting the @VERSION@ with the actual version (it also substitutes @VER_MAJOR@, @VER_MINOR@, @VER_PATCH@ and @VER_EXTRA@).

If your updated ScummVM.iss no longer contains the version, then you should indeed remove the ScummVM.iss.in AND update devtools/update-version.pl so that it no longer tries to process this file. Our wiki (notably http://wiki.scummvm.org/index.php/HOWTO-Release) will also need to be updated to reflect the change.

@lotharsm

This comment has been minimized.

Show comment
Hide comment
@lotharsm

lotharsm Sep 3, 2018

Member

if your updated ScummVM.iss no longer contains the version,

Correct, the ScummVM.iss script doesn't contain any version information now and just pulls the necessary information right from the executable file.

then you should indeed remove the ScummVM.iss.in AND update devtools/update-version.pl so that it no longer tries to process this file. Our wiki (notably http://wiki.scummvm.org/index.php/HOWTO-Release) will also need to be updated to reflect the change.

Thanks, I'll do this then.

Member

lotharsm commented Sep 3, 2018

if your updated ScummVM.iss no longer contains the version,

Correct, the ScummVM.iss script doesn't contain any version information now and just pulls the necessary information right from the executable file.

then you should indeed remove the ScummVM.iss.in AND update devtools/update-version.pl so that it no longer tries to process this file. Our wiki (notably http://wiki.scummvm.org/index.php/HOWTO-Release) will also need to be updated to reflect the change.

Thanks, I'll do this then.

@lotharsm

This comment has been minimized.

Show comment
Hide comment
@lotharsm

lotharsm Sep 4, 2018

Member

Do you consider this ready to be merged now or is there something left to do?

Member

lotharsm commented Sep 4, 2018

Do you consider this ready to be merged now or is there something left to do?

@Kirben

Kirben approved these changes Sep 5, 2018

@Kirben Kirben merged commit f10816d into scummvm:master Sep 5, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Kirben

This comment has been minimized.

Show comment
Hide comment
@Kirben

Kirben Sep 5, 2018

Member

Yes, merged as requested, thank you for the changes.

Member

Kirben commented Sep 5, 2018

Yes, merged as requested, thank you for the changes.

@lotharsm lotharsm deleted the lotharsm:innosetup-rewrite branch Sep 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment