Share dynamic values across dynamic_output actions #619
Conversation
facebook-github-bot
added
the
CLA Signed
Keyword arguments could absorb the fourth positional argument. So, we instead need to check if only three positional arguments are sufficient.
The lambda passed to dynamic_output must accept an additional positional argument for the resolved futures dictionary. The initial implementation of dynamic_value took place before the lambda parameter to dynamic_output was strictly typed. After rebasing on a later state of the Buck2 code base this caused issues.
This is no longer needed since we now always require a callback that takes a positional argument for `resolved` promises.
| // TODO(ah) consider merging with `dynamic`. | ||
| #[starlark(require = named, default = UnpackListOrTuple::default())] | ||
| promises: UnpackListOrTuple<DynamicValue>, |
There was a problem hiding this comment.
This is a breaking API change. The lambda that a user passes to dynamic_output now needs to take an additional positional argument. A problem is that it produces quite confusing errors on code that assumes the old API, because one of the commonly following keyword arguments with assigned default will be overridden with the additional positional argument. I'm open to suggestions for a different API. Perhaps this could be merged into the dynamic argument.
|
We have now successfully applied these changes on a Haskell project to achieve cross package module granular incremental builds. The dynamic value introduced here works to propagate module dependency graphs across package boundaries through tsets. I'd like to open this up for review and discuss whether the API change is acceptable. |
https://github.com/aherrmann/buck2/actions/runs/8755120386/job/24028493958#step:6:24 ``` prelude/android/android_binary_native_library_rules.bzl:244:67-75: Unused argument `resolved` prelude/android/android_binary_native_library_rules.bzl:1716:47-55: Unused argument `resolved` prelude/android/android_binary_native_library_rules.bzl:1755:39-47: Unused argument `resolved` prelude/android/android_binary_resources_rules.bzl:502:64-72: Unused argument `resolved` prelude/android/android_binary_resources_rules.bzl:594:67-75: Unused argument `resolved` prelude/android/dex_rules.bzl:147:55-63: Unused argument `resolved` prelude/android/dex_rules.bzl:544:74-82: Unused argument `resolved` prelude/android/dex_rules.bzl:439:63-71: Unused argument `resolved` prelude/apple/user/apple_selective_debugging.bzl:113:88-96: Unused argument `resolved` prelude/cxx/dist_lto/dist_lto.bzl:287:51-59: Unused argument `resolved` prelude/cxx/dist_lto/dist_lto.bzl:427:62-70: Unused argument `resolved` prelude/cxx/dist_lto/dist_lto.bzl:473:63-71: Unused argument `resolved` prelude/cxx/dist_lto/dist_lto.bzl:555:62-70: Unused argument `resolved` prelude/erlang/erlang_build.bzl:479:57-65: Unused argument `resolved` prelude/ocaml/ocaml.bzl:465:44-52: Unused argument `resolved` Command failed: Found 15 lints ```
```
error: `to_string()` called on a `&str`
--> app/buck2_build_api/src/interpreter/rule_defs/dynamic_value.rs:169:30
|
169 | .with_context(|| "Error accessing dynamic value".to_string())
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider using `.to_owned()`
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#str_to_string
```
|
Assigning the PR to myself. However, this week we have internal conference, and next week I'm on holidays. Can it wait two weeks @aherrmann? |
|
Hi @stepancheg, thank you for taking this on! Yes, of course, no problem if this waits two weeks. 🙂 |
|
@stepancheg I hope you had a good conference and vacation! Did you have a chance to take a look at this since you're back? |
|
@aherrmann working on it. Sorry for delay! |
|
No worries, I can relate. Thank you for looking into it! |
|
I'm working on some changes in Just FYI, this PR is not forgotten. |
|
@stepancheg Thanks for the update and for looking into it! Let me know if there's anything I can help with. |
Adds support for sharing values across dynamic_output actions.
The function given to a dynamic output action can now return a list of providers. The
ctx.actions.dynamic_outputfunction returns a handle to the future values of these providers. Another dynamic output action can take these handles in, which allows the function to resolve these handles into their resolved values. This way it is possible to share values that are obtained within a dynamic output lambda with another dynamic output lambda.Includes a small example demo
The motivation comes from Haskell builds, where we want to achieve cross package module granular builds as described in tweag/rules_haskell#1631. In that case we need to know the module dependency graph across package boundaries and track a
tsetfor each module that captures its transitive module dependencies across package boundaries. Without this feature this is not possible without piping these module dependency graphs through the filesystem and reloading and reanalyzing them over and over again in each packaeg's dynamic output action.