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

Windows: Cpack wix installer #6153

Merged
merged 7 commits into from Oct 26, 2018

Conversation

@adrido
Copy link
Contributor

adrido commented Jul 19, 2017

This allows CMake/CPack to use the Windows Installer XML (WIX) to create a Installer package.

The Installer package is only available if its not a RUN_IN_PLACE build. For that the zip package is generated.

Todo:

  • Uninstaller should clean the cache dir. *
  • Question: should uninstaller also remove the minetest.conf? *
  • Each sub-game should have its own component
    • "minimal" sub-game should may not installed by default only optional

*= Applications are installed for all users, while config file and cache is per user. Removing this while uninstalling is neither a good idea nor supported by MSI.
Related: #4931, #3596
Fixes: #4528, #3802, #4697

Download:

Minetest-0.5.0-win32.msi.zip
Minetest-0.5.0-win64.msi.zip

The installer here is packed in a zip, because Github does not allow MSI files.

Screenshots:

grafik
grafik
grafik
The minimal subgame have to be manually selected.
grafik
grafik
grafik

@nerzhul

This comment has been minimized.

Copy link
Member

nerzhul commented Jul 20, 2017

interesting !

@adrido adrido force-pushed the adrido:cpack-wix-installer branch 4 times, most recently from b6ab529 to 30223b7 Jul 26, 2017

@adrido

This comment has been minimized.

Copy link
Contributor Author

adrido commented Jul 28, 2017

Should be ready for review now.

@adrido adrido changed the title WIP: Cpack wix installer Windows: Cpack wix installer Jul 28, 2017

set(CPACK_CREATE_DESKTOP_LINKS ${PROJECT_NAME})

set(CPACK_WIX_PRODUCT_ICON "${CMAKE_CURRENT_SOURCE_DIR}/misc/minetest-icon.ico")
# Supported languages can be found at http://wixtoolset.org/documentation/manual/v3/wixui/wixui_localization.html

This comment has been minimized.

@SmallJoker

SmallJoker Jul 28, 2017

Member

Line too long


set(CPACK_RESOURCE_FILE_LICENSE "${CMAKE_CURRENT_SOURCE_DIR}/doc/lgpl-2.1.txt")

# The correct way would be to include both x32 and x64 into one installer and install the approitive one.

This comment has been minimized.

@SmallJoker

SmallJoker Jul 28, 2017

Member

Too long here too
EDIT: approitive -> appropriate (?)

README.txt Outdated
In Visual Studio 2017 Installer select "optional Features" -> "Wix Toolset"

Build the binaries like described above, but make shure you unselect "RUN_IN_PLACE".
"RUN_IN_PLACE" builds are not designed for installations and leads to errors if installd on newer Windows since XP!

This comment has been minimized.

@SmallJoker

SmallJoker Jul 28, 2017

Member

* make sure
and leads to errors. (shorten and remove "installed" misspelling)

Show resolved Hide resolved CMakeLists.txt
@@ -727,8 +727,8 @@ if(MSVC)
set(CMAKE_CXX_FLAGS_DEBUG "/MDd /Zi /Ob0 /Od /RTC1")

# Flags for C files (sqlite)
# /MT = Link statically with standard library stuff
set(CMAKE_C_FLAGS_RELEASE "/O2 /Ob2 /MT")
# /MD = Link statically to MSVCRT.lib and dynamically to MSVCR<version>.DLL

This comment has been minimized.

@sfan5

sfan5 Jul 29, 2017

Member

this is worded confusingly, just say dynamically link to MSVCR dll

Show resolved Hide resolved src/porting.cpp
path_share = std::string(buf) + "\\..";
path_share = exepath + "\\..";
if (detectMSVCBuildDir(exepath)) {
// The msvc build dir schould normaly not present if properly installed,

This comment has been minimized.

@sfan5

sfan5 Jul 29, 2017

Member

... not be present ...

# Include all dynamically linked runtime libaries
include(InstallRequiredSystemLibraries)

if(RUN_IN_PLACE)

This comment has been minimized.

@sfan5

sfan5 Jul 29, 2017

Member

since this whole process seems to require building on Windows,
shouldn't it still generate ZIPs for RUN_IN_PLACE=0 compiled via MinGW?

This comment has been minimized.

@adrido

adrido Jul 30, 2017

Author Contributor

This would not make much sense.

Also this would cause much confusion to players if accidentally shared.
e.g. forum posts like this would occur:
"PLASE HELP Mods don't load", "Where is my minetest.conf?"

It does not explicit requires building with MSVC, maybe even MinGW is supported. There is a standalone package available: http://wixtoolset.org/
It just depends on CMake to support that. (not tested)

This comment has been minimized.

@sfan5

sfan5 Jul 30, 2017

Member

So what happens if I compile using MinGW on Linux (-> no WiX toolset) with RUN_IN_PLACE=0 and run make package?

Also this would cause much confusion to players if accidentally shared.

Why? It would use the exact same runtime paths as if it was installed using the installer.

This comment has been minimized.

@adrido

adrido Jul 30, 2017

Author Contributor

So what happens if I compile using MinGW on Linux (-> no WiX toolset) with RUN_IN_PLACE=0 and run make package?

a) nothing
b) it prints an error message
c) it prints a warning and generates a zip package anyway
d) it generates a zip package without any other message
e) it downloads and installs the wix toolset and generates a msi package

