Add Export/Import commands #699

florelis · 2021-01-13T01:34:06Z

This implements a prototype of the import and export commands. Opening as draft to gather feedback on schema and command options.
Overview of changes:

Added a schema for a list of packages in JSON format. This lists packages (id, version, channel) and the sources they come from (name, arg, type, id).
Export command with syntax winget export [--output] file.json [common query filters]. This creates a list of the matching packages. Packages that are not available from any source are still listed so that they can be imported if they are added later to a source.
Import command with syntax winget import [--input] file.json [--exactVersions]. This installs all the packages from a list. Each package is installed from the source listed from it, unless there is no source specified in which case it is taken from any available. Installs latest version, unless the --exactVersions flag is used.

Still missing tests & resource strings.

Related: #155, #156, #220

Microsoft Reviewers: Open in CodeFlow

JohnMcPMS

We need to ensure that everyone is on the same page about the direction here before more work is done. Some of my comments may not apply afterward, but I have given feedback based on how I expect export/import to work.

doc/packages.schema.json

src/AppInstallerCLICore/Argument.cpp

src/AppInstallerCLICore/Commands/ExportCommand.cpp

src/AppInstallerCLICore/Commands/ImportCommand.cpp

src/AppInstallerCLICore/Workflows/ImportExportFlow.cpp

JohnMcPMS · 2021-01-14T02:10:15Z

src/AppInstallerCLICore/Workflows/ImportExportFlow.cpp

+
+            context << OpenNamedSource(requiredSource.Details.Name);
+            auto source = context.Get<Execution::Data::Source>();
+            aggregatedSource->AddAvailableSource(source);


This pattern does not force the source to actually be used for a given package, which is what we want. The goal should be that if a package should come from a specific source, then that source should be used exclusively. Otherwise, the standard unnamed source (all combined) should be used.

This pattern does not force the source to actually be used for a given package

What do you mean?

You are finding source 1, then adding it to the aggregate, then searching. All of these packages will come from the correct source.

Then you find source 2, add it to the aggregate, and search. These packages could be coming from source 1 or 2.

You should simply open the source and use it for the packages in question. No need to use the aggregate.

When I do the search, I'm not doing it in context which has the aggregate, but in searchContext which only has the individual source, so it should use only that one source unless I'm missing something.

I'm using the aggregate because some step of the installation (can't remember which atm) references the package's source so we need to keep it alive somehow (as the package has only a weak reference). Maybe keeping it as the root context's Source was not the best idea, more so since the composite source wasn't intended to be public. I can change it to keep around the sources as a list in the context.

But this all depends on me wanting to do all the searches before installing. If we change it to searching right before each install as you say, the aggregate can go away.

JohnMcPMS · 2021-01-14T02:16:51Z

src/AppInstallerCLICore/Workflows/ImportExportFlow.cpp

+                Execution::Context& searchContext = *searchContextPtr;
+                searchContext.Add<Execution::Data::Source>(source);
+
+                // TODO: Case insensitive?


Finding the appropriate package and version to install is going to be far more complex than this. We may not have the Id handy; we may just have the name and publisher and will require the ability to find something based on that.

But I think that having a review on the spec and schema will help solidify the requirements.

JohnMcPMS · 2021-01-14T02:21:26Z

src/AppInstallerCLICore/Workflows/InstallFlow.cpp

+
+            // Extract the data needed for installing
+            installContext.Add<Execution::Data::PackageVersion>(package);
+            installContext.Add<Execution::Data::Manifest>(package->GetManifest());


Is there not already a workflow task that does this step?

There is a GetManifest task but it does more work than is needed here (e.g. checking the arguments and using them to select the version). Here we want to seed the sub context with info we already have so that it can start doing its work.

* Made source details required in schema. * Stopped using CompositeSource in ImportFlow and instead moved to keeping a list of sources in the context. * Stopped selecting version to export from available ones, and instead always use installed one, warning if it is not available. * Stopped exporting unavailable packages. * Stopped allowing to import packages with no source. * Use existing code to select version to install. * Moved strings for JSON from .h to .cpp * Renamed PackageRequest class

