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

[crashrpt] Add new port #9162

Merged
merged 26 commits into from
Aug 9, 2020
Merged

[crashrpt] Add new port #9162

merged 26 commits into from
Aug 9, 2020

Conversation

tbdrake
Copy link
Contributor

@tbdrake tbdrake commented Nov 30, 2019

This builds the CrashRpt and CrashRptProbe libraries and CrashSender executable that must be distributed with apps that use this package. This is my first attempt at creating a port and I have some notes and questions, so I created this as a draft pull request.

Vendored dependencies

The CrashRpt repository includes old vendored dependencies for the following libraries. These have been replaced (except minizip) in this port with vcpkg dependencies so that these dependencies can be updated and maintained according to the maintainer guide.

  • zlib
  • minizip (not replaced since CrashRpt's copy has modifications that are necessary for CrashRpt's functionality)
  • libpng
  • libjpeg (replaced with libjpeg-turbo)
  • tinyxml
  • libogg
  • libtheora
  • wtl
  • dbghelp (replaced with new port that copies from Windows 10 SDK in build environment)

CrashSender exe and dependencies copied to "tools" directory

The portfile here copies the CrashSender executables to the "tools" and "debug/tools" directories. CrashSender also depends on a .ini file with localized strings, so the .ini files from the CrashRpt repo are also copied to the "tools" directory. Is there a more appropriate standard location besides "tools" for these files?

Distributing dbghelp.dll

Notes about redistributing dbghelp.dll
https://docs.microsoft.com/en-us/windows/win32/debug/dbghelp-versions?redirectedfrom=MSDN

The most current versions of DbgHelp.dll, SymSrv.dll, and SrcSrv.dll are available as a part of the Debugging Tools For Windows package. The redistribution policies for these included DLLs were specifically designed to make it as easy as possible for people to include these files in their own packages and releases.

https://docs.microsoft.com/en-us/windows/win32/debug/calling-the-dbghelp-library

When using DbgHelp, the best strategy is to install a copy of the library from the Debugging Tools For Windows package in the application directory logically adjacent to the software that calls it.

The CrashRpt documentation has this note:

Note:
dbghelp.dll library is required to generate minidump files. dbghelp.dll is included in Windows XP distribution and in later versions of Windows, and most user machines have this library installed. This means it is possible (but not recommended) to ignore distribution of dbghelp.dll, if you strongly wish.

Usage info

In a test app that uses the crashrpt package I added a post-build event like this to copy (and rename the .ini file) to the target directory like this:

copy "$(VcpkgCurrentInstalledDir)tools\crashrpt\crashrpt_lang_EN.ini" "$(TargetDir)crashrpt_lang.ini"
copy "$(VcpkgCurrentInstalledDir)tools\crashrpt\CrashSender1403.exe" "$(TargetDir)"

https://github.com/tbdrake/vcpkg-samples/tree/master/crashrpt/CrashRptSample

resolves #560

@msftclas
Copy link

msftclas commented Nov 30, 2019

CLA assistant check
All CLA requirements met.

ports/crashrpt/portfile.cmake Outdated Show resolved Hide resolved
ports/crashrpt/portfile.cmake Outdated Show resolved Hide resolved
ports/crashrpt/portfile.cmake Outdated Show resolved Hide resolved
@grdowns grdowns self-assigned this Dec 2, 2019
@grdowns grdowns changed the title Add port files for crashrpt [crashrpt] Add new port Dec 2, 2019
@LilyWangL
Copy link
Contributor

Pinging @tbdrake for response. Is work still being done for this PR?

@tbdrake
Copy link
Contributor Author

tbdrake commented Apr 15, 2020 via email

@tbdrake
Copy link
Contributor Author

tbdrake commented Apr 20, 2020

I'm still planning to look into replacing the vendored dependencies.

@johannesstricker
Copy link

I'm also interested in this and would be willing to help, but I couldn't find any information on how to list dependencies for a new port. Is it just a Build-Depends: ... in the CONTROL file?

