Skip to content

Conversation

@dreveman
Copy link
Collaborator

@dreveman dreveman commented Nov 3, 2025

Only FYI and don't prevent CI from being green.

Fixes: #3497

@dreveman dreveman requested a review from a team as a code owner November 3, 2025 22:48
@dreveman dreveman force-pushed the dev/reveman/rust-sdk-ci branch 2 times, most recently from 261eea4 to d82a50a Compare November 3, 2025 23:13
@dreveman dreveman force-pushed the dev/reveman/rust-sdk-ci branch 2 times, most recently from 3f239f0 to c62d5ea Compare November 3, 2025 23:27
@LalitMaganti LalitMaganti requested a review from primiano November 3, 2025 23:28
@LalitMaganti
Copy link
Member

LalitMaganti commented Nov 3, 2025

LGTM from my side but since this is a new Action, would ask Primiano also to comment on this just to double check I haven't missed anything from the security perspective.

@dreveman
Copy link
Collaborator Author

dreveman commented Nov 3, 2025

LGTM from my side but since this is a new Action, would ask Primiano also to comment on this just to double check I haven't missed anything from the security perspective.

Will do. No rush do merge this. But I uploaded #3513 in case you want to get a release out and have crates published before. As I've unfortunately not been able to claim the names without the -sdk suffix at this time.

@dreveman dreveman force-pushed the dev/reveman/rust-sdk-ci branch from c62d5ea to 552be7d Compare November 3, 2025 23:38
@LalitMaganti
Copy link
Member

Not particularly in a rush to get the release out. I think we can wait a few more days. Honestly renaming the crates sounds like a good idea anyway though given the situation. I'll LGTM that but let's wait a few more days to hear back.

Copy link
Member

@primiano primiano left a comment

Choose a reason for hiding this comment

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

Security-wise it seems alright

My only question is: how long does this take? Would it be possible / worth using caching for the installation of the toolchain like we do for our other prebuilts?

A bit lukewarm on the nightly format-check. That bot is doomed to become red and get ignored. Maybe it's better if for now this is only done locally.

You could (ask AI to) update our tools/format-sources, and create a tools/code_format_rust.py which runs only if the toolchain is installed.

Also separately we should probably have a --rust option to tools/install-build-deps which installs the relevant binaries to build rust (can do in a separate PR)

@dreveman dreveman force-pushed the dev/reveman/rust-sdk-ci branch from 552be7d to 8c2d59b Compare November 4, 2025 18:41
@dreveman
Copy link
Collaborator Author

dreveman commented Nov 4, 2025

Security-wise it seems alright

My only question is: how long does this take? Would it be possible / worth using caching for the installation of the toolchain like we do for our other prebuilts?

I don't know about installation of the toolchain but building and running it only takes a couple of minutes locally. I'll look at caching things once I know how long it takes in CI. Having trouble just testing it right now. Do I need the change to be approved before it runs more CI jobs?

A bit lukewarm on the nightly format-check. That bot is doomed to become red and get ignored. Maybe it's better if for now this is only done locally.

It uses a pinned version of nightly to ensure that the correct format doesn't change over time. I added some comments about this but can also remove the test for now if you prefer that?

You could (ask AI to) update our tools/format-sources, and create a tools/code_format_rust.py which runs only if the toolchain is installed.

Also separately we should probably have a --rust option to tools/install-build-deps which installs the relevant binaries to build rust (can do in a separate PR)

Yes, didn't know if you'd be OK with that but I think it would be a good idea to have install-build-deps be able to pull down the specific rust toolchains needed to build and test the rust SDK. Updating format-sources makes sense too. I'll work on both of these tasks as follow up changes.

@dreveman dreveman requested a review from primiano November 4, 2025 18:50
@dreveman
Copy link
Collaborator Author

dreveman commented Nov 4, 2025

hm, looks like I'm getting this error: https://github.com/google/perfetto/actions/runs/19079295703

[Invalid workflow file: .github/workflows/analyze.yml#L121](https://github.com/google/perfetto/actions/runs/19079295703/workflow)
The workflow is not valid. .github/workflows/analyze.yml (Line: 121, Col: 3): Error calling workflow 'google/perfetto/.github/workflows/rust-sdk-tests.yml@6de8b6d108f4610b5ff74c2d050ab87a0b472dd9'. The workflow is requesting 'contents: read', but is only allowed 'contents: none'.

@primiano
Copy link
Member

hm, looks like I'm getting this error: https://github.com/google/perfetto/actions/runs/19079295703

[Invalid workflow file: .github/workflows/analyze.yml#L121](https://github.com/google/perfetto/actions/runs/19079295703/workflow)
The workflow is not valid. .github/workflows/analyze.yml (Line: 121, Col: 3): Error calling workflow 'google/perfetto/.github/workflows/rust-sdk-tests.yml@6de8b6d108f4610b5ff74c2d050ab87a0b472dd9'. The workflow is requesting 'contents: read', but is only allowed 'contents: none'.

Hmm

  1. Any idea why do you need contents: read? what happens if you remove it altogether?
  2. If you end up needing it, my theory is that needs to be granted also to the "caller" workflow (analyze.yml). I guess github is preventing that a called workflow can end up acquiring extra privileges that the caller didn't want to grant

@dreveman dreveman force-pushed the dev/reveman/rust-sdk-ci branch 4 times, most recently from 0106453 to 13a858e Compare November 12, 2025 00:16
@dreveman dreveman force-pushed the dev/reveman/rust-sdk-ci branch from 13a858e to 898da1d Compare November 12, 2025 00:22
@dreveman dreveman force-pushed the dev/reveman/rust-sdk-ci branch from 898da1d to 12219cd Compare November 12, 2025 00:51
@primiano
Copy link
Member

I kicked off a rerun of the jobs to see if it works.
The comment from the security bot is interesting, it suggests we should use contents:read... but than it's what broke.

Out of curiosity did you try adding contents:read to both analyze.yml and the rust workflow?

@dreveman
Copy link
Collaborator Author

pull-requests: read seems to be working but this is running into some other confusing bindgen issues that I've never seen before and I need to figure out how to reproduce locally. I'm going to rebase this on https://github.com/google/perfetto/pull/3644 first and then investigate it more.

@dreveman dreveman force-pushed the dev/reveman/rust-sdk-ci branch from 12219cd to a1259c4 Compare November 12, 2025 22:35
Only FYI and don't prevent CI from being green.

Fixes: #3497
@dreveman dreveman force-pushed the dev/reveman/rust-sdk-ci branch from a1259c4 to 35a1548 Compare November 12, 2025 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add FYI CI job for Rust SDK

4 participants