Skip to content

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

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2ce316d
Update to support logging improvement and fix build issues
S-mohamed1 Oct 3, 2023
362e664
Update to support logging improvement and fix build issues
S-mohamed1 Oct 3, 2023
75d46d8
update per PR comments
S-mohamed1 Oct 19, 2023
eeea0c9
update resource strings
S-mohamed1 Oct 24, 2023
82a4c90
Update to reflect PR comments
S-mohamed1 Nov 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions MsixCore/msixmgr/CommandLineInterface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,19 @@ std::map<std::wstring, Options, CaseInsensitiveLess> CommandLineInterface::s_opt
return S_OK;
}),
},
{
L"-output",
Option(true, IDS_STRING_HELP_OPTION_OUTPUT,
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

@Smohamed-MSFT Smohamed-MSFT Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Fixed

[&](CommandLineInterface* commandLineInterface, const std::string& outPath)
{
if (commandLineInterface->m_operationType != OperationType::Unpack)
{
return E_INVALIDARG;
}
commandLineInterface->m_outputPath = utf8_to_utf16(outPath);
Copy link

Choose a reason for hiding this comment

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

m_outputPath

Would rename this to m_unpackOutputPath as well.

Copy link

@sreading-MSFT sreading-MSFT Oct 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Sure nice catch. Fixed!

return S_OK;
}),
},
{
L"-validateSignature",
Option(false, IDS_STRING_HELP_OPTION_UNPACK_VALIDATESIGNATURE,
Expand Down
2 changes: 2 additions & 0 deletions MsixCore/msixmgr/CommandLineInterface.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ class CommandLineInterface
OperationType GetOperationType() { return m_operationType; }
std::wstring GetOperationTypeAsString();
ULONGLONG GetVHDSize() { return m_vhdSize; }
std::wstring GetOutputPath() { return m_outputPath; }
private:
int m_argc = 0;
char ** m_argv = nullptr;
Expand All @@ -118,6 +119,7 @@ class CommandLineInterface
std::wstring m_packageFilePath;
std::wstring m_packageFullName;
std::wstring m_unpackDestination;
std::wstring m_outputPath;
std::wstring m_rootDirectory;
std::wstring m_mountImagePath;
std::wstring m_volumeId;
Expand Down
88 changes: 83 additions & 5 deletions MsixCore/msixmgr/msixmgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "Util.hpp"
#include "..\msixmgrLib\GeneralUtil.hpp"
#include "resource.h"
#include <fstream>
#include <VersionHelpers.h>
#include "UnpackProvider.hpp"
#include "ApplyACLsProvider.hpp"
Expand Down Expand Up @@ -173,6 +174,60 @@ void OutputUnpackFailures(
}
}

void OutputUnpackFailuresToFile(
_In_ std::wstring packageSource,
_In_ std::vector<std::wstring> skippedFiles,
_In_ std::vector<std::wstring> failedPackages,
_In_ std::vector<HRESULT> failedPackagesErrors,
_In_ std::wstring outfilePath)
{
std::wofstream outfile;
Copy link

Choose a reason for hiding this comment

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

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;
}

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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);

Copy link
Author

Choose a reason for hiding this comment

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

Completed making those changes 82a4c90

outfile.open(outfilePath);

if (!skippedFiles.empty())
{
outfile << std::endl;
outfile << "[WARNING] The following items from " << packageSource << " were ignored because they are not packages or bundles " << std::endl;
outfile << std::endl;

for (int i = 0; i < skippedFiles.size(); i++)
{
outfile << skippedFiles.at(i) << std::endl;
}

outfile << std::endl;
}

if (!failedPackages.empty())
{
outfile << std::endl;
outfile << "[WARNING] The following packages from " << packageSource << " failed to get unpacked. Please try again: " << std::endl;
outfile << std::endl;

for (int i = 0; i < failedPackages.size(); i++)
{
HRESULT hr = failedPackagesErrors.at(i);

outfile << L"Failed with HRESULT 0x" << std::hex << hr << L" when trying to unpack " << failedPackages.at(i) << std::endl;
if (hr == static_cast<HRESULT>(MSIX::Error::CertNotTrusted))
{
outfile << L"Please confirm that the certificate has been installed for this package" << std::endl;
}
else if (hr == static_cast<HRESULT>(MSIX::Error::FileWrite))
{
outfile << L"The tool encountered a file write error. If you are unpacking to a VHD, please try again with a larger VHD, as file write errors may be caused by insufficient disk space." << std::endl;
}
else if (hr == E_INVALIDARG)
{
outfile << "Please confirm the given package path is an .appx, .appxbundle, .msix, or .msixbundle file" << std::endl;
}

outfile << std::endl;
}
}
outfile.close();
}

