Implementation for Portable install flow #2078

ryfu-msft · 2022-04-08T18:40:39Z

Related to:

Support for installation of portable/standalone apps #182

This PR add support for the installation of portable standalone apps. The design aligns with what is specified in the portable install design spec. This PR only relates to the install command. Support for upgrade and uninstall will be handled in a separate PR.

Tests:

Added tests for modifying registry keys
Added portable workflow test
Added E2E tests for testing various portable install scenarios such as command field specified, apps and feature entry specified, and rename argument provided.

Microsoft Reviewers: Open in CodeFlow

… portableInstallFlow

src/AppInstallerCLICore/Workflows/PortableInstallFlow.cpp

src/AppInstallerCommonCore/Public/winget/PortableARPEntry.h

Trenly · 2022-04-25T21:43:53Z

src/AppInstallerCommonCore/PortableARPEntry.cpp

+    constexpr std::wstring_view s_UninstallRegistryX64 = L"Software\\Microsoft\\Windows\\CurrentVersion\\Uninstall";
+    constexpr std::wstring_view s_UninstallRegistryX86 = L"Software\\Wow6432Node\\Microsoft\\Windows\\CurrentVersion\\Uninstall";
+    constexpr std::wstring_view s_DisplayName = L"DisplayName";
+    constexpr std::wstring_view s_DisplayVersion = L"DisplayVersion";
+    constexpr std::wstring_view s_Publisher = L"Publisher";
+    constexpr std::wstring_view s_InstallDate = L"InstallDate";
+    constexpr std::wstring_view s_URLInfoAbout = L"URLInfoAbout";
+    constexpr std::wstring_view s_HelpLink = L"HelpLink";
+    constexpr std::wstring_view s_UninstallString = L"UninstallString";
+    constexpr std::wstring_view s_WinGetInstallerType = L"WinGetInstallerType";
+    constexpr std::wstring_view s_InstallLocation = L"InstallLocation";
+    constexpr std::wstring_view s_PortableTargetFullPath = L"TargetFullPath";
+    constexpr std::wstring_view s_PortableSymlinkFullPath = L"SymlinkFullPath";
+    constexpr std::wstring_view s_SHA256 = L"SHA256";
+    constexpr std::wstring_view s_WinGetPackageIdentifier = L"WinGetPackageIdentifier";
+    constexpr std::wstring_view s_WinGetSourceIdentifier = L"WinGetSourceIdentifier";


Couldn't you put these inside of a map and then use the map as a dictionary later instead of using all of the if..else if..else statements in the ToString method?

Created a map for these strings as suggested

A switch statement would be the preferred way to do ToString. And I would use a macro to stamp out the cases since the enum value and static string can shared a common name fragment.

Added a macro to stamp out cases and use a switch statement. Removed map

.github/actions/spelling/expect.txt

src/AppInstallerCLICore/Workflows/DownloadFlow.cpp

src/AppInstallerCLICore/Workflows/InstallFlow.cpp

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw

src/AppInstallerCommonCore/Filesystem.cpp

src/AppInstallerCommonCore/PortableARPEntry.cpp

JohnMcPMS · 2022-04-26T17:55:56Z

src/AppInstallerCommonCore/PortableARPEntry.cpp

+    constexpr std::wstring_view s_UninstallRegistryX64 = L"Software\\Microsoft\\Windows\\CurrentVersion\\Uninstall";
+    constexpr std::wstring_view s_UninstallRegistryX86 = L"Software\\Wow6432Node\\Microsoft\\Windows\\CurrentVersion\\Uninstall";
+    constexpr std::wstring_view s_DisplayName = L"DisplayName";
+    constexpr std::wstring_view s_DisplayVersion = L"DisplayVersion";
+    constexpr std::wstring_view s_Publisher = L"Publisher";
+    constexpr std::wstring_view s_InstallDate = L"InstallDate";
+    constexpr std::wstring_view s_URLInfoAbout = L"URLInfoAbout";
+    constexpr std::wstring_view s_HelpLink = L"HelpLink";
+    constexpr std::wstring_view s_UninstallString = L"UninstallString";
+    constexpr std::wstring_view s_WinGetInstallerType = L"WinGetInstallerType";
+    constexpr std::wstring_view s_InstallLocation = L"InstallLocation";
+    constexpr std::wstring_view s_PortableTargetFullPath = L"TargetFullPath";
+    constexpr std::wstring_view s_PortableSymlinkFullPath = L"SymlinkFullPath";
+    constexpr std::wstring_view s_SHA256 = L"SHA256";
+    constexpr std::wstring_view s_WinGetPackageIdentifier = L"WinGetPackageIdentifier";
+    constexpr std::wstring_view s_WinGetSourceIdentifier = L"WinGetSourceIdentifier";