github-actions · 2021-01-30T03:40:36Z

New misspellings found, please review:

noversion
validator
valijson
withversion

To accept these changes, run the following commands

perl -e '
my @expect_files=qw('".github/actions/spelling/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"AComment addressof ALIGNAS allescaped alse ARMEL backend bbwe bitfield blep BORLAND cassert cctype cdunn cerr clocale CMake cmath cname codepoint cplusplus CPPLIB cstddef cstdio cstdlib cstr cstring CStyle ctor czstring deque deref dllimport dnp elif emark endcode endverbatim EOL eturn eyc fpclassify gcc GNUC hpux HREF hrow ieeefp inl iosfwd isfinite Isfinitef isnan isprint IString javascript jsonvalue keey keylength lconv len Lepilleur localeconv lossfree malloc maxsize memcmp memset MINGW modf msvc Multiline mutators nfinity noreturn NULLREF nvcc NVIDIAs OString ote ptrdiff py Skype skypeapp Solaris sourceforge sout SOVERSION ssin stackoverflow STARTUPINFOW stdarg strchr strcmp strdup strlen strncmp unindent Unserialize usf Utils valueiterator vscprintf vsnprintf walkaround wiki wikipedia Workaround wstringstream xf xl "');
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)(?:$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect.txt";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"noversion utils validator valijson withversion "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
git add .github/actions/spelling || echo '... you want to ensure .github/actions/spelling/expect.txt is added to your repository...'

florelis · 2021-02-02T02:34:38Z

src/AppInstallerCLICore/PackageCollection.cpp

+
+        std::optional<PackageCollection> ParseJson(const Json::Value& root)
+        {
+            // TODO: Embed schema in binaries & validate file


I had some trouble getting this to work. @yao-msft is also working on something with JSON schemas and seems to have this figured out, so I'll wait for him to check in his change and then hook this up with his code.

github-actions · 2021-02-03T02:02:35Z

New misspellings found, please review:

wcout
wofstream
wostream

To accept these changes, run the following commands

perl -e '
my @expect_files=qw('".github/actions/spelling/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"AComment addressof ALIGNAS allescaped alse ARMEL backend bbwe bitfield blep BORLAND cassert cctype cdunn cerr clocale CMake cmath cname codepoint cplusplus CPPLIB cstddef cstdio cstdlib cstr cstring CStyle ctor czstring deque deref dllimport dnp elif emark endcode endverbatim EOL eturn eyc fpclassify gcc GNUC hpux HREF hrow ieeefp inl iosfwd isfinite Isfinitef isnan isprint IString javascript jsonvalue keey keylength lconv len Lepilleur localeconv lossfree malloc maxsize memcmp memset MINGW modf msvc Multiline mutators nfinity noreturn NULLREF nvcc NVIDIAs OString ote ptrdiff py Skype skypeapp Solaris sourceforge sout SOVERSION ssin stackoverflow STARTUPINFOW stdarg strchr strcmp strdup strlen strncmp unindent Unserialize usf validator valijson valueiterator vscprintf vsnprintf walkaround wiki wikipedia Workaround xf xl "');
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)(?:$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect.txt";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"wcout wofstream wostream "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
git add .github/actions/spelling || echo '... you want to ensure .github/actions/spelling/expect.txt is added to your repository...'

JohnMcPMS · 2021-02-03T17:56:42Z

New misspellings found, please review:

wcout

wofstream

wostream

The presence of these suggests misuse; neither our production code nor our tests should be using (w)cout directly. [Although I do frequently do some local debugging of tests with them.]

github-actions · 2021-02-08T23:31:34Z

Misspellings found, please review:

wstringstream

To accept these changes, run the following commands from this repository on this branch

pushd $(git rev-parse --show-toplevel)
perl -e '
my @expect_files=qw('".github/actions/spelling/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"validator valijson valueiterator "');
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);
make_path ".github/actions/spelling";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"wstringstream "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
popd

jsoref · 2021-02-08T23:47:44Z

.github/actions/spelling/allow.txt

@@ -450,6 +450,7 @@ VOS
 vso
 wapproj
 wchar
+wcout


#699 (comment)

This is not in the product code or tests directly, but in a standalone .exe used as a test installer. It was mixing narrow and wide chars and that was getting complicated, but it didn't seem worth it to add conversion between them there, so I moved it all to wide. It should be fine as it's independent of everything else.

The only thing I'd note is that by using it here instead of, e.g. just telling the bot to ignore the file entirely, the bot will never warn about the next violation.

Think of it like page-fault protection.

Obviously reviewers are responsible for the content, so in theory some reviewer should catch future incursions.

(Just feedback from the peanut gallery.)

schemas/JSON/packages/packages.schema.1.0.json

src/AppInstallerCLICore/Commands/ExportCommand.cpp

src/AppInstallerCLICore/Commands/ImportCommand.cpp

src/AppInstallerCLICore/Workflows/ImportExportFlow.cpp

src/AppInstallerCLICore/Workflows/WorkflowBase.cpp

src/AppInstallerCLITests/TestSource.h

src/AppInstallerRepositoryCore/CompositeSource.h

src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp

Co-authored-by: JohnMcPMS <[email protected]>

JohnMcPMS

Just want to clarify how version should be handled by default, as I believe that wanting a specific version is a bit of an edge case (at least wanting a specific version of every package). So I think that export should not include the version, but allow for it with a flag. And import should probably just have flag more like --ignoreVersions that will ignore every version rather than allow fallback to latest in some cases.

src/AppInstallerCLICore/Commands/ImportCommand.cpp

src/AppInstallerCLICore/Workflows/ImportExportFlow.cpp

quangkieu · 2021-02-12T20:35:39Z

@JohnMcPMS how about by default is >= export version. User could add the top limit

JohnMcPMS · 2021-02-12T20:00:43Z

src/AppInstallerCLICore/Workflows/ImportExportFlow.cpp

-                version.get(),
-                channel.get());
+            // but take the exported version from the installed package if needed.
+            if (context.Args.Contains(Execution::Args::Type::IncludeVersions))


