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.

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

Jump to bottom

[breakpad] update to 2023.01.27 #34273

Merged
merged 5 commits into from
Dec 12, 2023
Merged

[breakpad] update to 2023.01.27 #34273

merged 5 commits into from
Dec 12, 2023

Conversation

liamwhite
Copy link
Contributor

Fixes #34264

  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version [N/A]
  • Any fixed CI baseline entries are removed from that file. [N/A]
  • Any patches that are no longer applied are deleted from the port's directory. [N/A]
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@liamwhite liamwhite force-pushed the update-breakpad branch 3 times, most recently from c5aae52 to f98d6c7 Compare October 5, 2023 04:39
@liamwhite liamwhite marked this pull request as ready for review October 5, 2023 04:47
BillyONeal
BillyONeal reviewed Oct 5, 2023
View reviewed changes
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

Has the new patch been submitted upstream?

ports/breakpad/fix-const-char.patch Outdated Show resolved Hide resolved
@liamwhite
Copy link
Contributor Author

@BillyONeal the code has been completely reworked upstream to remove the usage of mkstemp and fix multiple compilation issues. However no newer tagged release with this refactor exists.

@liamwhite liamwhite marked this pull request as draft October 5, 2023 19:41
@liamwhite liamwhite force-pushed the update-breakpad branch 2 times, most recently from 98936ee to e20274e Compare October 5, 2023 19:47
@liamwhite
Copy link
Contributor Author

liamwhite commented Oct 5, 2023 •
edited
Loading

Pushed to fix another compilation error in the tools feature, zlib is now required to build it

@liamwhite liamwhite marked this pull request as ready for review October 5, 2023 19:47
BillyONeal
BillyONeal reviewed Oct 6, 2023
View reviewed changes
ports/breakpad/CMakeLists.txt Outdated
@@ -1,7 +1,9 @@
cmake_minimum_required(VERSION 3.8)
project(breakpad CXX)

set(CMAKE_CXX_STANDARD 11)
include(CMakeFindDependencyMacro)
Copy link
Member

Choose a reason for hiding this comment

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

This goes in the installed config, not in Breakpad's cmakelists.txt

(CMakeLists.txt says find_package(args args args REQUIRED), and it installs a breakpad-config.cmake or similar that says find_dependency(the same args args args but not required). This makes sure after find_package(breakpad REQUIRED) in a downstream dependency, the target ZLIB::ZLIB exists so that the target dump_syms depends on targets that actually exist. The port zyre is an example (although what it does with vcpkg-cmake-wrapper.cmake would not be had that port not been authored 4 years ago :) ))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried to add this based on other port files. Let me know if I got it right.

@FrankXie05 FrankXie05 added the category:port-update The issue is with a library, which is requesting update new revision label Oct 7, 2023
@FrankXie05
Copy link
Contributor

@liamwhite Please merge this PR to master. :)

@liamwhite liamwhite force-pushed the update-breakpad branch 2 times, most recently from 66546fe to d2ef324 Compare November 9, 2023 14:02
FrankXie05
FrankXie05 previously approved these changes Dec 7, 2023
View reviewed changes
@FrankXie05 FrankXie05 added the info:reviewed Pull Request changes follow basic guidelines label Dec 7, 2023
@BillyONeal
Copy link
Member

  1. The config is clearly wrong because (1) it doesn't include the targets, and (2) it doesn't have the right name: the package name is unofficial-breakpad but that config file would be for the one named breakpad.
  2. In my testing, trying to install breakpad[tools] still does not work, despite the ZLIB::ZLIB; I've asked for help because I can't make heads or tails of this.
    zlib build failure demonstration
  3. I tried to push a change which fixes this but I ran into the above issue: BillyONeal@7e3c4d1#diff-a980ac0e3394c4193b7e881380247a8334b690eaaaf23094ffb1c050c4e83da3R1
  4. After having tried this myself, I only now see that zlib is only used when building tools, meaning maybe it shouldn't be exposed in the config after all. This is really a function of whether the ZLIB::ZLIB target is necessary to use the exported targets and it isn't clear to me whether targets for the tool binaries are exported. But I couldn't test this because I ran into that compilation issue.

I asked for help from other maintainers.

@BillyONeal
Copy link
Member

It turns out, the file in question got picked up with a GLOB, so it shouldn't have been listed with that target at all.

BillyONeal
BillyONeal reviewed Dec 9, 2023
View reviewed changes
ports/breakpad/CMakeLists.txt
Comment on lines -144 to -151
src/common/linux/crc32.cc
src/common/linux/dump_symbols.cc
src/common/linux/elf_symbols_to_module.cc
src/common/linux/elfutils.cc
src/common/linux/file_id.cc
src/common/linux/linux_libc_support.cc
src/common/linux/memory_mapped_file.cc
src/common/linux/safe_readlink.cc
Copy link
Member

Choose a reason for hiding this comment

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

These are already in libbreakpad_client from line 64 and therefore don't also need to be added to dump_syms.

@vicroms vicroms merged commit 1464414 into microsoft:master Dec 12, 2023
15 checks passed
@liamwhite liamwhite deleted the update-breakpad branch December 12, 2023 14:17
autoantwort pushed a commit to autoantwort/vcpkg that referenced this pull request Dec 13, 2023
@liamwhite @BillyONeal @autoantwort
[breakpad] update to 2023.01.27 (microsoft#34273)
7eaebd3 
* [breakpad] update to 2023.01.27

* Fix the CMakeFindDependencyMacro part.

* Attach zlib to where the glob happens, and remove duplicate cpp files from dump_syms.

* Zlib is an unconditional dependency on Linux

---------

Co-authored-by: Billy Robert O'Neal III <bion@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BillyONeal BillyONeal BillyONeal approved these changes

@FrankXie05 FrankXie05 FrankXie05 left review comments

Assignees

@FrankXie05 FrankXie05

Labels
category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[breakpad] build failure
4 participants
@liamwhite @FrankXie05 @BillyONeal @vicroms