A switch statement would be the preferred way to do ToString. And I would use a macro to stamp out the cases since the enum value and static string can shared a common name fragment.

github-actions · 2022-04-27T18:49:25Z

@check-spelling-bot Report

Unrecognized words, please review:

nonterminated
VALUENAMECASE

Previously acknowledged words that are now absent

activatable amd Archs dsc enr FWW Globals hackathon lww mytool OSVERSION Packagedx parametermap symlink Uninitialize WDAG whatif wsb

To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the git@github.com:ryfu-msft/winget-cli.git repository
on the portableInstallFlow branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/winget-cli/issues/comments/1111362109" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u

ryfu-msft · 2022-04-27T20:53:41Z

/azp run

azure-pipelines · 2022-04-27T20:53:50Z

Azure Pipelines successfully started running 1 pipeline(s).

Trenly · 2022-04-27T19:36:13Z

src/AppInstallerCLICore/Workflows/PortableInstallFlow.cpp

+            }
+            else
+            {
+                source = s_LocalSource;


Do we want to include the asterisk in the package Identifier? Technically it is an allowed character, but knowing that users may use powershell, and for potential forward compatibility, it may be best to omit it from the product code

I think that for consistency this function should apply MakeSuitablePathPart before returning, which will remove the *.

That's probably a good call. As we don't have wildcard support, and some terminals might do unexpected things.

Trenly · 2022-04-27T20:23:02Z

src/AppInstallerCLICore/Workflows/PortableInstallFlow.cpp

+            AICLI_LOG(CLI, Info, << "Writing to Uninstall registry complete.");
+        }
+
+        void MovePortableExeAndCreateSymlink(Execution::Context& context)


I don't know what it is, but something about this name just doesn't seem right. I think it is the And which indicates that this task is doing two tasks and that it may be best to split it into one task for moving the exe, and one for creating the symlink? I'm not an expert though, so I'd like others' opinions on this too

cc @JohnMcPMS

I agree that it would generally make more sense to split the two operations if they are logically independent. I don't think that we need to now, but I definitely foresee the need for this in the future for portable(s) in an archive support, where it isn't just a single file and symlink. I think it can wait until then though.

As for the name, again I'm fine with it for now because I expect it to be refactored for the archive support.

src/AppInstallerCLIE2ETests/InstallCommand.cs

src/AppInstallerCLICore/Workflows/PortableInstallFlow.cpp

Trenly · 2022-04-27T20:54:03Z

src/AppInstallerCommonCore/DateTime.cpp

+        auto now = std::chrono::system_clock::now();
+        std::time_t tt = std::chrono::system_clock::to_time_t(now);
+
+        struct tm newTime;
+        localtime_s(&newTime, &tt);
+
+        std::stringstream ss;
+        ss << std::put_time(&newTime, "%Y%m%d");
+        return ss.str();


Do you have to use std::chrono::system_clock::now()? Or would this work?

Suggested change

auto now = std::chrono::system_clock::now();

std::time_t tt = std::chrono::system_clock::to_time_t(now);

struct tm newTime;

localtime_s(&newTime, &tt);

std::stringstream ss;

ss << std::put_time(&newTime, "%Y%m%d");

return ss.str();

std::time_t now = std::time(nullptr);

std::stringstream ss;

ss << std::put_time(std::localtime(&now), "%Y%m%d");

return ss.str();

(Just trying to learn, so this might not be right)

Looks correct to me (although nullptr is preferred over 0).

Although if we convert to C++20, we can just get the time formatting goodness and not use the icky C stuff.

JohnMcPMS · 2022-04-27T22:50:39Z

src/AppInstallerCLICore/Workflows/PortableInstallFlow.cpp

+            }
+            else
+            {
+                source = s_LocalSource;


I think that for consistency this function should apply MakeSuitablePathPart before returning, which will remove the *.

src/AppInstallerCLICore/Workflows/PortableInstallFlow.cpp

JohnMcPMS · 2022-04-27T23:03:25Z

src/AppInstallerCLICore/Workflows/PortableInstallFlow.cpp

+            AICLI_LOG(CLI, Info, << "Writing to Uninstall registry complete.");
+        }
+
+        void MovePortableExeAndCreateSymlink(Execution::Context& context)


I agree that it would generally make more sense to split the two operations if they are logically independent. I don't think that we need to now, but I definitely foresee the need for this in the future for portable(s) in an archive support, where it isn't just a single file and symlink. I think it can wait until then though.

As for the name, again I'm fine with it for now because I expect it to be refactored for the archive support.

src/AppInstallerCLICore/Workflows/PortableInstallFlow.cpp

JohnMcPMS · 2022-04-27T23:23:44Z

src/AppInstallerCommonCore/DateTime.cpp

+        auto now = std::chrono::system_clock::now();
+        std::time_t tt = std::chrono::system_clock::to_time_t(now);
+
+        struct tm newTime;
+        localtime_s(&newTime, &tt);
+
+        std::stringstream ss;
+        ss << std::put_time(&newTime, "%Y%m%d");
+        return ss.str();


Looks correct to me (although nullptr is preferred over 0).

Although if we convert to C++20, we can just get the time formatting goodness and not use the icky C stuff.

src/AppInstallerCommonCore/PortableARPEntry.cpp

Trenly · 2022-04-28T20:57:08Z

🎉

denelon · 2022-04-28T21:57:35Z

We will look to cut another preview build ASAP 😊

Trenly · 2022-04-28T21:58:37Z

We will look to cut another preview build ASAP 😊

Potential for CI/CD pre-releases after every PR merge?

JohnMcPMS · 2022-04-28T22:04:20Z

Potential for CI/CD pre-releases after every PR merge?

It would have to be of the dev package due to policies. But the pipeline could be updated to create a package, sign it, and leave it in the artifacts.

Trenly · 2022-04-28T22:06:41Z

Potential for CI/CD pre-releases after every PR merge?

It would have to be of the dev package due to policies. But the pipeline could be updated to create a package, sign it, and leave it in the artifacts.

I think that would be great to have a WingetDev build accessible in the public artifacts of a pipeline in case some power users want to really test things before they hit an official pre-release

Ryan Fu and others added 29 commits February 15, 2022 11:08

add initial cli arguments

90d186f

add proposed schema changes for 1.2

0f64c0c

make adjustments to changes

ec39660

update DefaultLocale

c6eee67

add initial commit for portable install

307f37a

Add initial portable flow and check for running exe

a63c7fe

save state

233e314

extend write to registry functionality

b7523ed

clean up code

ad1bcec

add support for progress

336c073

refactor portable code

f5b0aa7

revert changes

be0307b

add initial registry tests

5badd5b

cleanup code and list tests

876decf

save work

577f803

Write to path

4e03b2e

Merge branch 'master' of https://github.com/ryfu-msft/winget-cli into…

a92eb04

… portableInstallFlow

resolve merge conflicts

aa7e99e

clean up code

e4faa1f

fix spell check

8f7c498

revert installer manifest

82b39bd

refactor code

2954827

revert changes

ac12821

revert resource strings

63731b7

rearrange code

13065c6

refactor portable install flow

db5d9f3

add tests