I think we could be more efficient above if we inspected this flag to prevent the need to even match on an available version.

Updated to do that.

Luis Enrique Chacon Ochoa added 4 commits January 12, 2021 16:56

Prototype of import/export

dac55b5

Preserve unavailable packages on export; add option for exact versions

4d6c28a

typo

60109c5

Lowercase command names

01c6b86

florelis requested a review from a team January 13, 2021 22:54

JohnMcPMS requested changes Jan 14, 2021

View reviewed changes

ghost added Needs-Author-Feedback Issue needs attention from issue or PR author and removed Needs-Author-Feedback Issue needs attention from issue or PR author labels Jan 14, 2021

Luis Enrique Chacon Ochoa added 7 commits January 21, 2021 12:22

Remove query arguments for export (except source)

c89e8a0

Use Workflow::VerifyFile for import

6e0830f

Move argument definitions to be in command definitions

d015f37

Find sources by identifier instead of argument

9842b1f

Add tests for workflow & detect installed packages

c6408d7

Add unit tests for JSON read/write

8122ea8

Luis Enrique Chacon Ochoa added 2 commits February 1, 2021 09:21

Spell checking

8b07d23

Disable schema validation

c82d9cb

florelis commented Feb 2, 2021

View reviewed changes

florelis requested a review from JohnMcPMS February 2, 2021 02:34

florelis marked this pull request as ready for review February 2, 2021 02:35

Luis Enrique Chacon Ochoa added 3 commits February 2, 2021 13:03

Add E2E tests

3c45df8

Track installed version in test exe installer

86101a8

Uninstall test exe before tests

508d094

Spell checking

b176066

