[perfetto] initial port of 43.1 #37959

rtzoeller · 2024-04-03T20:58:25Z

rtzoeller · 2024-04-03T21:02:45Z

@microsoft-github-policy-service agree company="ALIARO"

rtzoeller · 2024-04-04T04:48:07Z

I've only directly tested the output on Linux x64, but I have no reason to believe it will not work in other environments.

dg0yt · 2024-04-05T07:10:30Z

How much overlap has this port with an official (gn) build?
Isn't it possible to use a shared lib on non-windows?

rtzoeller · 2024-04-05T13:42:57Z

How much overlap has this port with an official (gn) build?

@dg0yt one of the outputs of the official build is the pair of amalgamated source files we're consuming, which are attached to each tagged release (e.g. for 43.0).

This greatly simplifies the build process, and matches the example built with CMake.

Isn't it possible to use a shared lib on non-windows?

Maybe, but I've not verified Perfetto's functionality when built into a shared library and presumed we could enable it later if needed without breaking compatibility.

If this is a blocker for the initial port let me know and I'll happily investigate.

rtzoeller · 2024-04-05T14:58:37Z

Isn't it possible to use a shared lib on non-windows?

Looks like it is. I'll get a fix pushed shortly.

ports/perfetto/CMakeLists.txt

ports/perfetto/usage

ports/perfetto/CMakeLists.txt

ports/perfetto/portfile.cmake

rtzoeller · 2024-04-08T15:37:57Z

@WangWeiLin-MV I've squashed the proposed changes into the original commit and added you as a co-author for it. This should be ready for another review.

WangWeiLin-MV

Port usage tests pass with following triplets:

x64-linux
x64-windows
x64-windows-static
x64-windows-static-md
x86-windows

ports/perfetto/vcpkg.json

JavierMatosD · 2024-04-09T18:34:37Z

Hi @rtzoeller,

We generally aim to use upstream's build configurations directly and only resort to providing unofficial exports when integrating with upstream would involve extensive patching or customization that doesn't seem justifiable. Could you share your rationale for opting for an unofficial CMake export instead of adapting Perfetto's existing Meson build for vcpkg? I'm not deeply familiar with Meson and its intricacies, so your insights would be very valuable to me and perhaps highlight aspects I might not be aware of.

Thanks for your contribution and looking forward to your thoughts!

rtzoeller · 2024-04-09T19:36:13Z

Thanks for the insight @JavierMatosD.

Could you share your rationale for opting for an unofficial CMake export instead of adapting Perfetto's existing Meson build for vcpkg?

I wasn't previously aware of any existing Meson support in Perfetto (the source comment referring to Meson came from @WangWeiLin-MV); the primary build instructions only mention GN as a build system, although the repo also provides an example of using CMake to consume the amalgamated source files. In my usage of Perfetto I've always consumed the amalgamated SDK source files (e.g. for v43.0), and given the example CMake build this was the most approachable option. I'm also trying to integrate vcpkg into a CMake project which would consume the perfetto dependency, so it was a natural fit.

I now see that there is a meson.build file, and can take a look at consuming it.

I'm not deeply familiar with Meson and its intricacies, so your insights would be very valuable to me and perhaps highlight aspects I might not be aware of.

I have zero familiarity with Meson, so please bear with me as I attempt to rework this PR. 😄

rtzoeller · 2024-04-09T20:03:47Z

At first glance the existing Meson support appears to be rather incomplete.

AFAICT, it only supports building a static library and does not install any headers (but does include the .proto file). It's likely possible for us to patch meson.build as part of the port, but I'm unsure that's any better than the current CMake situation -- the minimal commit history for meson.build and lack of documentation/examples using Meson to consume Perfetto admittedly don't inspire a ton of confidence in me.

@JavierMatosD can you please advise on which direction is acceptable for moving forward? Thanks!