c45fc1c

Add warning strings and cleanup tests

3d420d7

fix spelling

f361ae0

ryfu-msft requested a review from a team as a code owner April 8, 2022 18:40

fix cleanup on E2E tests

e11260f

microsoft deleted a comment from github-actions bot Apr 25, 2022

Trenly reviewed Apr 25, 2022

View reviewed changes

src/AppInstallerCLICore/Workflows/PortableInstallFlow.cpp Outdated Show resolved Hide resolved

src/AppInstallerCommonCore/Public/winget/PortableARPEntry.h Outdated Show resolved Hide resolved

Trenly reviewed Apr 25, 2022

View reviewed changes

fix trenly comments

2a45cb3

JohnMcPMS requested changes Apr 26, 2022

View reviewed changes

ghost added the Needs-Author-Feedback Issue needs attention from issue or PR author label Apr 26, 2022

fix reportInstallerResult and address comments

1886e68

ghost removed the Needs-Author-Feedback Issue needs attention from issue or PR author label Apr 27, 2022

Ryan Fu added 2 commits April 27, 2022 11:51

fix spelling

fcd3029

fix tests

5b5f13d

Trenly reviewed Apr 27, 2022

View reviewed changes

Ryan Fu and others added 2 commits April 27, 2022 15:17

remove portable checks from nonportable installertypes

2be3590

Merge branch 'master' into portableInstallFlow

8c0317d

JohnMcPMS requested changes Apr 27, 2022

View reviewed changes

ghost added the Needs-Author-Feedback Issue needs attention from issue or PR author label Apr 27, 2022

address final comments and add test for existing directory

b5c1ad2

ghost removed the Needs-Author-Feedback Issue needs attention from issue or PR author label Apr 28, 2022

fix error message for invalid reserve

1b1a7e2

JohnMcPMS approved these changes Apr 28, 2022

View reviewed changes

ryfu-msft merged commit 5d63c38 into microsoft:master Apr 28, 2022

ryfu-msft deleted the portableInstallFlow branch May 16, 2022 22:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation for Portable install flow #2078

Implementation for Portable install flow #2078

ryfu-msft commented Apr 8, 2022 •

edited

Loading

Trenly Apr 25, 2022

ryfu-msft Apr 26, 2022

JohnMcPMS Apr 26, 2022

ryfu-msft Apr 27, 2022

JohnMcPMS Apr 26, 2022

github-actions bot commented Apr 27, 2022 •

edited

Loading

ryfu-msft commented Apr 27, 2022

azure-pipelines bot commented Apr 27, 2022

Trenly Apr 27, 2022

JohnMcPMS Apr 27, 2022

denelon Apr 27, 2022

Trenly Apr 27, 2022

JohnMcPMS Apr 27, 2022

Trenly Apr 27, 2022 •

edited

Loading

JohnMcPMS Apr 27, 2022

JohnMcPMS Apr 27, 2022

JohnMcPMS Apr 27, 2022

JohnMcPMS Apr 27, 2022

Trenly commented Apr 28, 2022

denelon commented Apr 28, 2022

Trenly commented Apr 28, 2022

JohnMcPMS commented Apr 28, 2022

Trenly commented Apr 28, 2022

Implementation for Portable install flow #2078

Implementation for Portable install flow #2078

Conversation

ryfu-msft commented Apr 8, 2022 • edited Loading

Microsoft Reviewers: Open in CodeFlow

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 27, 2022 • edited Loading

@check-spelling-bot Report

Unrecognized words, please review:

ryfu-msft commented Apr 27, 2022

azure-pipelines bot commented Apr 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trenly Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trenly commented Apr 28, 2022

denelon commented Apr 28, 2022

Trenly commented Apr 28, 2022

JohnMcPMS commented Apr 28, 2022

Trenly commented Apr 28, 2022

ryfu-msft commented Apr 8, 2022 •

edited

Loading

github-actions bot commented Apr 27, 2022 •

edited

Loading

Trenly Apr 27, 2022 •

edited

Loading