Skip to content

Conversation

@alexeagle
Copy link
Collaborator

pre-factoring for --noenable_workspace and Bazel 9

@fhanau I think you'll tell me that there's a dev workflow for updating the json file here which is load-bearing. We can script around buildozer to easily update the MODULE file with a lot less complexity.

@alexeagle alexeagle requested review from fhanau and mikea October 20, 2025 20:48
@alexeagle alexeagle requested review from a team as code owners October 20, 2025 20:48
@github-actions
Copy link

github-actions bot commented Oct 20, 2025

The generated output of @cloudflare/workers-types matches the snapshot in types/generated-snapshot 🎉

@fhanau
Copy link
Contributor

fhanau commented Oct 20, 2025

pre-factoring for --noenable_workspace and Bazel 9

@fhanau I think you'll tell me that there's a dev workflow for updating the json file here which is load-bearing. We can script around buildozer to easily update the MODULE file with a lot less complexity.

Yeah, if we can do it easily, I'm looking forward to a PR doing so 😄 – we often rely on updating capnproto quickly, and developers should only have to learn a new approach for updating capnproto once, so moving it when we have that set up would be best.

@alexeagle
Copy link
Collaborator Author

Can you give me an idea of what command users would want? Still update-deps.py or it can be a different script? Do we want it to take an argument like the SHA of capnp proto to point at, or just always update to HEAD?

@fhanau
Copy link
Contributor

fhanau commented Oct 21, 2025

Can you give me an idea of what command users would want? Still update-deps.py or it can be a different script? Do we want it to take an argument like the SHA of capnp proto to point at, or just always update to HEAD?

For now keeping the change transparent by keeping it within update-deps.py might be the most straightforward, although making it go through something else is fine if we document it properly.
I think we'll need both options – it should be easy to specify a custom commit (as with freeze_commit and freeze_version in the current update scheme), and to just update to the latest commit/version if the version isn't frozen.

pre-factoring for --noenable_workspace and Bazel 9
@alexeagle
Copy link
Collaborator Author

@fhanau okay, I've never used the update_deps.py script and don't want to spend much time reverse-engineering the command line API. I added a trivial script update-capnp.sh next to it which illustrates how easy this is with Bazel's tooling.

Could you pick this up and adjust to the CLI you think your users prefer?

Note that AFAIK your dev environment doesn't currently include buildozer. I recommend https://github.com/buildbuddy-io/bazel_env.bzl is the best way to add that, and will send a PR to add it if you like. If you have a different way of managing developer's environments then add it there?

@fhanau
Copy link
Contributor

fhanau commented Oct 21, 2025

@fhanau okay, I've never used the update_deps.py script and don't want to spend much time reverse-engineering the command line API. I added a trivial script update-capnp.sh next to it which illustrates how easy this is with Bazel's tooling.

Could you pick this up and adjust to the CLI you think your users prefer?

Note that AFAIK your dev environment doesn't currently include buildozer. I recommend https://github.com/buildbuddy-io/bazel_env.bzl is the best way to add that, and will send a PR to add it if you like. If you have a different way of managing developer's environments then add it there?

I'll try to take a look tomorrow. @mikea has the most context on the update-deps script, he might have good ideas here too.

@alexeagle
Copy link
Collaborator Author

Interesting single failure currently:

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
external/+_repo_rules+capnp-cpp/src/capnp/arena.h:25:2: error: "This header is only meant to be included by Cap'n Proto's own source code."
   25 | #error "This header is only meant to be included by Cap'n Proto's own source code."
      |  ^
1 error generated.

Somehow this wasn't tickled when it was imported via WORKSPACE rather than bzlmod...

@fhanau
Copy link
Contributor

fhanau commented Oct 22, 2025

Interesting single failure currently:

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
external/+_repo_rules+capnp-cpp/src/capnp/arena.h:25:2: error: "This header is only meant to be included by Cap'n Proto's own source code."
   25 | #error "This header is only meant to be included by Cap'n Proto's own source code."
      |  ^
1 error generated.

Somehow this wasn't tickled when it was imported via WORKSPACE rather than bzlmod...

That's due to the mangled repo name – we set build:parse_headers --per_file_copt=external/capnp-cpp/src/capnp/arena.h@-DCAPNP_PRIVATE in .bazelrc.

under bzlmod you can't assume a given path to external/ repo
@alexeagle
Copy link
Collaborator Author

Thanks for pointing to that @fhanau - green now.

@mikea do you want to hold this PR for ergonomics of the update script?

@fhanau
Copy link
Contributor

fhanau commented Oct 23, 2025

Thanks for pointing to that @fhanau - green now.

@mikea do you want to hold this PR for ergonomics of the update script?

Yup – we discussed this internally having a user-friendly way to update deps is a priority, something we'd want to have before transitioning capnproto.

@codspeed-hq
Copy link

codspeed-hq bot commented Oct 23, 2025

CodSpeed Performance Report

Merging #5357 will degrade performances by 25.6%

Comparing rm_deps_capnp (521b7a3) with main (9287cf6)

Summary

⚡ 2 improvements
❌ 3 regressions
✅ 28 untouched
⏩ 9 skipped1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
Encode_ASCII_8192[0/0/8192] 5.6 s 7.5 s -25.6%
Encode_OneByte_1024[0/1/1024] 3 s 1.2 s ×2.5
Encode_OneByte_8192[0/1/8192] 8.4 s 9.2 s -8.26%
Encode_TwoByte_1024[0/2/1024] 8.8 s 1.9 s ×4.5
Encode_TwoByte_8192[0/2/8192] 17.4 s 21.6 s -19.68%

Footnotes

  1. 9 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@alexeagle
Copy link
Collaborator Author

I'm happy to work more on this script, could we land this one since it's green? @mikea let's chat on Slack about the design of the script, I think you simply want to advance to HEAD of v2 branch regularly?

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.

2 participants