Luis Enrique Chacon Ochoa added 3 commits February 3, 2021 14:08

Fix test failure

1a30962

Move and rename packages schema file

576c051

Merge branch 'master' into ImportExport

20aba26

Spell checking

5bcd4a2

jsoref reviewed Feb 8, 2021

View reviewed changes

Fix error codes in tests after merge

33ba202

JohnMcPMS requested changes Feb 11, 2021

View reviewed changes

ghost added Needs-Author-Feedback Issue needs attention from issue or PR author and removed Needs-Author-Feedback Issue needs attention from issue or PR author labels Feb 11, 2021

Luis Chacón and others added 9 commits February 10, 2021 17:19

Apply suggestions from code review

660387d

Co-authored-by: JohnMcPMS <[email protected]>

Remove exact versions import argument

20984c6

Match sources with identifier and type

7eb3359

Add execution stages

4c93054

Add --force argument to skip missing packages

08b1604

Rename ParseJson -> TryParseJson

dc8bcad

Use workflow task for building Sources vector

f7a9c43

Fix tests

4b59e1a

Change composite search behavior from flag to enum

8b45f6d

JohnMcPMS requested changes Feb 12, 2021

View reviewed changes

src/AppInstallerCLICore/Commands/ImportCommand.cpp Outdated Show resolved Hide resolved

src/AppInstallerCLICore/Commands/ImportCommand.cpp Show resolved Hide resolved

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

ghost added the Needs-Author-Feedback Issue needs attention from issue or PR author label Feb 12, 2021

Luis Enrique Chacon Ochoa added 2 commits February 11, 2021 17:24

Address comments from review

58f6fe4

Update behavior around versions

1357453

ghost removed the Needs-Author-Feedback Issue needs attention from issue or PR author label Feb 12, 2021

Fix tests

a09ad02

JohnMcPMS approved these changes Feb 12, 2021

View reviewed changes

Don't check that installed version is available unless needed

abd0e4c

florelis merged commit 009e684 into microsoft:master Feb 13, 2021

denelon linked an issue Mar 4, 2021 that may be closed by this pull request

Export/Import apps list #220

Closed

florelis deleted the ImportExport branch April 2, 2021 23:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Export/Import commands #699

Add Export/Import commands #699

florelis commented Jan 13, 2021 •

edited by ghost

Loading

JohnMcPMS left a comment

JohnMcPMS Jan 14, 2021

florelis Jan 14, 2021

JohnMcPMS Jan 14, 2021

florelis Jan 14, 2021

JohnMcPMS Jan 14, 2021

JohnMcPMS Jan 14, 2021

florelis Jan 26, 2021

github-actions bot commented Jan 30, 2021

florelis Feb 2, 2021

github-actions bot commented Feb 3, 2021

JohnMcPMS commented Feb 3, 2021

New misspellings found, please review:

github-actions bot commented Feb 8, 2021

jsoref Feb 8, 2021

florelis Feb 9, 2021

jsoref Feb 9, 2021

JohnMcPMS left a comment

quangkieu commented Feb 12, 2021

JohnMcPMS Feb 12, 2021

florelis Feb 13, 2021

@@ @@ -450,6 +450,7 @@ VOS @@
               vso
               wapproj
               wchar
+              wcout

Add Export/Import commands #699

Add Export/Import commands #699

Conversation

florelis commented Jan 13, 2021 • edited by ghost Loading

Microsoft Reviewers: Open in CodeFlow

JohnMcPMS left a 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

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 Jan 30, 2021

New misspellings found, please review:

Choose a reason for hiding this comment

github-actions bot commented Feb 3, 2021

New misspellings found, please review:

JohnMcPMS commented Feb 3, 2021

New misspellings found, please review:

github-actions bot commented Feb 8, 2021

Misspellings found, please review:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnMcPMS left a comment

Choose a reason for hiding this comment

quangkieu commented Feb 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

florelis commented Jan 13, 2021 •

edited by ghost

Loading