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 Workflow logs and fix installed version workflow bug #4432

Merged
merged 5 commits into from
May 1, 2024

Conversation

JohnMcPMS
Copy link
Member

@JohnMcPMS JohnMcPMS commented Apr 30, 2024

Fixes #4425

Change

This adds a Workflow channel (disabled by default) that has the entry to tasks and the Get/Add/Contains calls logged to it. This enables a view into a flow that can be collected by a third party.

It also fixes a behavior change with the side-by-side code path for selecting the installed version that would lead to no data item being inserted, when the previous behavior was that a nullptr would be inserted when no version is installed.

Validation

Manual validation of the workflow output, including using the windbg command to see the function name (for function pointer tasks).
Manual validation of at least one case of the GetVariant(14) issue being fixed.

Microsoft Reviewers: Open in CodeFlow

@JohnMcPMS JohnMcPMS requested a review from a team as a code owner April 30, 2024 22:38
break;
}

UNREFERENCED_PARAMETER(map);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we plan to use this variable in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had considered a much more complex runtime checking with tasks actually having information on which data they produced/consumed, but realized that a runtime check is only as good as the tests and so provided very little value. I didn't feel like removing the parameter at that point.

@@ -1365,6 +1365,8 @@ namespace AppInstaller::Repository
}
}

using namespace anon;
Copy link
Contributor

Choose a reason for hiding this comment

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

curious why do we need to change the empty namespace?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've learned that anonymous namespace does not output type information into the PDB in a way that debuggers can find currently, making it much harder to debug. With a named namespace, the derived type information is found and so the debugger can show the member fields of a derived type (vtable -> type mapping). One can manually do all of this with enough effort, and maybe eventually the debuggers will support it directly, but at this time I see simply naming the namespace as the easiest answer.

@JohnMcPMS JohnMcPMS merged commit 2b185be into microsoft:master May 1, 2024
8 checks passed
@JohnMcPMS JohnMcPMS deleted the workflow-logs branch May 1, 2024 16:16
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.

sideBySide Experimental Feature Causing GetVariant(14) 0x80070490 Error - Winget v1.8.1133-preview
2 participants