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 4 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_UNPACK_OUTPUT,
[&](CommandLineInterface* commandLineInterface, const std::string& outPath)
{
if (commandLineInterface->m_operationType != OperationType::Unpack)
{
return E_INVALIDARG;
}
commandLineInterface->m_unpackOutputLoggingPath = utf8_to_utf16(outPath);
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_unpackOutputLoggingPath; }
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_unpackOutputLoggingPath;
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 << "Wrote 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 << "Wrote 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 << "Wrote error output to file: " << outputFilePath;
}

// 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_UNPACK_OUTPUT "optional parameter selects logging location to write the command line output to"
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_UNPACK_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