int main(int argc, char * argv[])
{
// Register the providers
Expand Down Expand Up @@ -478,7 +533,7 @@ int main(int argc, char * argv[])
// Telemetry : Unpack Workflow Log
msixmgrTraceLogging::TraceLogUnpackWorkflow(workflowId.c_str(), msixmgrTraceLogging::ExtractPackageNameFromFilePath(cli.GetPackageFilePathToInstall()).c_str(),
cli.GetFileTypeAsString().c_str(), cli.GetVHDSize(), cli.IsCreate(), cli.IsApplyACLs());

auto outputFilePath = cli.GetOutputPath();
auto packageSourcePath = cli.GetPackageFilePathToInstall();
auto unpackDestination = cli.GetUnpackDestination();
auto rootDirectory = cli.GetRootDirectory();
Expand Down Expand Up @@ -645,8 +700,15 @@ int main(int argc, char * argv[])
std::wcout << "Successfully created the CIM file: " << unpackDestination << std::endl;
std::wcout << std::endl;

OutputUnpackFailures(packageSourcePath, skippedFiles, failedPackages, failedPackagesErrors);

if (outputFilePath.empty())
{
OutputUnpackFailures(packageSourcePath, skippedFiles, failedPackages, failedPackagesErrors);
}
else
{
OutputUnpackFailuresToFile(packageSourcePath, skippedFiles, failedPackages, failedPackagesErrors, outputFilePath);
wcout << "Written error output to file: " << outputFilePath;
}
// Telemetry : Workflow Log
QueryPerformanceCounter(&msixMgrLoad_EndCounter);
workflowElapsedTime = msixmgrTraceLogging::CalcWorkflowElapsedTime(msixMgrLoad_StartCounter, msixMgrLoad_EndCounter, msixMgrLoad_Frequency);
Expand Down Expand Up @@ -761,7 +823,15 @@ int main(int argc, char * argv[])
std::wcout << std::endl;
}

OutputUnpackFailures(packageSourcePath, skippedFiles, failedPackages, failedPackagesErrors);
if (outputFilePath.empty())
{
OutputUnpackFailures(packageSourcePath, skippedFiles, failedPackages, failedPackagesErrors);
}
else
{
OutputUnpackFailuresToFile(packageSourcePath, skippedFiles, failedPackages, failedPackagesErrors, outputFilePath);
wcout << "Written error output to file: " << outputFilePath;
}

std::wcout << std::endl;
std::wcout << "Finished unpacking packages to: " << unpackDestination << std::endl;
Expand Down Expand Up @@ -801,7 +871,15 @@ int main(int argc, char * argv[])
std::wcout << "Finished unpacking packages to: " << unpackDestination << std::endl;
std::wcout << std::endl;

OutputUnpackFailures(packageSourcePath, skippedFiles, failedPackages, failedPackagesErrors);
if (outputFilePath.empty())
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Written

Wrote

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

}

// Telemetry : Workflow Log
QueryPerformanceCounter(&msixMgrLoad_EndCounter);
Expand Down
2 changes: 1 addition & 1 deletion MsixCore/msixmgr/msixmgr.rc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ BEGIN
IDS_STRING_HELP_OPTION_MOUNT_FILETYPE "the type of file to mount or unmount. The following file types are currently supported: {VHD, VHDX, CIM}"
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"
Copy link

Choose a reason for hiding this comment

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

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?)

Copy link
Author

Choose a reason for hiding this comment

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

That is correct

END

#endif // English (United States) resources
Expand Down
2 changes: 1 addition & 1 deletion MsixCore/msixmgr/resource.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@
#define IDS_STRING_HELP_OPTION_MOUNT_FILETYPE 161
#define IDS_STRING_HELP_OPTION_UNPACK_VHDSIZE 162
#define IDS_STRING_HELP_OPTION_MOUNT_READONLY 163

#define IDS_STRING_HELP_OPTION_OUTPUT 164
// Next default values for new objects
//
#ifdef APSTUDIO_INVOKED
Expand Down
1 change: 1 addition & 0 deletions MsixCore/msixmgrLib/resourceConfig.rcconfig.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
<resources>
<win32Resources fileType="Application">
<neutralResources>
<resourceType typeNameId="#6"/>
</neutralResources>
<localizedResources>
<resourceType typeNameId="#6"/>
Expand Down