@JackBoosY
Copy link
Contributor

/azp run

…nd add vcpkg port features to enable building the tests and demos
…rashrpt to replace vendored dependency of dbghelp.
@tbdrake
Copy link
Contributor Author

tbdrake commented Apr 26, 2020

@johannesstricker Thank you, any help and review would be appreciated. I worked on replacing the vendored dependencies as best I could by looking at existing ports in vcpkg. Some vcpkg documentation was also helpful:

Besides the Build-Depends in the CONTROL file I also had to update some of the CMakeLists.txt to use find_package()/find_library()/find_path() to locate and use the vcpkg libraries rather than the versions in the CrashRpt git repo. I also made a few small code changes in the patch files in this branch that were required to fix compilation issues when using the versions of the dependencies from vcpkg (newer than the ones CrashRpt was originally written with).

I added feature options tests and demos to build the test and demo applications (console demo, WTL demo, MFC demo) that are included with CrashRpt, but some tests are failing unexpectedly on my machine and I have not verified that the demos are working properly. Any help in testing and fixing issues so that the tests pass and demos work properly would be greatly appreciated. I have not had time to look in to the causes of the failures and if these are new failures because of the updated dependencies.

One issue I noticed for example is that the "Export..." button in Error Report Details is not successfully saving the error report zip file (it seems to silently fail when I click "Save"):
image

@JackBoosY
Copy link
Contributor

JackBoosY commented Apr 27, 2020

Need my help?

@tbdrake
Copy link
Contributor Author

tbdrake commented May 9, 2020

@JackBoosY
Yes I could use help. One thing I am not sure how to handle correctly is the CI pipeline, particularly for the dbghelp port I added here which crashrpt depends on.

The build for dbghelp:x86-windows here failed and stdout-x86-windows-log shows this message that dbghelp.lib could not be found where expected:

CMake Error at ports/dbghelp/portfile.cmake:16 (message):
  Cannot find Windows 10.0.18362.0 SDK.  File does not exist: C:\Program
  Files (x86)\Windows Kits\10\Debuggers\lib\x86\dbghelp.lib
Call Stack (most recent call first):
  scripts/ports.cmake:90 (include)

I think this is because the dbghelp library is included in the Windows SDK installation only if Debugging Tools for Windows feature is included, so maybe the CI system does not have this installed:
image

How should I fix this?

@JackBoosY
Copy link
Contributor

@tbdrake Can you provide the way to reproduce this?

Thanks.

@tbdrake
Copy link
Contributor Author

tbdrake commented Jun 17, 2020

@JackBoosY Here are the steps I'm doing to build and run the CrashRpt automated tests in Release. Results from my earlier comment were from Debug builds and the steps are similar and I get different results using Debug as well. Also note that there are 3 DeliveryTests that are expected to fail unless you have the additional setup required (there's a note here).

Using vcpkg and this port:

  1. Clone this pull request's branch: git clone https://github.com/tbdrake/vcpkg.git -b crashrpt
  2. In vcpkg directory cloned above, build vcpkg: bootstrap-vcpkg.bat
  3. Install crashrpt including the tests feature: vcpkg.exe install crashrpt[tests]
  4. Run the crashrpt automated tests: installed/x86-windows/tools/crashrpt/Tests.exe
  5. Enter n for first prompt about testing UNICODE compatibility, then enter a blank line in the second prompt to run all tests. Here is the test summary I get:
=== Summary ===

Error list:
1: In test: CrashRptAPITests::Test_crInstallW_zero_info Expr: nInstallResult==0
2: In test: CrashRptAPITests::Test_crInstallA_zero_info Expr: nInstallResult==0
3: In test: CrashRptAPITests::Test_crInstallA_twice Expr: nInstallResult==0
4: In test: CrashRptAPITests::Test_crInstallA_short_path_name Expr: nInstallResult==0
5: In test: CrashRptAPITests::Test_crInstallW_short_path_name Expr: nInstallResult==0
6: In test: CrashRptAPITests::Test_crInstallToCurrentThread2 Expr: nInstResult==0
7: In test: CrashRptAPITests::Test_crInstallToCurrentThread2_concurrent Expr: nInstResult==0
8: In test: CrashRptAPITests::Test_crAddFile2A Expr: nInstallResult==0
9: In test: CrashRptAPITests::Test_crAddFile2W Expr: nInstallResult==0
10: In test: CrashRptAPITests::Test_crAddScreenshot Expr: nInstallResult==0
11: In test: CrashRptAPITests::Test_crAddScreenshot2 Expr: nInstallResult==0
12: In test: CrashRptAPITests::Test_crAddPropertyA Expr: nInstallResult==0
13: In test: CrashRptAPITests::Test_crAddPropertyW Expr: nInstallResult==0
14: In test: CrashRptAPITests::Test_crAddRegKeyA Expr: nInstallResult==0
15: In test: CrashRptAPITests::Test_crAddRegKeyW Expr: nInstallResult==0
16: In test: CrashRptAPITests::Test_crAddVideo Expr: nInstallResult==0
17: In test: CrashRptAPITests::Test_crAddVideo_defaults Expr: nInstallResult==0
18: In test: CrashRptAPITests::Test_crAddVideo_crash Expr: nInstallResult==0
19: In test: CrashRptAPITests::Test_crSetCrashCallbackA Expr: nInstall==0
20: In test: CrashRptAPITests::Test_crSetCrashCallbackW Expr: nInstall==0
21: In test: CrashRptAPITests::Test_crSetCrashCallbackW_stage Expr: nInstall==0
22: In test: CrashRptAPITests::Test_crSetCrashCallbackW_cancel Expr: nInstall==0
23: In test: CrashRptAPITests::Test_crGenerateErrorReport Expr: nInstResult==0
24: In test: CrashRptAPITests::Test_crGetLastErrorMsgA Expr: nInstallResult==0
25: In test: CrashRptAPITests::Test_crGetLastErrorMsgW Expr: nInstallResult==0
26: In test: CrashRptAPITests::Test_CrAutoInstallHelper Expr: cr_install_helper.m_nInstallStatus==0
27: In test: CrashRptAPITests::Test_CrThreadAutoInstallHelper Expr: cr_install_helper.m_nInstallStatus==0
28: In test: CrashRptAPITests::Test_crInstall_in_different_folder Expr: bCopy3
29: In test: CrashRptAPITests::Test_symbol_file_exists Expr: dwAttrs!=INVALID_FILE_ATTRIBUTES Msg: File does not exist: c:\Users\tdrake\Desktop\vcpkg\installed\x86-windows\tools\crashrpt\CrashRpt1403.pdb
30: In test: CrashRptProbeAPITests::SetUp Expr: bCreateReport
31: In test: Test_crpOpenErrorReportA Expr: SetUp Failure
32: In test: Test_crpOpenErrorReportW Expr: SetUp Failure
33: In test: Test_crpCloseErrorReport Expr: SetUp Failure
34: In test: Test_crpExtractFileW Expr: SetUp Failure
35: In test: Test_crpExtractFileA Expr: SetUp Failure
36: In test: Test_crpGetLastErrorW Expr: SetUp Failure
37: In test: Test_crpGetLastErrorA Expr: SetUp Failure
38: In test: Test_crpGetPropertyW Expr: SetUp Failure
39: In test: Test_crpGetPropertyA Expr: SetUp Failure
40: In test: Test_crpGetProperty Expr: SetUp Failure
41: In test: Test_crashrptprobe_dll_file_version Expr: SetUp Failure
42: In test: TearDown Expr: SetUp Failure
43: In test: CrproberTests::SetUp Expr: bCreateReport
44: In test: Test_help Expr: SetUp Failure
45: In test: Test_output Expr: SetUp Failure
46: In test: Test_extract_file Expr: SetUp Failure
47: In test: Test_get Expr: SetUp Failure
48: In test: TearDown Expr: SetUp Failure
49: In test: DeliveryTests::Test_HttpDelivery Expr: nInstResult==0
50: In test: DeliveryTests::Test_SmtpDelivery Expr: nInstResult==0
51: In test: DeliveryTests::Test_SmtpDelivery_proxy Expr: nInstResult==0
52: In test: DeliveryTests::Test_SMAPI_Delivery Expr: nInstResult==0
53: In test: ExceptionHandlerTests::Test_CatchException Expr: bFind Msg: Could not find generated crash report file for exception type: 11 SIGSEGV signal
54: In test: LangFileTests::Test_lang_file_versions Expr: nActualVer==CRASHRPT_VER

 Time elapsed: 19.14 sec.
   Test count: 72
 Tests passed: 18
 Tests failed: 54