Continue maintaining an unofficial CMake build (i.e. merge the existing PR).
Patch the existing Meson build system to also install perfetto.h to the include directory.
- Optionally patch the Meson build system to support building as a dynamic library.
- Full disclosure: I have effectively no experience with Meson other than reviewing the existing build just now.
Move away from the amalgamated SDK builds provided by the official Perfetto releases and build the source directly with GN instead.
- Full disclosure: I have no experience with GN.

rtzoeller · 2024-04-10T13:55:26Z

@JavierMatosD @WangWeiLin-MV I looked at what it would take to port this to use the Meson build; the results are here.

It still needs to be cleaned up (and building as a shared library is still an unknown), but my primary takeaway was that using the Meson build costs us the automatic CMake integration for clients -- I still needed to write an unofficial-perfetto-config.cmake file for clients to be able to use find_package() and link successfully.

JavierMatosD · 2024-04-10T17:00:58Z

Big thanks for diving into the integration options! I'm initially leaning towards the unofficial CMake route because it seems super practical for now. But, I want to run it by our team to double-check we’re all on the same page for the long haul.

I’ll circle back with you as soon as we've made a decision. Really appreciate your patience and all the effort you're putting into this!

dg0yt · 2024-04-10T18:26:30Z

This port has no dependencies. So providing an unofficial CMake config shouldn't be more difficult to write by hand than a whole vendored CMake build system. If meson doesn't already provided official pkg-config files.

JavierMatosD · 2024-04-12T14:11:20Z

@data-queue, @ras0219, @vicroms, @BillyONeal, and I discussed this and based on the CMake usage in Perfetto’s upstream example here, we agree that adopting CMake is appropriate. This approach is supported by the example provided by the library developers and aligns with our existing tools, avoiding the complexities and maintenance challenges associated with using GN.

ports/perfetto/CMakeLists.txt

JavierMatosD · 2024-04-12T14:15:43Z

ports/perfetto/CMakeLists.txt

+)
+
+set(THREADS_PREFER_PTHREAD_FLAG TRUE)
+find_package(Threads REQUIRED)


Should Threads be listed as a dependency?

I'm unsure what you mean here aside from what's asked in the other comment.

Should PThreads be a dependency (or feature) in the vcpkg.json?

The lib pthread is a system library, it just requires the find_package, and it will only be used on NON-WINDOWS. The checks in upstream https://github.com/google/perfetto/blob/v43.1/include/perfetto/ext/base/thread_checker.h#L22-L24.

ports/perfetto/CMakeLists.txt

JavierMatosD · 2024-04-12T14:31:38Z

ports/perfetto/CMakeLists.txt

+endif(WIN32)
+
+if(MSVC)
+    target_compile_options(perfetto PRIVATE "/permissive-")


Why is this needed?

To be clear that turns off permissive mode (note the minus), and puts MSVC into standards-compliant mode.

I wasn't able to get it to fail without this flag as part of the automated build, but my recollection from previous usage is that it's needed for older MSVC versions (2017?).

It's in the recommended set of flags for using the tracing compiler:

# Enable standards-compliant mode when using the Visual Studio compiler. if (MSVC) target_compile_options(example PRIVATE "/permissive-") endif (MSVC)

I can remove it if you want!

~~If it's not strictly required we should remove this.~~ Thanks for the explanation!

Never mind. I think we should just follow upstreams recommendation here. :)

JavierMatosD · 2024-04-12T14:34:40Z

Setting the PR to draft. Please mark ready for review when you've responded. That way I will know you've responded

Co-authored-by: WangWeiLin-MV <156736127+WangWeiLin-MV@users.noreply.github.com>

JavierMatosD · 2024-04-15T17:13:44Z

ports/perfetto/CMakeLists.txt

+endif(WIN32)
+
+if(MSVC)
+    target_compile_options(perfetto PRIVATE "/permissive-")


~~If it's not strictly required we should remove this.~~ Thanks for the explanation!

Never mind. I think we should just follow upstreams recommendation here. :)

JavierMatosD · 2024-04-15T17:18:26Z

ports/perfetto/vcpkg.json

