-l,--location option not working on all msi packages

[Cube707]

Brief description of your issue

This issue is a followup on my PR for a package mainfest.

There seem to be a lot of inconsistencyes on how to set the install location path for msi packages. The code currently sets the TARGETDIR option.

winget-cli/src/AppInstallerCommonCore/Manifest/ManifestCommon.cpp

Line 461 in b79e408

{InstallerSwitchType::InstallLocation, ManifestInstaller::string_t("TARGETDIR=\"" + std::string(ARG_TOKEN_INSTALLPATH) + "\"")}

This is only one of two available options (see here) and some packages seem to require the alternative INSTALLDIR option.

I have tested the packages Oracle.JDK.17, StrawberryPerl.StrawberryPerl and ChristianHohnstadt.xca, which all currently don't work with the -l switch.

In contrast to that, the Borvid.HttpMasterExpress package works absoloutly fine as is.

One possible soloution would be to allways pass both of these options, which worked in my short testing.

Steps to reproduce

Use one of the following commands:

winget install Oracle.JDK.17 -l C:\ProgStuff\jdk
winget install Oracle.JDK.17 -l "C:\ProgStuff\jdk"
winget install Oracle.JDK.17 --override "/passive TARGETDIR=C:\ProgStuff\jdk"

The jdk is now installed into C:\Program Files\Java\jdk17 and NOT into the given location.

Only winget install Oracle.JDK.17 --override "/passive INSTALLDIR=C:\ProgStuff\jdk" works as expected.

Expected behavior

The package should be installed into the given Path.

Actual behavior

The given directory is ignored and the package is installed into the default directory.

Environment

[winget --info]
Windows Package Manager v1.1.13405
Copyright (c) Microsoft Corporation. Alle Rechte vorbehalten.

Windows: Windows.Desktop v10.0.19043.1415
Paket: Microsoft.DesktopAppInstaller v1.16.13405.0

Protokolle: %LOCALAPPDATA%\Packages\Microsoft.DesktopAppInstaller_8wekyb3d8bbwe\LocalState\DiagOutputDir

[OfficialEsco]

Sooo i learned something today in microsoft/winget-pkgs#41913 (comment)
I would bet ANY installer that does not respect -l is actually wix instead of msi 🤦‍♂️🤦‍♂️🤦‍♂️
I don't know my way around this repo, but this tends to be where my searches lead me

winget-cli/src/AppInstallerCommonCore/Manifest/ManifestCommon.cpp

Lines 451 to 481 in b79e408

	switch (installerType)
	{
	case InstallerTypeEnum::Burn:
	case InstallerTypeEnum::Wix:
	case InstallerTypeEnum::Msi:
	return
	{
	{InstallerSwitchType::Silent, ManifestInstaller::string_t("/quiet")},
	{InstallerSwitchType::SilentWithProgress, ManifestInstaller::string_t("/passive")},
	{InstallerSwitchType::Log, ManifestInstaller::string_t("/log \"" + std::string(ARG_TOKEN_LOGPATH) + "\"")},
	{InstallerSwitchType::InstallLocation, ManifestInstaller::string_t("TARGETDIR=\"" + std::string(ARG_TOKEN_INSTALLPATH) + "\"")}
	};
	case InstallerTypeEnum::Nullsoft:
	return
	{
	{InstallerSwitchType::Silent, ManifestInstaller::string_t("/S")},
	{InstallerSwitchType::SilentWithProgress, ManifestInstaller::string_t("/S")},
	{InstallerSwitchType::InstallLocation, ManifestInstaller::string_t("/D=" + std::string(ARG_TOKEN_INSTALLPATH))}
	};
	case InstallerTypeEnum::Inno:
	return
	{
	{InstallerSwitchType::Silent, ManifestInstaller::string_t("/VERYSILENT")},
	{InstallerSwitchType::SilentWithProgress, ManifestInstaller::string_t("/SILENT")},
	{InstallerSwitchType::Log, ManifestInstaller::string_t("/LOG=\"" + std::string(ARG_TOKEN_LOGPATH) + "\"")},
	{InstallerSwitchType::InstallLocation, ManifestInstaller::string_t("/DIR=\"" + std::string(ARG_TOKEN_INSTALLPATH) + "\"")}
	};
	default:
	return {};
	}
	}

And from my understand winget-cli uses the same switches for burn, wix and msi, if someone could double check the InstallerSwitches for burn and wix we over at the winget-pkgs could fix the InstallerTypes

cc @ItzLevvie @jedieaston @Trenly @denelon

winget-pkgs progress: https://github.com/microsoft/winget-pkgs/search?p=1&q=InstallerType%3A+wix&type=code

[Trenly]

Apparently they should be the same?
https://stackoverflow.com/a/44347702/9998654

[jedieaston]

It looks like WiX doesn't have a standard argument for install location, but by convention people use INSTALLLOCATION? https://stackoverflow.com/questions/24266720/how-to-define-installation-folder-from-command-line-parameter-in-wix-installer

If we add INSTALLLOCATION=<location> to the custom arguments on a MSI manifest does it work?

[dorssel]

Just my 2 cents.

WiX has always used something different than TARGETDIR. The reason is that WiX actually does it "wrong" (or at least: their advertised examples).

The MSI specs are very clear: TARGETDIR is to be used if you want the non-standard installation folder. See: https://docs.microsoft.com/en-us/windows/win32/msi/targetdir (first paragraph).

To accomplish this, a MSI should set the TARGETDIR property if and only if it is not set by the user. Something like (pseudo code):