Using original CrashRpt git repo, CMake GUI, and Visual Studio:

  1. Clone crashrpt: git clone https://git.code.sf.net/p/crashrpt/code crashrpt
  2. Open CMake GUI and set the source code and binaries directory to above location of crashrpt repo:
    image
  3. Click Configure and select Win32 for the Optional platform generator, then Finish:
    image
  4. Click Generate
  5. Click Open Project
  6. In Visual Studio, select Release configuration and then Build -> Rebuild Solution
  7. Set the Tests project as the startup project, then click the Run button:
    image
  8. In Tests.exe window, enter n for first prompt about testing UNICODE compatibility, then enter a blank line in the second prompt to run all tests. Here is the test summary I get:
=== Summary ===

Error list:
1: In test: CrashRptAPITests::Test_symbol_file_exists Expr: dwAttrs!=INVALID_FILE_ATTRIBUTES Msg: File does not exist: C:\Users\tdrake\Desktop\crashrpt\bin\CrashRpt1403.pdb
2: In test: DeliveryTests::Test_HttpDelivery Expr: dwExitCode==0
3: In test: DeliveryTests::Test_SmtpDelivery Expr: dwExitCode==0
4: In test: DeliveryTests::Test_SmtpDelivery_proxy Expr: dwExitCode==0
5: In test: ExceptionHandlerTests::Test_CatchException Expr: _tcscmp(szBuffer, _T("crEmulateCrash"))==0

 Time elapsed: 44.59 sec.
   Test count: 72
 Tests passed: 67
 Tests failed: 5

@tbdrake
Copy link
Contributor Author

tbdrake commented Jun 18, 2020

@JackBoosY I realized today some of those test failures in my previous comment under Using vcpkg and this port: are just because there needs to be a crashrpt_lang.ini file in the same directory as CrashSender1403.exe, so just need to copy/rename one of the lang files before running the Tests.exe built by vcpkg:

  • cp ./installed/x86-windows/tools/crashrpt/crashrpt_lang_EN.ini ./installed/x86-windows/tools/crashrpt/crashrpt_lang.ini

However I still get these test failures:

=== Summary ===