+  "license": "Apache-2.0",
+  "supports": "!uwp & !x86",
+  "dependencies": [
+    "pthreads",


Suggested change

"pthreads",

{

"name"" "pthreads",

"platform": "!windows"

},

@JavierMatosD Are you serious about this? Almost no port does this, but almost every non-trivial port uses pthreads.

Serious about what exactly? Adding the platform expression or adding it as a dependency?

Do we really request a pthreads dependency on platforms where the port is empty and where most ports don't use the dependency and where depending ports don't need to be rebuilt when the windows-only implementation is updated?

### Change - Upgrade `perfetto` version to [v44.0](https://github.com/google/perfetto/releases/tag/v44.0) - Remove dependency `pthread` because the lib is a system library which just requires the `find_package` and will only be used on NON-WINDOWS. The checks in upstream [perfetto/ext/base/thread_checker.h#L22-L24](https://github.com/google/perfetto/blob/v44.0/include/perfetto/ext/base/thread_checker.h#L22-L24) - Remove the `x86` exclusion from #37959 (comment) ### Checklist - [x] Changes comply with the [maintainer guide](https://github.com/microsoft/vcpkg-docs/blob/main/vcpkg/contributing/maintainer-guide.md). - [x] SHA512s are updated for each updated download. - [x] The "supports" clause reflects platforms that may be fixed by this new version. - [ ] ~Any fixed [CI baseline](https://github.com/microsoft/vcpkg/blob/master/scripts/ci.baseline.txt) entries are removed from that file.~ - [ ] ~Any patches that are no longer applied are deleted from the port's directory.~ - [x] The version database is fixed by rerunning `./vcpkg x-add-version --all` and committing the result. - [x] Only one version is added to each modified port's versions file. ### Test Port usage tests pass with following triplets: * x64-linux * x64-windows * x64-windows-static * x64-windows-static-md * x86-windows

rtzoeller force-pushed the perfetto branch from 6145e8a to 20e347a Compare April 4, 2024 04:07

rtzoeller marked this pull request as ready for review April 4, 2024 04:47

rtzoeller marked this pull request as draft April 5, 2024 14:58

rtzoeller force-pushed the perfetto branch from 20e347a to 57ab81a Compare April 5, 2024 15:16

rtzoeller marked this pull request as ready for review April 5, 2024 15:31

dg0yt reviewed Apr 5, 2024

View reviewed changes

ports/perfetto/CMakeLists.txt Outdated Show resolved Hide resolved

dg0yt reviewed Apr 5, 2024

View reviewed changes

ports/perfetto/CMakeLists.txt Outdated Show resolved Hide resolved

ports/perfetto/usage Outdated Show resolved Hide resolved

rtzoeller force-pushed the perfetto branch from 57ab81a to a297ede Compare April 5, 2024 19:20

WangWeiLin-MV self-assigned this Apr 7, 2024

WangWeiLin-MV added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Apr 7, 2024

WangWeiLin-MV requested changes Apr 7, 2024

View reviewed changes

ports/perfetto/CMakeLists.txt Outdated Show resolved Hide resolved

ports/perfetto/portfile.cmake Outdated Show resolved Hide resolved

ports/perfetto/portfile.cmake Outdated Show resolved Hide resolved

rtzoeller force-pushed the perfetto branch 2 times, most recently from d8596fc to b2332d8 Compare April 8, 2024 13:21

WangWeiLin-MV previously approved these changes Apr 9, 2024

View reviewed changes

ports/perfetto/vcpkg.json Show resolved Hide resolved

WangWeiLin-MV added the info:reviewed Pull Request changes follow basic guidelines label Apr 9, 2024

rtzoeller dismissed WangWeiLin-MV’s stale review via 7486698 April 9, 2024 03:57

rtzoeller force-pushed the perfetto branch 2 times, most recently from 7486698 to 2d80dbd Compare April 9, 2024 04:03

WangWeiLin-MV previously approved these changes Apr 9, 2024

View reviewed changes

JavierMatosD added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Apr 9, 2024

JavierMatosD reviewed Apr 12, 2024

View reviewed changes

JavierMatosD marked this pull request as draft April 12, 2024 14:34

[perfetto] initial port of 43.1

f03b85e

Co-authored-by: WangWeiLin-MV <156736127+WangWeiLin-MV@users.noreply.github.com>

rtzoeller dismissed WangWeiLin-MV’s stale review via f03b85e April 12, 2024 16:20

rtzoeller force-pushed the perfetto branch from 2d80dbd to f03b85e Compare April 12, 2024 16:20

rtzoeller marked this pull request as ready for review April 12, 2024 16:27

JavierMatosD reviewed Apr 15, 2024

View reviewed changes

JavierMatosD removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Apr 15, 2024

JavierMatosD merged commit b4a3d89 into microsoft:master Apr 15, 2024
16 checks passed

WangWeiLin-MV mentioned this pull request Apr 16, 2024

[perfetto] Update to 44.0 and clean deps and allow x86 #38205

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[perfetto] initial port of 43.1 #37959

[perfetto] initial port of 43.1 #37959

rtzoeller commented Apr 3, 2024 •

edited

rtzoeller commented Apr 3, 2024

rtzoeller commented Apr 4, 2024

dg0yt commented Apr 5, 2024

rtzoeller commented Apr 5, 2024 •

edited

rtzoeller commented Apr 5, 2024

rtzoeller commented Apr 8, 2024

WangWeiLin-MV left a comment

JavierMatosD commented Apr 9, 2024

rtzoeller commented Apr 9, 2024 •

edited

rtzoeller commented Apr 9, 2024 •

edited

rtzoeller commented Apr 10, 2024 •

edited

JavierMatosD commented Apr 10, 2024

dg0yt commented Apr 10, 2024 •

edited

JavierMatosD commented Apr 12, 2024

JavierMatosD Apr 12, 2024

rtzoeller Apr 12, 2024

WangWeiLin-MV Apr 15, 2024

JavierMatosD Apr 12, 2024

rtzoeller Apr 12, 2024 •

edited

JavierMatosD Apr 15, 2024 •

edited

JavierMatosD commented Apr 12, 2024

JavierMatosD Apr 15, 2024 •

edited

JavierMatosD Apr 15, 2024

dg0yt Apr 15, 2024

JavierMatosD Apr 15, 2024

dg0yt Apr 15, 2024

-    "pthreads",
+    {
+         "name"" "pthreads",
+         "platform": "!windows"
+    },

[perfetto] initial port of 43.1 #37959

[perfetto] initial port of 43.1 #37959

Conversation

rtzoeller commented Apr 3, 2024 • edited

rtzoeller commented Apr 3, 2024

rtzoeller commented Apr 4, 2024

dg0yt commented Apr 5, 2024

rtzoeller commented Apr 5, 2024 • edited

rtzoeller commented Apr 5, 2024

rtzoeller commented Apr 8, 2024

WangWeiLin-MV left a comment

Choose a reason for hiding this comment

JavierMatosD commented Apr 9, 2024

rtzoeller commented Apr 9, 2024 • edited

rtzoeller commented Apr 9, 2024 • edited

rtzoeller commented Apr 10, 2024 • edited

JavierMatosD commented Apr 10, 2024

dg0yt commented Apr 10, 2024 • edited

JavierMatosD commented Apr 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtzoeller Apr 12, 2024 • edited

Choose a reason for hiding this comment

JavierMatosD Apr 15, 2024 • edited

Choose a reason for hiding this comment

JavierMatosD commented Apr 12, 2024

JavierMatosD Apr 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtzoeller commented Apr 3, 2024 •

edited

rtzoeller commented Apr 5, 2024 •

edited

rtzoeller commented Apr 9, 2024 •

edited

rtzoeller commented Apr 9, 2024 •

edited

rtzoeller commented Apr 10, 2024 •

edited

dg0yt commented Apr 10, 2024 •

edited

rtzoeller Apr 12, 2024 •

edited

JavierMatosD Apr 15, 2024 •

edited

JavierMatosD Apr 15, 2024 •

edited