if TARGETDIR is not set:
   set TARGETDIR = %ProgramFiles%\Vendor\Product

This is clear from https://docs.microsoft.com/en-us/windows/win32/msi/using-the-directory-table.

This can be easily accomplished in WiX as well, by a <SetProperty Id="TARGETDIR" Value="<XXX>" Before="CostInitialize">NOT TARGETDIR</SetProperty>. However, almost no one actually does this. Most WiX users go for the standard WiX MinimalUI or WiX AdvancedUI templates. Those are really old (10+ years) and not well maintained. Besides, most online examples are equally old and this keeps the "myth alive".

Most WiX examples use:

<Directory Id="TARGETDIR" Name="SourceDir">
    <Directory Id="ProgramFiles64Folder">
        <Directory Id="Vendor" />
            <Directory Id="APPLICATIONFOLDER" Name="YYY" />
        </Directory>
    </Directory>
</Directory>

The problem is that even if you override TARGETDIR, the next "child directory" (note: that is not the same as a subdirectory) immediately get set to %ProgramFiles64Folder%, as that is a predefined property. As a result, the TARGETDIR gets completely ignored and everything ends up below %ProgramFiles64Folder% again. Unless you override the very last directory in this list. In the example above that would be APPLICATIONFOLDER. That is what current WiX examples use: https://wixtoolset.org//documentation/manual/v3/wixui/dialog_reference/wixui_advanced.html. But in the past, it has been INSTALLLOCATION. And in fact, it could be anything. Even worse: if the Id of the final destination directory is not spelled in ALL-CAPPS, then you cannot even override it at all.

To be clear, the above example should read:

<SetProperty Id="TARGETDIR" Value="<enter default here>" Before="CostInitialize">NOT TARGETDIR</SetProperty>
<Directory Id="TARGETDIR" Name="SourceDir">

but (again): nobody actually does this.

ADDED:
This is how I got my MSI installer to play nice with both WiX and winget:
https://github.com/dorssel/usbipd-win/blob/7072c6275f1499f8b165f6cf7d1278d602403b66/Installer/Product.wxs#L49-L57

[marshallwp]

How the -l and --location options work is defined by the InstallLocation field of the Installer Manifest. As an example of how to get the -l and --location options working with WiX installers, check out the PuTTY manifest. Specifically lines 14-15:

InstallerSwitches:
  InstallLocation: INSTALLDIR="<INSTALLPATH>"

In other words, this is not an issue with the client, but rather an issue with incomplete manifest files.

[ItzLevvie]

InstallLocation is actually defined automatically depending on the InstallerType in the manifest:

winget-cli/src/AppInstallerCommonCore/Manifest/ManifestCommon.cpp

Lines 477 to 509 in 5d63c38

	std::map<InstallerSwitchType, Utility::NormalizedString> GetDefaultKnownSwitches(InstallerTypeEnum installerType)
	{
	switch (installerType)
	{
	case InstallerTypeEnum::Burn:
	case InstallerTypeEnum::Wix:
	case InstallerTypeEnum::Msi:
	return
	{
	{InstallerSwitchType::Silent, ManifestInstaller::string_t("/quiet")},
	{InstallerSwitchType::SilentWithProgress, ManifestInstaller::string_t("/passive")},
	{InstallerSwitchType::Log, ManifestInstaller::string_t("/log \"" + std::string(ARG_TOKEN_LOGPATH) + "\"")},
	{InstallerSwitchType::InstallLocation, ManifestInstaller::string_t("TARGETDIR=\"" + std::string(ARG_TOKEN_INSTALLPATH) + "\"")}
	};
	case InstallerTypeEnum::Nullsoft:
	return
	{
	{InstallerSwitchType::Silent, ManifestInstaller::string_t("/S")},
	{InstallerSwitchType::SilentWithProgress, ManifestInstaller::string_t("/S")},
	{InstallerSwitchType::InstallLocation, ManifestInstaller::string_t("/D=" + std::string(ARG_TOKEN_INSTALLPATH))}
	};
	case InstallerTypeEnum::Inno:
	return
	{
	{InstallerSwitchType::Silent, ManifestInstaller::string_t("/VERYSILENT")},
	{InstallerSwitchType::SilentWithProgress, ManifestInstaller::string_t("/SILENT")},
	{InstallerSwitchType::Log, ManifestInstaller::string_t("/LOG=\"" + std::string(ARG_TOKEN_LOGPATH) + "\"")},
	{InstallerSwitchType::InstallLocation, ManifestInstaller::string_t("/DIR=\"" + std::string(ARG_TOKEN_INSTALLPATH) + "\"")}
	};
	default:
	return {};
	}
	}

This is the reason why not all MSI and WIX manifests in the winget-pkgs repository have it. It's also the same with InstallerSwitches.

It does look like some installers work a bit differently so the snippet of code above will need a bit of adjustments. This is down to the differences between WIX and MSI noted in #1857 (comment) which is why the PuTTY manually declares INSTALLDIR instead of TARGETDIR.

Also the --location parameter with winget install can be a bit finicky depending on whether you're using it in Command Prompt or Windows PowerShell or other terminals. See #1639.

[marshallwp]

@ItzLevvie I'd still consider this an issue better addressed by modifying the manifest files. It's certainly not a bug (as this issue is labled) if winget does not implicitly pickup on non-standard switches implemented by developers. Especially if that can be worked around with a properly defined manifest file.

[dinhani]

Some MSI files like Zeal are using INSTALL_ROOT to define the install location. Setting the value with --custom INSTALL_ROOT=path/to/dir works.

I am not sure if this is a standard property, but it seems there are more applications using it.