Error list:
1: In test: CrashRptAPITests::Test_crAddFile2A Expr: nResult3==0
2: In test: CrashRptAPITests::Test_crAddFile2W Expr: nResult3==0
3: In test: CrashRptAPITests::Test_symbol_file_exists Expr: dwAttrs!=INVALID_FILE_ATTRIBUTES Msg: File does not exist: C:\Users\tdrake\Desktop\vcpkg\installed\x86-windows\tools\crashrpt\CrashRpt1403.pdb
4: In test: CrashRptProbeAPITests::SetUp Expr: bCreateReport
5: In test: Test_crpOpenErrorReportA Expr: SetUp Failure
6: In test: Test_crpOpenErrorReportW Expr: SetUp Failure
7: In test: Test_crpCloseErrorReport Expr: SetUp Failure
8: In test: Test_crpExtractFileW Expr: SetUp Failure
9: In test: Test_crpExtractFileA Expr: SetUp Failure
10: In test: Test_crpGetLastErrorW Expr: SetUp Failure
11: In test: Test_crpGetLastErrorA Expr: SetUp Failure
12: In test: Test_crpGetPropertyW Expr: SetUp Failure
13: In test: Test_crpGetPropertyA Expr: SetUp Failure
14: In test: Test_crpGetProperty Expr: SetUp Failure
15: In test: Test_crashrptprobe_dll_file_version Expr: SetUp Failure
16: In test: TearDown Expr: SetUp Failure
17: In test: CrproberTests::SetUp Expr: bCreateReport
18: In test: Test_help Expr: SetUp Failure
19: In test: Test_output Expr: SetUp Failure
20: In test: Test_extract_file Expr: SetUp Failure
21: In test: Test_get Expr: SetUp Failure
22: In test: TearDown Expr: SetUp Failure
23: In test: DeliveryTests::Test_SMAPI_Delivery Expr: dwExitCode!=0
24: In test: ExceptionHandlerTests::Test_CatchException Expr: bFind Msg: Could not find generated crash report file for exception type: 11 SIGSEGV signal
25: In test: LangFileTests::Test_lang_file_versions Expr: nActualVer==CRASHRPT_VER

 Time elapsed: 32.06 sec.
   Test count: 72
 Tests passed: 47
 Tests failed: 25

@JackBoosY
Copy link
Contributor

JackBoosY commented Jun 19, 2020

There are indeed many more errors, but I don’t know why.
Edit: May be related to some options?

* Use vendored minizip since it has modifications to support Unicode file paths using wchar_t* which CrashRpt depends on
* Add feature "probe" in order to allow excluding the CrashRptProbe library
@tbdrake
Copy link
Contributor Author

tbdrake commented Jul 26, 2020

I found that the issue causing some of the CrashRpt unit tests to fail was that the copy of minizip in CrashRpt has modifications that are needed for CrashRpt to work properly, so when I had changed it to use minizip from vcpkg those things no longer worked. So in the latest commit today I changed this port back to using CrashRpt's vendored copy of minizip.

One example of a difference in minizip that caused an issue:

https://sourceforge.net/p/crashrpt/code/ci/master/tree/thirdparty/minizip/ioapi.c#l116:

file = _wfopen((wchar_t*)filename, mode_fopen);

vs https://github.com/madler/zlib/blob/master/contrib/minizip/ioapi.c#L127 (FOPEN_FUNC is fopen64 on Windows)

file = FOPEN_FUNC((const char*)filename, mode_fopen);

# Conflicts:
#	scripts/ci.baseline.txt
@tbdrake tbdrake marked this pull request as ready for review July 26, 2020 16:01
@tbdrake tbdrake requested a review from JackBoosY July 26, 2020 16:03
@JackBoosY
Copy link
Contributor

Our pipeline machine has some problems, please wait for fix it.

@JackBoosY
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JackBoosY JackBoosY added requires:discussion requires:testing Needs tests added before merging labels Aug 3, 2020
@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:testing Needs tests added before merging labels Aug 4, 2020

vcpkg_get_windows_sdk(WINDOWS_SDK)

if (WINDOWS_SDK MATCHES "10.")
Copy link
Contributor

@strega-nil strega-nil Aug 5, 2020

Choose a reason for hiding this comment

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

Suggested change
if (WINDOWS_SDK MATCHES "10.")
if (WINDOWS_SDK MATCHES "^10\.")

or, alternatively,

Suggested change
if (WINDOWS_SDK MATCHES "10.")
if (WINDOWS_SDK VERSION_GREATER "10")

This'll match "9.10.1", for example, or "101"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion, updated to use VERSION_GREATER

@JackBoosY JackBoosY added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Aug 6, 2020
… in dbghelp portfile.cmake. Remove redundant check of WINDOWS_SDK version later in script.
@strega-nil strega-nil added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Aug 9, 2020
@strega-nil strega-nil merged commit 4b1950f into microsoft:master Aug 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port request: CrashRpt
9 participants