Well I don't know. 😆 How did you generate your 7zip packages you uploaded here? You could probably do the same only as zip format.

Why? It would use the exact same runtime paths as if it was installed using the installer.

The problem is to tell the kiddies that "zip package 1" finds its mods and conf in a different place then "zip package 2" 🤔

This comment has been minimized.

@sfan5

sfan5 Jul 30, 2017

Member

Well I don't know. 😆 How did you generate your 7zip packages you uploaded here? You could probably do the same only as zip format.

I use make package but that's not the point.
The problem I have here is that you are willfully breaking package creation when cross-compiling for Windows with RUN_IN_PLACE=0

(also since you didn't seem to understand: the WiX toolset does not work on Linux, like not at all)

The problem is to tell the kiddies that "zip package 1" finds its mods and conf in a different place then "zip package 2" 🤔

That's true I guess.
But IMO trying to protect devs from building broken packages is not useful, previously the build system did not prevent building RUN_IN_PLACE=0 ZIP packages either.

@Calinou

This comment has been minimized.

Copy link
Member

Calinou commented Jul 29, 2017

Note that it's not required for the users to accept the LGPL license in order to play Minetest, so the "license" step of the installer can be removed (to make installation a bit quicker).

(GPL FAQ)

@adrido adrido force-pushed the adrido:cpack-wix-installer branch 4 times, most recently from db1d443 to 03bbfb9 Jul 30, 2017

@adrido

This comment has been minimized.

Copy link
Contributor Author

adrido commented Jul 30, 2017

Note that it's not required for the users to accept the LGPL license in order to play Minetest, so the "license" step of the installer can be removed

The problem is, if no licence file is speciefied, CMake generates and shows a dummy file. This reads like "The package creator was too lazy to choose a real license". So its imo the best to show users that Minetest is open source, licensed under LGPL. Other OS programs shows the GPL or LGPL license file too while installing e.g. Gimp

@sfan5 sfan5 added the Feature label Jul 30, 2017

Show resolved Hide resolved CMakeLists.txt
README.txt Outdated
In Visual Studio 2017 Installer select "optional Features" -> "Wix Toolset"

Build the binaries like described above, but make sure you unselect "RUN_IN_PLACE".
"RUN_IN_PLACE" builds are not designed for installations and leads to errors

This comment has been minimized.

@sfan5

sfan5 Jul 30, 2017

Member

this is also confusing because creating RUN_IN_PLACE=1 installer builds is impossible (prevented by the build system)

This comment has been minimized.

@adrido

adrido Jul 30, 2017

Author Contributor

I can remove the last sentence if think its better.

@adrido adrido force-pushed the adrido:cpack-wix-installer branch 2 times, most recently from 892b156 to b493275 Jul 31, 2017

@adrido adrido force-pushed the adrido:cpack-wix-installer branch 2 times, most recently from c6ce24f to b6ab203 Aug 18, 2017

@adrido

This comment has been minimized.

Copy link
Contributor Author

adrido commented Aug 18, 2017

Rebased.

@adrido adrido force-pushed the adrido:cpack-wix-installer branch from b6ab203 to 4882f6b Aug 20, 2017

@Fixer-007

This comment has been minimized.

Copy link
Contributor

Fixer-007 commented Mar 3, 2018

BTW, why it says "this feature requires 0 kb" on screenshot N3?

@Calinou

This comment has been minimized.

Copy link
Member

Calinou commented Mar 3, 2018

  1. Installing programs always requires advanced privileges, even if you have full access to the install destination, because the installer creates an entry in the "Installed Program's" list.

This is not true; you can create user-wide Start Menu entries in %APPDATA%\Microsoft\Windows\Start Menu\Programs. See my Godot Inno Setup installer for an example of an installer that doesn't ask for administrator privileges.

It's probably a wiser idea to install user-wide by default, it makes for a smoother experience for nearly everyone as no UAC prompts are displayed when installing.

My builds does not have gettext enabled, because of licensing trouble: Gettext does not allow to be shared together with Microsoft redistributables.

Can you please link justification for that? As far as I know, Microsoft C++ redistributables fall under the "system library" exception as defined by the (L)GPL.

@adrido

This comment has been minimized.

Copy link
Contributor Author

adrido commented Mar 3, 2018

BTW, why it says "this feature requires 0 kb" on screenshot N3?

I had already installed it, and reinstalled it with a installer that had a different signature/timestamp/whatever but 0 bytes difference. 8)

