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

Add support for in-proc Com state separation #2068

Merged
merged 9 commits into from
Apr 8, 2022

Conversation

yao-msft
Copy link
Contributor

@yao-msft yao-msft commented Apr 5, 2022

Change

Added a process global PackageManagerSettings Com object to support better state separation for in-proc Com invocations.
Also make the package applicability check take in InstallOptions to give better results.

Validation

Added back unpackaged tests run under system context.
Manually tried calling the PackageManagerSettings and see logs, etc are saved under state specific folder.

Next is the Com e2e test.

Microsoft Reviewers: Open in CodeFlow

@github-actions
Copy link

github-actions bot commented Apr 5, 2022

@check-spelling-bot Report

Unrecognized words, please review:

  • accepteula
Previously acknowledged words that are now absent activatable amd Archs dsc Globals hackathon mytool Packagedx parametermap whatif
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 [email protected]:yao-msft/winget-cli.git repository
on the comstate 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/1088189740" > "$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

@yao-msft yao-msft marked this pull request as ready for review April 5, 2022 23:30
@yao-msft yao-msft requested a review from a team as a code owner April 5, 2022 23:31
UserSettings::UserSettings() : m_type(UserSettingsType::Default)
bool TryInitializeCustomUserSettings(std::string content)
{
if (s_userSettingsInitialized || s_userSettingsInInitialization)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably the right approach for now, but we should have a test to make sure that our suggested method for initialization is working so that we don't load the settings earlier than that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I verified manually it's working as expected, and it should and will be part of Com e2e test.

CoCreatableClassWrlCreatorMapInclude(PackageManager);
CoCreatableClassWrlCreatorMapInclude(FindPackagesOptions);
CoCreatableClassWrlCreatorMapInclude(CreateCompositePackageCatalogOptions);
CoCreatableClassWrlCreatorMapInclude(InstallOptions);
CoCreatableClassWrlCreatorMapInclude(UninstallOptions);
CoCreatableClassWrlCreatorMapInclude(PackageMatchFilter);
CoCreatableClassWrlCreatorMapInclude(PackageManagerSettings);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like the only thing stopping this from being used OOP is the fact that it isn't registered in our AppxManifest.xml. I don't know if that makes it impossible to create, or just harder.

Not that it would be the end of the world or anything, but I think we would prefer that they not create it OOP if that doesn't require a lot of work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After several hours of exploring, I think it requires some amount of work to do custom registration and maintaining the registrations. And it needs more thorough testing if we choose that route. So I'll leave it for now.

@yao-msft yao-msft merged commit 4c44131 into microsoft:master Apr 8, 2022
@yao-msft yao-msft deleted the comstate branch April 8, 2022 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants