[MSIX app attach] Enable logging of output to a file for the IT Pro. #600

Smohamed-MSFT · 2023-10-06T18:28:46Z

Why is this change being made?
Support IT Pro by allowing the output of logging errors to file.

What changed?
Added a -output flag that allow to specify a file location to output logs to after crash.

How was the change tested?
Validated by reverting to previous commit as the current wasn't building. Successfully output the errors to a log file when specified using the -output flag.

Smohamed-MSFT · 2023-10-09T18:12:59Z

Why is this change being made?
Support IT Pro by allowing the output of logging errors to file.

What changed?
Added a -output flag that allow to specify a file location to output logs to after crash.

How was the change tested?
Validated by reverting to previous commit as the current wasn't building. Successfully output the errors to a log file when specified using the -output flag.

sreading-MSFT · 2023-10-12T20:11:33Z

MsixCore/msixmgr/CommandLineInterface.cpp

            },
+            {
+                L"-output",
+                Option(true, IDS_STRING_HELP_OPTION_OUTPUT,


IDS_STRING_HELP_OPTION_OUTPUT

Since this option is unpack only I'd rename these to fit the unpack specific options elsewhere, so IDS_STRING_HELP_OPTION_UNPACK_OUTPUT

sreading-MSFT · 2023-10-12T20:13:52Z

MsixCore/msixmgr/CommandLineInterface.cpp

+                    {
+                        return E_INVALIDARG;
+                    }
+                    commandLineInterface->m_outputPath = utf8_to_utf16(outPath);


m_outputPath

Would rename this to m_unpackOutputPath as well.

Actually is "output" set in stone? Without reading the help I'm not sure it would be obvious what the difference between "-output" and "-destination" would be. Maybe m_unpackOutputLoggingPath would be more clear at least in code if the cmdline interface arg name is set.

Sure nice catch. Fixed!

sreading-MSFT · 2023-10-12T20:38:14Z

MsixCore/msixmgr/msixmgr.cpp

+                    else
+                    {
+                        OutputUnpackFailuresToFile(packageSourcePath, skippedFiles, failedPackages, failedPackagesErrors, outputFilePath);
+                        wcout << "Written error output to file: " << outputFilePath;


Written

Wrote

sreading-MSFT · 2023-10-12T20:42:14Z

MsixCore/msixmgr/msixmgr.rc

 	IDS_STRING_HELP_OPTION_UNPACK_VHDSIZE "the desired size of the VHD or VHDX file in MB. Must be between 5 and 2040000 MB. Use only for VHD or VHDX files"
 	IDS_STRING_HELP_OPTION_MOUNT_READONLY "boolean (true of false) indicating whether a VHD(X) should be mounted as read only. If not specified, the image is mounted as read-only by default"
-    
+	IDS_STRING_HELP_OPTION_OUTPUT "optional parameter selects logging location to write the command line output to"


to write the command line output to

Code as implemented does not write all command line output, only the errors. I assume this is intentional since piping all output to a file is a built in feature of powershell so probably does not need to be a special parameter here. Would specify that this is errors only (and maybe only a very specific set of errors at that? Is it supposed to be writing all errors or just the list of files that failed?)

That is correct

sreading-MSFT · 2023-10-12T21:36:12Z

MsixCore/msixmgr/msixmgr.cpp

                    std::wcout << std::endl;

-                    OutputUnpackFailures(packageSourcePath, skippedFiles, failedPackages, failedPackagesErrors);
+                    if (outputFilePath.empty())


if (outputFilePath.empty())

Not sure it makes sense to have this be an either\or case. As long as so much other logging is still going to the command line, it seems like this stuff should still go there too even if output path is set. If outputpath is set it can be duplicated to the file.

If output path is set this would end up being logged in the file

sreading-MSFT · 2023-10-12T22:29:13Z

MsixCore/msixmgr/msixmgr.cpp

+    _In_ std::vector<HRESULT> failedPackagesErrors,
+    _In_ std::wstring outfilePath)
+{
+    std::wofstream  outfile;


std::wofstream

Duplicating all this code for writing to the different types of logs should be unnecessary, but writing multiple streams at once also seems to have some complexity. Easiest solution seems like building a big wstringstream of everything that needs to get logged in this function and then having the wrapper function either write that to just std::wcout or both std::wcout and the file stream.

std::wstringstream unpackFailureString;
use unpackFailureString everywhere std::wcout is used.
add unpackFailureString << std::flush;

then do:
std::wcout << unpackFailureString.rdbuf() << std::endl;
if (!outputFilePath.empty)
{
open
outFile << unpackFailureString.rdbuf() << std::endl;
}

The duplication works because we want to have different behavior when output path is set and when it isn't this is something I've validated with PM during design.

Duplicating all the output string logic isn't necessary to have different behavior when output path is set.

void GetUnpackOutputFailures(
In std::wstring packageSource,
In std::vectorstd::wstring skippedFiles,
In std::vectorstd::wstring failedPackages,
In std::vector failedPackagesErrors,
In std::wstringstream& unpackFailureStringStream)
{
if (!skippedFiles.empty())
{
unpackFailureStringStream << std::endl;
unpackFailureStringStream << "[WARNING] The following items from " << packageSource << " were ignored because they are not packages or bundles " << std::endl;
...
etc
}

void OutputUnpackFailures(
In std::wstring packageSource,
In std::vectorstd::wstring skippedFiles,
In std::vectorstd::wstring failedPackages,
In std::vector failedPackagesErrors,
In std::wstring outputFilePath)
{
std::wstringstream unpackFailureStringStream;
GetUnpackOutputFailures(packageSource, skippedFiles, failedPackages, failedPackagesErrors, unpackFailureStringStream);
std::wstring unpackFailureString = unpackFailureStringStream.str();
std::wcout << unpackFailureString << std::endl;
if (!outputFilePath.empty())
{
std::wofstream outFile;
outFile.open(outputFilePath);
outFile << unpackFailureString << std::endl;
outFile.close();
wcout << "Wrote error output to file: " << outputFilePath;
}
}

And then just call the OutputUnpackFailures function and let it deal with the if\else for if outputFilePath is set.
std::wcout << std::endl;
std::wcout << "Finished unpacking packages to: " << unpackDestination << std::endl;
std::wcout << std::endl;

OutputUnpackFailures(packageSourcePath, skippedFiles, failedPackages, failedPackagesErrors, outputFilePath);

Completed making those changes 82a4c90

sreading-MSFT · 2023-10-12T22:34:01Z

    std::wcout << std::endl;

At a very high level I'm trying to figure out if the feature this code review is trying to implement would work better by just changing all the std::wcout uses in this particular function to std::wcerr and then just let the caller pipe the error output to a file rather than having a separate command line arg.

Refers to: MsixCore/msixmgr/msixmgr.cpp:135 in 2ce316d. [](commit_id = 2ce316d, deletion_comment = False)

sreading-MSFT · 2023-11-20T20:13:00Z

Looks like my username isn’t marked as valid for approving on this project for checkin - but thank you for all the updates. I approve.

S-mohamed1 added 2 commits October 2, 2023 20:07

Update to support logging improvement and fix build issues

2ce316d

Update to support logging improvement and fix build issues

362e664

Smohamed-MSFT requested a review from cwruss October 6, 2023 18:28

Smohamed-MSFT requested review from a team, jyvenugo, sarjanas-msft and wcheng-msft as code owners October 6, 2023 18:28

Smohamed-MSFT requested review from pratiksinha07, ravishroshanms and vishmicrosoft October 6, 2023 18:28

Smohamed-MSFT changed the title ~~User/smohamed/update to support logging improv~~ [MSIX app attach] Enable logging of output to a file for the IT Pro. Oct 6, 2023

sreading-MSFT reviewed Oct 12, 2023

View reviewed changes

S-mohamed1 added 3 commits October 19, 2023 11:48

update per PR comments

75d46d8

update resource strings

eeea0c9

Update to reflect PR comments

82a4c90

[MSIX app attach] Enable logging of output to a file for the IT Pro. #600

Are you sure you want to change the base?

[MSIX app attach] Enable logging of output to a file for the IT Pro. #600

Uh oh!

Conversation

Smohamed-MSFT commented Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Smohamed-MSFT commented Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Smohamed-MSFT Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sreading-MSFT Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sreading-MSFT commented Oct 12, 2023

Uh oh!

sreading-MSFT commented Nov 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Smohamed-MSFT commented Oct 6, 2023 •

edited

Loading

Smohamed-MSFT commented Oct 9, 2023 •

edited

Loading

Smohamed-MSFT Oct 19, 2023 •

edited

Loading

sreading-MSFT Oct 12, 2023 •

edited

Loading