This is not true; you can create user-wide Start Menu entries in %APPDATA%\Microsoft\Windows\Start Menu\Programs. See my Godot Inno Setup installer for an example of an installer that doesn't ask for administrator privileges.

I guess you did misunderstand me. The "Installed Programs" is not the list in the startmenu. Its the list displayed in system control under "Add and remove Programs" or "All Apps" on Windows 10.

Can you please link justification for that? As far as I know, Microsoft C++ redistributables fall under the "system library" exception as defined by the (L)GPL.

https://www.gnu.org/licenses/gpl-faq.en.html#WindowsRuntimeAndGPL
2nd paragraph.
Im not a lawyer, but as far I understood it, the redistributables in their dll form are not system libraries.
But discussing this here would be offtopic.

@Fixer-007

This comment has been minimized.

Copy link
Contributor

Fixer-007 commented Mar 3, 2018

I've rechecked 32 bit installer, works fine to me, installing textures/games/mods into %appdata%/minetest works, few wishes:

  • create empty folders in %appdata%/minetest for assets, aka games, mods, textures, so player can recognise were to put addons
  • locales support
  • I'm still convinced you can install without admin privs, windows 7 even can add it to "Programs" list
@Calinou

This comment has been minimized.

Copy link
Member

Calinou commented Mar 3, 2018

I'm still convinced you can install without admin privs, windows 7 even can add it to "Programs" list

Indeed, as an example, my Godot installer appears in Programs and Features once installed.

@Fixer-007

This comment has been minimized.

Copy link
Contributor

Fixer-007 commented Mar 3, 2018

Maybe it is MSI windows installer limitation?

@adrido

This comment has been minimized.

Copy link
Contributor Author

adrido commented Mar 4, 2018

Oh, I have to apologize, you both were right. MSI supports perUser installation and the WixToolset too. (http://wixtoolset.org/documentation/manual/v3/xsd/wix/package.html) The problem is, that CMake/CPack does not offer a switch for it (https://cmake.org/cmake/help/v3.10/module/CPackWIX.html). It would possible to workaround that using a patch file, but that would make it more complex and thus more unlikely to merge.
Apart from that in the last years of working with Windows I had only one Installer that did a perUser installation: The whatsapp "native" desktop client. (I immediately uninstalled it.)
The correct place for programs is still C:\Program Files\ and if someone want to use it only for himself, he can still use the portable version.
If someone does not have administrative privileges, he may not allowed to install software neither a per user installation nor a per machine installation. If a child want to install it on the family computer it have to ask it parents for permissions (which is absolutely correct).

the multilanguage installer is a feature request since many years for cmake: https://gitlab.kitware.com/cmake/cmake/issues/14925

The empty folders may added later, first this have to be merged, everything else would wasted time. That may also a good idea for the linux users, since their installed version is also missing them.

@rubenwardy

This comment has been minimized.

Copy link
Member

rubenwardy commented May 14, 2018

I'd like to see this for 0.5.0. What remains to be done?

@adrido

This comment has been minimized.

Copy link
Contributor Author

adrido commented May 15, 2018

From my side its ready. I would also like to see this in 0.4.17+

@rubenwardy
Copy link
Member

rubenwardy left a comment

Tested by multiple users, code looks ok

@sfan5 sfan5 added the One approval label Oct 25, 2018

@ClobberXD

This comment has been minimized.

Copy link
Contributor

ClobberXD commented Oct 26, 2018

@adrido Would you be able to resolve the merge conflict please?

Show resolved Hide resolved CMakeLists.txt Outdated

@adrido adrido force-pushed the adrido:cpack-wix-installer branch from f8b23e1 to ccde870 Oct 26, 2018

Update CMakeLists.txt
Co-Authored-By: adrido <robots_only_adrido@gmx.com>
@sfan5

sfan5 approved these changes Oct 26, 2018

@sfan5 sfan5 added >= Two approvals and removed One approval labels Oct 26, 2018

@SmallJoker SmallJoker merged commit 2322078 into minetest:master Oct 26, 2018

1 check passed

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

osjc added a commit to osjc/minetest that referenced this pull request Jan 23, 2019

Windows: Cpack wix installer (minetest#6153)
Create CPack WIX msi Installer for RUN_IN_PLACE=0 builds
Correct paths on Windows for RUN_IN_PLACE=0
Install only required font files
Games have their own components, and "minimal" is optional

@adrido adrido deleted the adrido:cpack-wix-installer branch Feb 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.